Closed Bug 1479245 Opened Last year Closed Last year

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

Categories

(Firefox :: PDF Viewer, enhancement)

57 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
See Also: → 1371340
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 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 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 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.
Attachment #8995770 - Flags: review?(bdahl) → review+
(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.
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
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/f5e9e5994a5d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1481859
Depends on: 1496855
Blocks: 1559195
You need to log in before you can comment on or make changes to this bug.