1479245 - Don't load any pdf.js scripts in the content process before they're needed

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[Kris Maglione [:kmag]]

There's no need to load any JS for pdf.js into the content process before something tries to instantiate one of its contract IDs. We have mechanisms for handling this, and should use them.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

Review commit: https://reviewboard.mozilla.org/r/260146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260146/

Comment 2

[Brendan Dahl [:bdahl]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

https://reviewboard.mozilla.org/r/260146/#review267584

::: browser/extensions/pdfjs/content/moz.build:9
(Diff revision 1)
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXTRA_COMPONENTS += [
> +    'pdfjs.js',
> +    'pdfjs.manifest',

I wanted to check a few things locally, but this isn't building. Appears this file is missing?

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

https://reviewboard.mozilla.org/r/260146/#review267584

> I wanted to check a few things locally, but this isn't building. Appears this file is missing?

Yeah, sorry about that. I apparently forgot to check it in. I'll update it.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260146/diff/1-2/

Comment 5

[Brendan Dahl [:bdahl]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

https://reviewboard.mozilla.org/r/260146/#review267614

::: browser/extensions/pdfjs/content/PdfJs.jsm
(Diff revision 2)
>  
>      initializeDefaultPreferences();
>    },
>  
> -  updateRegistration: function updateRegistration() {
> -    if (this.checkEnabled()) {

Now that this is no longer called, the pref that caches the state of whether pdf.js is enabled is no longer updated.

For example, setting pdfjs.disabled to true no longer does anything.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260146/diff/2-3/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260146/diff/3-4/

Comment 8

[Brendan Dahl [:bdahl]]

Comment on attachment 8995770 [details]
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.

https://reviewboard.mozilla.org/r/260146/#review267912

Looks good! Things are much simpler now that we don't need to be a bootstrappable extension. I should note, toggling the pdfjs.disabled pref now requires a restart, but I'm fine with that since it simplifies other things.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Brendan Dahl [:bdahl] from comment #8)
> Comment on attachment 8995770 [details]
> Bug 1479245: Don't eagerly load any PDF.js scripts in the content process.
> 
> https://reviewboard.mozilla.org/r/260146/#review267912
> 
> Looks good! Things are much simpler now that we don't need to be a
> bootstrappable extension. I should note, toggling the pdfjs.disabled pref
> now requires a restart, but I'm fine with that since it simplifies other
> things.

Oh, I think I actually fixed that after I noticed a failure in my last try run (I missed it in the first one, because it was mixed in with some unrelated errors) by changing the observer to do:

    Services.ppmm.sharedData.set("pdfjs.enabled", this.checkEnabled());

rather than:

    Services.ppmm.sharedData.set("pdfjs.enabled", this.enabled);

I guess I forgot to rebase that onto the original patch before I pushed to review.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ccbdcaa8a1e82b43406540c76ebac4236581ee
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process. r=bdahl

Comment 11

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Linux%20debug%20Mochitests%20with%20e10s%20test-linux32%2Fdebug-mochitest-browser-chrome-e10s-9%20M-e10s(bc9)&fromchange=f150e62dcbbdc14bec93db0705470a5d9e71737e&tochange=97465ceca62ccbf4363105561e9a0515df660ca3&selectedJob=191498401

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=191498401&repo=mozilla-inbound&lineNumber=7343
https://treeherder.mozilla.org/logviewer.html#?job_id=191498250&repo=mozilla-inbound&lineNumber=26182

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/97465ceca62ccbf4363105561e9a0515df660ca3

There were a rise of intermittents with these line - 
TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_savedialog.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_views.js | Test timed out -

[task 2018-08-02T00:48:25.329Z] 00:48:25     INFO - TEST-PASS | browser/extensions/pdfjs/test/browser_pdfjs_views.js | pdf handler defaults to internal - 
[task 2018-08-02T00:48:25.331Z] 00:48:25     INFO - Pref action: 3
[task 2018-08-02T00:48:25.334Z] 00:48:25     INFO - Buffered messages logged at 00:46:55
[task 2018-08-02T00:48:25.336Z] 00:48:25     INFO - Console message: [JavaScript Error: "uncaught exception: 2147746132"]
[task 2018-08-02T00:48:25.339Z] 00:48:25     INFO - Console message: [JavaScript Error: "uncaught exception: 2147746132"]
[task 2018-08-02T00:48:25.341Z] 00:48:25     INFO - Buffered messages finished
[task 2018-08-02T00:48:25.344Z] 00:48:25     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_views.js | Test timed out - 
[task 2018-08-02T00:48:25.349Z] 00:48:25     INFO - GECKO(3070) | MEMORY STAT | vsize 616MB | residentFast 244MB | heapAllocated 61MB
[task 2018-08-02T00:48:25.352Z] 00:48:25     INFO - TEST-OK | browser/extensions/pdfjs/test/browser_pdfjs_views.js | took 90180ms
[task 2018-08-02T00:48:25.354Z] 00:48:25     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-08-02T00:48:25.355Z] 00:48:25     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_views.js | Found a tab after previous test timed out: about:blank - 
[task 2018-08-02T00:48:25.358Z] 00:48:25     INFO - GECKO(3070) | ++DOCSHELL 0xe9226c00 == 1 [pid = 3185] [id = {76b7311f-8709-4e92-a560-bba84daaeba3}]
[task 2018-08-02T00:48:25.359Z] 00:48:25     INFO - GECKO(3070) | ++DOMWINDOW == 1 (0xf71e9160) [pid = 3185] [serial = 5] [outer = (nil)]
[task 2018-08-02T00:48:25.363Z] 00:48:25     INFO - GECKO(3070) | ++DOMWINDOW == 2 (0xe995cc00) [pid = 3185] [serial = 6] [outer = 0xf71e9160]
[task 2018-08-02T00:48:25.365Z] 00:48:25     INFO - checking window state

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5e9e5994a5d2dcfa1fe83c19101bb92cdf7ff4c
Bug 1479245: Don't eagerly load any PDF.js scripts in the content process. r=bdahl

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f5e9e5994a5d