Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
solprofi opened this issue Feb 8, 2024 · 7 comments · Fixed by #6459
Closed

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

solprofi opened this issue Feb 8, 2024 · 7 comments · Fixed by #6459

Comments

@solprofi
Copy link

solprofi commented Feb 8, 2024

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 solprofi added Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. Question labels Feb 8, 2024
@robwalch
Copy link
Collaborator

robwalch commented Feb 8, 2024

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 robwalch added Need info and removed Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Feb 9, 2024
@xta
Copy link
Contributor

xta commented May 29, 2024

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.

@robwalch
Copy link
Collaborator

robwalch commented May 29, 2024

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).

@xta
Copy link
Contributor

xta commented May 29, 2024

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

@robwalch
Copy link
Collaborator

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.

@xta
Copy link
Contributor

xta commented May 29, 2024

@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!

@robwalch robwalch linked a pull request May 29, 2024 that will close this issue
3 tasks
@robwalch robwalch added this to the 1.5.10 milestone May 29, 2024
@robwalch
Copy link
Collaborator

robwalch commented May 31, 2024

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 muxinc/elements@61a4cfe (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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants