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

Worker source handoff is fragile and likely to break with bundlers/minifiers #5107

Closed
5 tasks done
issacgerges opened this issue Dec 14, 2022 · 7 comments · Fixed by #5299
Closed
5 tasks done

Worker source handoff is fragile and likely to break with bundlers/minifiers #5107

issacgerges opened this issue Dec 14, 2022 · 7 comments · Fixed by #5299

Comments

@issacgerges
Copy link

issacgerges commented Dec 14, 2022

What version of Hls.js are you using?

1.2.9

What browser (including version) are you using?

Chrome 108.0.5359.98 (Official Build) (x86_64)

What OS (including version) are you using?

MacOS Monterey 12.6.1

Test stream

No response

Configuration

{}

Additional player setup steps

No response

Checklist

Steps to reproduce

  1. Build hls.js with esbuild with a flag like keep-names
  2. Attempt to load any stream with any config where the worker isn't forcibly disabled

Expected behaviour

The stream should load normally

What actually happened?

The stream will fail to load

Console output

Uncaught ReferenceError: __name is not defined

or, minified

Uncaught ReferenceError: t is not defined

This was previously reported in #3749, but closed as there was no repro and the report was unspecific

@issacgerges issacgerges added Bug Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Dec 14, 2022
@issacgerges
Copy link
Author

issacgerges commented Dec 14, 2022

In order to load its worker, hls.js creates a blob url using Function.prototype.toString of a module in webpacks module cache, as well as some injected bootstrap code by a forked version of webworkify-webpack.js. The use of toString here can break in unexpected ways when a minifier modifies the code.

When using esbuild, runtime like the following is injected at the top of the script (in an iife)

  var __create = Object.create;
  var __defProp = Object.defineProperty;
  var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
  var __getOwnPropNames = Object.getOwnPropertyNames;
  var __getProtoOf = Object.getPrototypeOf;
  var __hasOwnProp = Object.prototype.hasOwnProperty;
  var __name = (target, value) => __defProp(target, "name", { value, configurable: true });

These helpers are lexically scoped to now mangled code that relies on them. Any calls to those functions added to transmuxer-worker will be broken when that modules contents are accessed as a string via toString().

When the worker loads this string via blob url, all of those functions are now undefined and will cause the runtime error described above.

This type of issue is described in the esbuild docs here but are likely not specific to only it.

I think most of this issue could be avoided by adding a workerSrc (or similar) param to the config object that allows the consumer to minify this file themselves and make it available. This file would also need to be available via a predictable entry point (like hls.js/worker)

@issacgerges
Copy link
Author

issacgerges commented Dec 14, 2022

I've provided a minimal repro here. Also, here is a report from a different library showing this class of issue can occur in Terser as well

@issacgerges
Copy link
Author

Hey @robwalch, any good options here? We've got a workaround in place where we disable terser on this bundle and consume hls.min.js directly, but we'd love to move to something a bit less fragile.

@robwalch
Copy link
Collaborator

robwalch commented Jan 3, 2023

Hi @issacgerges,

I'd like to maintain the current ease of use at all costs.

Supporting an optional/alternative loaded worker script when defined in the config is a good idea. I would review and be open to accepting a contribution for this. Any such change should not break the default worker injection.

In terms of fragility with minifiers and manglers, I would also accept changes like #5036 that improve support for changes made to minified code with certain options enabled.

Whether the default worker injection is enabled or an optional worker script is configured, if either method failed (A: to run the worker or B: the main player could not ack it), operation should fallback to workerless mode. This was implemented in #5016. I expect HLS.js to run even with your repro steps when using the latest player version. If that is not the case, let me know. Forcing enableWorker to false should not be required.

@robwalch
Copy link
Collaborator

Hi @issacgerges,

I'd like to get your feedback on #5299. Would it solve or at least prevent some of the bundling issues you've described?

cc @laurens94 and @dev-justin

robwalch added a commit that referenced this issue Mar 17, 2023
robwalch added a commit that referenced this issue Mar 17, 2023
Add an es module target (#2910)
UMD build worker injection (#5107)
Add ES5 syntax check for UMD builds (#5301, #5288)
robwalch added a commit that referenced this issue Mar 23, 2023
Add an es module target (#2910)
UMD build worker injection (#5107)
Add ES5 syntax check for UMD builds (#5301, #5288)
robwalch added a commit that referenced this issue Mar 24, 2023
Add an es module target (#2910)
UMD build worker injection (#5107)
Add ES5 syntax check for UMD builds (#5301, #5288)
robwalch added a commit that referenced this issue Mar 27, 2023
Add an es module target (#2910)
UMD build worker injection (#5107)
Add ES5 syntax check for UMD builds (#5301, #5288)
robwalch added a commit that referenced this issue Mar 27, 2023
)

* Convert build packager to rollup
* Add an es module target (#2910)
* UMD build worker injection (#5107)
* Add ES5 syntax check for UMD builds (#5301, #5288)
* Add common rollup config
* handle `self` not existing (happens in nodejs. The check that makes sure the dist file can be required in node and not throw was failing because of this.)
* Disable coverage in CI
* Add `config.workerPath` option and "hls.worker.js" build output (#5107)
* Include transmuxer-interface id ("main" and "audio") in Web Worker setup and error logs

Co-authored-by: Tom Jenkinson <tom@tjenkinson.me>
@robwalch
Copy link
Collaborator

Version 1.4 is in beta now. We've switched from webpack to rollup for packaging. The library now includes ES module output with "hls.mjs", and a separate worker script "hls.worker.js" which you can point to via config.workerPath should you choose to use the ESM output or repackage HLS.js such that the UMD worker injection is no longer available. The worker injection is also much simpler, and the player will fallback to inline demuxing when it fails (this issue).

https://github.com/video-dev/hls.js/releases/tag/v1.4.0-beta.2

cjpillsbury added a commit to muxinc/elements that referenced this issue Jul 7, 2023
#718)

…, since this appears to cause edge case issues with initial times after
ts segment transmuxing.

To test Next.js usage:
- confirm playbackId `8xhTU2GpKuskv1eCx8rdp3PodlGb6dABCf27f4BsmW8` does
not continually load the 0th segment before playback
- confirm that playback begins at time 0
- confirm that you can seek back to time 0 after playback has begun

**NOTE**: We originally used the minified version of hls.js due to
issues with svelte-kit and Vue minified builds, with a root cause of how
its web worker was integrated. This appears to be resolved since hls.js
has migrated its build setup to rollup. Confirmed locally that there
were no issues with svelte-kit, but make sure to do additional smoke
testing for our Vue and Svelte apps before any approval. For reference,
see:

- #541
- video-dev/hls.js#5107
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.

2 participants