hls.js icon indicating copy to clipboard operation
hls.js copied to clipboard

How to prevent an error that xhrSetup throws after package migration to v1.5.0?

Open solprofi opened this issue 1 year ago • 1 comments

What do you want to do with Hls.js?

Hi! I'm using hls.js in my React project.

After I updated the package from v1.4.14 to v1.5.0, I started seeing a new error in the console:

Uncaught (in promise) TypeError: Cannot read properties of null (reading 'onError')

It's coming from an internal method loadInternal (screenshot attached), which seems to be affected by xhrSetup.

image image

According to the docs, if xhrSetup throws, the error will be caught, and xhrSetup will be called a second time, which is exactly what's happening. The difference being that without any code changes from my side, the error is logged to the console in the latest version of the package.

I'm not doing anything special in my xhrSetup method:

      xhrSetup: (xhr, uri) => {
        // some one-liner here that's using xhr.withCredentials 
      },

And if I remove this method from my hls config, the error goes away.

However, I haven't seen this error in the previous version of the package and I can't find anything related to this in the release changelog.

Could someone please point out if I can do anything to prevent this specific error from happening?

What have you tried so far?

No response

solprofi avatar Feb 08 '24 18:02 solprofi

Hi @solprofi,

Please provide a standalone example that reproduces the issue.

Based on where the catch is something is throwing in openAndSendXhr - presumably, your xhrSetup call or the call to open a bad request.

What is the value of this in that catch block? What kind of request is it (playlist, fragment, key, steering manifest, something else)?

robwalch avatar Feb 08 '24 19:02 robwalch

Hi @robwalch, great work on hls!

I'm seeing this.stats as null in both line 103 & 111.

So I'm seeing the errors TypeError: Cannot read properties of null (reading 'aborted') and related Unhandled Runtime Error TypeError: Cannot read properties of null (reading 'onError').

Note: I'm using this hls as a dependency of another lib.

Like @solprofi, I'm able to work around this by passing a config xhrSetup: undefined, but I would prefer not to have to and use XMLHttpRequest as configured.

xta avatar May 29 '24 08:05 xta

I'm seeing this.stats as null in both line 103 & 111.

If you submit a PR that adds optional chaining operators to this.stats and this.callbacks in the Promise handlers (this.stats.aborted -> this.stats?.aborted) we can get that in a patch (assuming it fixes your issue).

robwalch avatar May 29 '24 14:05 robwalch

Thanks @robwalch I created a PR that fixes the null errors I was encountering

xta avatar May 29 '24 21:05 xta

Thanks for the PR @xta. I'll get it reviewed and merged. Do you need it in a patch for v1.5.x?

I would still appreciate if you could provide steps to reproduce in this issue. Looking at the code I'm assuming that the instance of Hls or the loader with an xhrSetup callback is being destroyed on load, synchronously after loadInternal is called, before the async callback that checks stats.

robwalch avatar May 29 '24 21:05 robwalch

@robwalch v1.5.x would be great

I'm sorry I can't provide you with my next app to repro. The xhrSetup is here https://github.com/muxinc/elements/blob/main/packages/playback-core/src/index.ts#L540-L546

Thanks for your PR feedback!

xta avatar May 29 '24 21:05 xta

I'm sorry I can't provide you with my next app to repro. The xhrSetup is here https://github.com/muxinc/elements/blob/main/packages/playback-core/src/index.ts#L540-L546

It's probably worth commenting on https://github.com/muxinc/elements/commit/61a4cfe5d50431e9b38a8b43f8ea1d71c0d1521e (cc @cjpillsbury) that the HlsConfig cmcd option takes a list of keys to include via HlsConfig.cmcd.includeKeys https://github.com/video-dev/hls.js/blob/7c9e9a40ff0e5351a1671ed385065535d6f74778/src/controller/cmcd-controller.ts#L58C12-L58C23

Using includeKeys would be more performant than xhrSetup because it doesn't schedule request start on the microtask queue.

robwalch avatar May 31 '24 20:05 robwalch