1352218 - Don't check PdfJs.enabled in the child process bootstrap script

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[Andrew McCreight [:mccr8]]

Right now, nsBrowserGlue.js loads the process script pdfjschildbootstrap.js, which calls PdfJs.updateRegistration() in the child process. This in turn calls PdfJs.enabled.

This is bad for two reasons:
1. It requires a sync IPC call to the parent (where we just were).
2. It requires an import of PdfjsContentUtils.jsm.

To fix this, I split the bootstrap file into two, one which does the PdfjsContentUtils part and one which does the PdfJs part. We only load the latter process script when PdfJs.enabled is true in the parent process, so we can skip the call to .enabled.

Comment 1

[Andrew McCreight [:mccr8]]

This is my pull request in the PDF.js repo:
  https://github.com/mozilla/pdf.js/pull/8218

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1352218, part 1 - Split pdf.js boot strap file.

One is always run, the other is only run when PdfJs.enabled is true in
the parent process.

The extensions changes are from:
  https://github.com/mozilla/pdf.js/pull/8218

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

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1352218, part 2 - Avoid PdfJs.enabled call.

Changes entirely from:
  https://github.com/mozilla/pdf.js/pull/8218

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

Comment 4

[:Gijs (he/him)]

I ran into this earlier when poking at bug 1352348 because we hit pdfjs.enabled for every new tab that gets opened (on my machine), from the content process, and that triggers a sync content-to-parent message that then hits the registry (on Windows, anyway). Is there a separate bug to remove the sync messaging entirely? Feels like we should be able to propagate this state to the content process from the parent rather than requiring a sync message whenever the content process needs to check it...

Comment 5

[Andrew McCreight [:mccr8]]

I'm not familiar with any such bug. I've been focused on avoiding jsm loads rather than sync messages, but this just came up as I was working on something.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8853183 [details]
Bug 1352218, part 1 - Split pdf.js boot strap file.

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

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8853184 [details]
Bug 1352218, part 2 - Avoid PdfJs.enabled call.

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

Comment 8

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17da77fd8d891fef65818faff8265f17fadbf109

If the patches look okay, could you also merge my pdf.js pull request, Yury? Thanks.

Comment 9

[Andrew McCreight [:mccr8]]

Maybe you could look at this, Gijs? Only the nsBrowserGlue.js change really needs review. The rest of the code in these patches has already been landed upstream.

Comment 10

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cb195247ca45e64cc0c93efa00374164c4c5333

Comment 11

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Maybe you could look at this, Gijs? Only the nsBrowserGlue.js change really
> needs review. The rest of the code in these patches has already been landed
> upstream.

Oh, hm. The patches look to me like they'd make pdfjs not load if it's not enabled when the first window shows up. Doesn't look like you're adding anything to post-load the '-enabled' version of the file if that changes at runtime. In particular, AIUI there's UI for this in the 'Applications' (Now "Downloads and Links") in the prefs/options... it would be a shame if we're essentially breaking the 'turn pdfjs on' version of that UI...

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to :Gijs from comment #11)
> Oh, hm. The patches look to me like they'd make pdfjs not load if it's not
> enabled when the first window shows up. Doesn't look like you're adding
> anything to post-load the '-enabled' version of the file if that changes at
> runtime. In particular, AIUI there's UI for this in the 'Applications' (Now
> "Downloads and Links") in the prefs/options... it would be a shame if we're
> essentially breaking the 'turn pdfjs on' version of that UI...

Thanks for taking a look. I think the way it is supposed to work is that PdfjsContentUtils.jsm, which is unconditionally loaded, listens for the PDFJS:Child:refreshSettings message from the parent. The parent listens for pref changes, and sends a message to the child if they do change, which I think loads all of the same stuff we do in the -enabled bootstrap script.

Comment 13

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> (In reply to :Gijs from comment #11)
> > Oh, hm. The patches look to me like they'd make pdfjs not load if it's not
> > enabled when the first window shows up. Doesn't look like you're adding
> > anything to post-load the '-enabled' version of the file if that changes at
> > runtime. In particular, AIUI there's UI for this in the 'Applications' (Now
> > "Downloads and Links") in the prefs/options... it would be a shame if we're
> > essentially breaking the 'turn pdfjs on' version of that UI...
> 
> Thanks for taking a look. I think the way it is supposed to work is that
> PdfjsContentUtils.jsm, which is unconditionally loaded, listens for the
> PDFJS:Child:refreshSettings message from the parent. The parent listens for
> pref changes, and sends a message to the child if they do change, which I
> think loads all of the same stuff we do in the -enabled bootstrap script.

Ah, right. Thanks for explaining. Then yes, this LGTM. I'll stamp these on mozreview, but this probably wants landing together with 1354018 in some way. I'll assume you know what you're doing there. :-)

Comment 14

[:Gijs (he/him)]

Comment on attachment 8853183 [details]
Bug 1352218, part 1 - Split pdf.js boot strap file.

https://reviewboard.mozilla.org/r/125260/#review130410

Comment 15

[:Gijs (he/him)]

Comment on attachment 8853184 [details]
Bug 1352218, part 2 - Avoid PdfJs.enabled call.

https://reviewboard.mozilla.org/r/125262/#review130412

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20def3cc99f4
part 1 - Split pdf.js boot strap file. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/752d857f85cd
part 2 - Avoid PdfJs.enabled call. r=Gijs

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/20def3cc99f4
https://hg.mozilla.org/mozilla-central/rev/752d857f85cd