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

Detect premature worker load error #7107

Merged
merged 1 commit into from
Apr 2, 2016

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Mar 23, 2016

Rejects the ready promise when the worker fails to load, e.g. due to network errors.

@kyoshino
Copy link

kyoshino commented Apr 2, 2016

This PR is required for Firefox 45+, because new Worker() no longer throws a SecurityError when the worker failed to load due to an origin issue, according to Bug 1260388 and Bug 1260961. Any load errors must be caught with the error handler, otherwise the fallback function will never get called. See my site compatibility doc for details.

@@ -1302,11 +1302,25 @@ var PDFWorker = (function PDFWorkerClosure() {
// https://bugzilla.mozilla.org/show_bug.cgi?id=683280
var worker = new Worker(workerSrc);
var messageHandler = new MessageHandler('main', 'worker', worker);
var terminateEarly = function() {
this._readyCapability.reject(new Error('Worker was destroyed'));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._setupFakeWorker(); is needed here to call the fallback function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; I've overwritten the previous commit, with the following changes (see PR for full change set).

@@ -1303,15 +1303,22 @@ var PDFWorker = (function PDFWorkerClosure() {
           var worker = new Worker(workerSrc);
           var messageHandler = new MessageHandler('main', 'worker', worker);
           var terminateEarly = function() {
-            this._readyCapability.reject(new Error('Worker was destroyed'));
-            messageHandler.destroy();
             worker.removeEventListener('error', onWorkerError);
+            messageHandler.destroy();
             worker.terminate();
+            if (this.destroyed) {
+              this._readyCapability.reject(new Error('Worker was destroyed'));
+            } else {
+              // Fall back to fake worker if the termination is caused by an
+              // error (e.g. NetworkError / SecurityError).
+              this._setupFakeWorker();
+            }
           }.bind(this);

           var onWorkerError = function(event) {
-            if (!event.message && !this.destroyed && !this._webWorker) {
-              // Worker failed to initialize due to a load error.
+            if (!this._webWorker) {
+              // Worker failed to initialize due to an error. Clean up and fall
+              // back to the fake worker.
               terminateEarly();
             }   
           }.bind(this);

@Snuffleupagus
Copy link
Collaborator

This PR is required for Firefox 45+, because new Worker() no longer throws a SecurityError when the worker failed to load due to an origin issue, according to Bug 1260388 and Bug 1260961.

If I'm understanding the bugs correctly, this issue only affect custom deployments of PDF.js, and not the version built-in to Firefox!?
That seems like an important distinction to me, since it means we won't need to uplift the fix to Firefox.

@kyoshino
Copy link

kyoshino commented Apr 2, 2016

I think the build-in version in Firefox is NOT affected.

Fall back to a fake worker if the worker fails to load or initialize,
e.g. due to a network error, a security error or simply a script error.
@yurydelendik
Copy link
Contributor

Looks good.

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2016

From: Bot.io (Windows)


Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/f90f21d7f244cd5/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2016

From: Bot.io (Linux)


Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/75c21ae65928459/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2016

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/f90f21d7f244cd5/output.txt

Total script time: 20.08 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Apr 2, 2016

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/75c21ae65928459/output.txt

Total script time: 22.68 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@yurydelendik yurydelendik merged commit 055d642 into mozilla:master Apr 2, 2016
@yurydelendik
Copy link
Contributor

Thank you for the patch.

@kyoshino
Copy link

kyoshino commented Apr 2, 2016

Awesome, thank you!

@kyoshino
Copy link

kyoshino commented Apr 2, 2016

@yurydelendik Any ETA on PDF.js v1.4.187?

@yurydelendik
Copy link
Contributor

Any ETA on PDF.js v1.4.187?

@kyoshino you can download library from the mozilla/pdfjs-dist repo. the viewer package will be available as soon we are release next beta. You can build it yourself using gulp generic.

@kyoshino
Copy link

kyoshino commented Apr 2, 2016

Oh I see, thanks. I'll mention it in my site compat doc.

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

Successfully merging this pull request may close these issues.

6 participants