Fix exceptions in IE #1786

johnBartos · 2018-06-15T18:24:27Z

This PR will...

Revert the Array.from changes
Avoid throwing an avoidable exception accessing crypto
Avoid reassigning window

Why is this Pull Request needed?

The Array.from changes cause IE to fail with an unhandled exception:

2018-06-15 16:29:53.019: "hlsjs fatal error :internalException"
2018-06-15 16:29:53.020: "exception in :demuxerWorker"
2018-06-15 16:29:53.020: "Function.prototype.call: 'this' is not a Function object (blob:D44328A0-4D47-40C3-9929-A44E11920A9B:1)"

These changes are not valuable so they have been reverted.

Makes it a bit easier to debug when the all exceptions flag does not get triggered due to a caught exception.
It is a bad practice to reassign globals.

Are there any points in the code the reviewer needs to double check?

I don't think so. I did a spot check on IE/Edge/Chrome/FF and everything looks fine.

Resolves issues:

Reported via video-dev slack

Checklist

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

… redundant window code

This reverts commit 2306a46.

tjenkinson · 2018-06-15T19:29:09Z

#1769 could work in place of some of these commits. Understand if we'd rather hold that back for now though and get a new version out

johnBartos · 2018-06-15T19:38:37Z

@tjenkinson Would like to take a closer look at that one. I think we need to get this in & cut a release asap

tchakabam · 2018-06-15T21:51:39Z

src/remux/mp4-generator.js

@@ -3,7 +3,6 @@
 */

 // import Hex from '../utils/hex';


if this import isn't needed we should remove it

tchakabam · 2018-06-15T21:52:54Z

src/remux/mp4-generator.js

-      sps = sps.concat(makeArrayFromArrayLike(data)); // SPS
+
+      // SPS
+      if (typeof Array.from === 'function') {


not sure what is broken in there, but: why don't you fix the util function (makeArrayFromArrayLike)?

the way you did it here, we will have more code-size if this functionnality is needed somewhere else.

update: see my proposed fix further down

tchakabam · 2018-06-15T21:54:09Z

src/crypt/decrypter.js

-const window = getSelfScope(); // safeguard for code that might run both on worker and main thread
-
-const { performance, crypto } = window;
+const global = getSelfScope(); // safeguard for code that might run both on worker and main thread


if this is a problem on IE11, can we add a linter setting that forbids the variable name window? it would be the way to avoid regression in this case.

or this is what no-global-assign does?

Yeah no-global-assign does that

tchakabam · 2018-06-15T21:54:47Z

.eslintrc.js

@@ -83,6 +83,7 @@ module.exports = {
    'no-irregular-whitespace': 1,
    'no-self-assign': 1,
    'new-cap': 1,
-    'no-undefined': 1
+    'no-undefined': 1,
+    'no-global-assign': 1


if this is a critical issue, you shouldn't make it a warning, but an error (2).

Ah thanks. Will address

tchakabam · 2018-06-15T21:58:58Z

src/remux/mp4-generator.js

@@ -344,7 +349,12 @@ class MP4 {
      len = data.byteLength;
      pps.push((len >>> 8) & 0xFF);
      pps.push((len & 0xFF));
-      pps = pps.concat(makeArrayFromArrayLike(data));
+
+      if (typeof Array.from === 'function') {


like this is actually redundant code really growing the package, isn't it?

it seems more intuitive to rather fix the issue here: https://github.com/video-dev/hls.js/blob/master/src/utils/make-array-from-array-like.js#L1

if the current state doesn't work...I guess you need to change it to something like:

export function makeArrayFromArrayLike(arrayLike) { return (typeof Array.from === 'function' ? Array.from(arrayLike) : Array.prototype.slice.call(arrayLike)); }

and you can use it without changes the exact same way.

IE11 seems to have a bug in exporting native prototype members :)

tchakabam · 2018-06-15T22:06:08Z

@tjenkinson let's first get this fixed based on the status quo, and after that merge your PR asap.

tchakabam · 2018-06-15T22:08:03Z

src/remux/mp4-generator.js

@@ -3,7 +3,6 @@
 */

 // import Hex from '../utils/hex';
-import { makeArrayFromArrayLike } from '../utils/make-array-from-array-like';


if you wanted to remove it, we should have deleted the module as well. but imo we should definitely keep it.

tchakabam · 2018-06-15T22:09:50Z

@johnBartos it seems you fixed this without completely understanding what the issue was :) i hope my suggestion may help!

johnBartos · 2018-06-15T22:33:04Z

@tchakabam Ah looks like I reverted the wrong commit. Will fix

johnBartos · 2018-06-15T22:35:55Z

@tchakabam Fixed. That's what I get for rushing 😅

tchakabam · 2018-06-15T23:10:06Z

@johnBartos Awesome work!

Also I need to admit that I found out: This util function was a wrong thing to do in the first place the way it was done. One can't export a call function in general 8| this is something for exampleFirefox also doesn't get right. We could have done it right by setting the Array.from property globally to something implemented by slice where its not defined, as in a proper polyfill.

Sorry, my bad :/

tchakabam · 2018-06-15T23:20:52Z

or something like this would have worked: export const makeArrayFromArrayLike = typeof Array.from === 'function' ? Array.from : ((arrayLike) => Array.prototype.slice.call(arrayLike));

John Bartos added 4 commits June 15, 2018 14:07

Avoid accessing undefined variable, avoid reassigning window, cleanup…

3630bf9

… redundant window code

Revert "Use wrapper function"

9b5d520

This reverts commit 2306a46.

Avoid more reassignments of window

1879842

Avoid throwing a preventable exception accessing crypto

452865e

johnBartos added the Bug label Jun 15, 2018

johnBartos added this to the 0.10.1 milestone Jun 15, 2018

johnBartos requested review from tchakabam, mangui and tjenkinson June 15, 2018 18:24

John Bartos added 2 commits June 15, 2018 14:31

Add no-global-assing lint rule

bdacea1

Remove useless lint rules

00d7421

tjenkinson approved these changes Jun 15, 2018

View reviewed changes

tchakabam suggested changes Jun 15, 2018

View reviewed changes

tchakabam reviewed Jun 15, 2018

View reviewed changes

Remove all instances of Array.from

ee43967

John Bartos added 2 commits June 15, 2018 18:38

Remove hex import

674ce79

Revert dist

5b1c417

tchakabam approved these changes Jun 15, 2018

View reviewed changes

tchakabam merged commit 26bbb62 into master Jun 16, 2018

tchakabam deleted the bugfix/ie-exceptions branch June 16, 2018 16:09

renovate bot mentioned this pull request Jun 6, 2021

chore(deps): update dependency hls.js to v0.14.17 neuron-digital/wjplayer#34

Open

1 task

renovate bot mentioned this pull request Aug 13, 2022

Update dependency hls.js to v0.14.17 phpb-com/video-react#69

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix exceptions in IE #1786

Fix exceptions in IE #1786

johnBartos commented Jun 15, 2018

tjenkinson commented Jun 15, 2018

johnBartos commented Jun 15, 2018

tchakabam Jun 15, 2018

tchakabam Jun 15, 2018

tchakabam Jun 15, 2018

tchakabam Jun 15, 2018

tchakabam Jun 15, 2018

johnBartos Jun 15, 2018

tchakabam Jun 15, 2018

johnBartos Jun 15, 2018

tchakabam Jun 15, 2018 •

edited

Loading

tchakabam commented Jun 15, 2018 •

edited

Loading

tchakabam Jun 15, 2018 •

edited

Loading

tchakabam commented Jun 15, 2018

johnBartos commented Jun 15, 2018

johnBartos commented Jun 15, 2018

tchakabam commented Jun 15, 2018

tchakabam commented Jun 15, 2018 •

edited

Loading

Fix exceptions in IE #1786

Fix exceptions in IE #1786

Conversation

johnBartos commented Jun 15, 2018

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

tjenkinson commented Jun 15, 2018

johnBartos commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

tchakabam commented Jun 15, 2018 • edited Loading

tchakabam Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

tchakabam commented Jun 15, 2018

johnBartos commented Jun 15, 2018

johnBartos commented Jun 15, 2018

tchakabam commented Jun 15, 2018

tchakabam commented Jun 15, 2018 • edited Loading

tchakabam Jun 15, 2018 •

edited

Loading

tchakabam commented Jun 15, 2018 •

edited

Loading

tchakabam Jun 15, 2018 •

edited

Loading

tchakabam commented Jun 15, 2018 •

edited

Loading