If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
PDF Viewer
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
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.
(Assignee)

Comment 1

6 months ago
This is my pull request in the PDF.js repo:
  https://github.com/mozilla/pdf.js/pull/8218
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

6 months ago
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...
Flags: needinfo?(continuation)
(Assignee)

Comment 5

6 months ago
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.
Flags: needinfo?(continuation)

Updated

6 months ago
See Also: → bug 1353029
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

6 months ago
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.
(Assignee)

Updated

6 months ago
Whiteboard: [qf]
(Assignee)

Updated

6 months ago
Blocks: 1354081
(Assignee)

Comment 9

6 months ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 10

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

Comment 11

6 months ago
(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...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(continuation)
(Assignee)

Comment 12

6 months ago
(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.
Flags: needinfo?(continuation)

Comment 13

6 months ago
(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. :-)

Updated

6 months ago
Attachment #8853183 - Flags: review?(ydelendik) → review?(gijskruitbosch+bugs)

Comment 14

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

https://reviewboard.mozilla.org/r/125260/#review130410
Attachment #8853183 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

6 months ago
Attachment #8853184 - Flags: review?(ydelendik) → review?(gijskruitbosch+bugs)

Comment 15

6 months ago
mozreview-review
Comment on attachment 8853184 [details]
Bug 1352218, part 2 - Avoid PdfJs.enabled call.

https://reviewboard.mozilla.org/r/125262/#review130412
Attachment #8853184 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 16

6 months ago
Pushed by amccreight@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/20def3cc99f4
https://hg.mozilla.org/mozilla-central/rev/752d857f85cd
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.