PDFjs should stop using sync messaging to check if it's enabled, and propagate that state asynchronously from the parent to the child instead

RESOLVED FIXED in Firefox 55

Status

()

Firefox
PDF Viewer
RESOLVED FIXED
3 months ago
19 days ago

People

(Reporter: Gijs, Assigned: dthayer)

Tracking

(Blocks: 2 bugs)

53 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
As noted in bug 1352218 (and something I ran into elsewhere still), currently PDFjs checks if it's "enabled", which it takes to mean a number of things beyond the pref, including whether it's the default handler for the PDF mimetype according to nsIExternalHelperAppService . Specifically, it sends a sync message from here:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsContentUtils.jsm#104-105

to here:

https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfjsChromeUtils.jsm#301-304



Instead, we should work out a way to do this without the sync messaging. Ideally, some of the information from the external helper app service should just be available (read-only) in the child, then we wouldn't even need to go talk to the parent.
As a general point, we should probably take a look at what pdfium is doing while we're investigating this bug to make sure it's doing the right thing as well so we don't end up having to do this all over again once we switch over to it.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
According to telemetry, the PDFJS:Parent:isDefaultHandlerApp sync IPC's median time is 113ms on Nightly.

Comment 3

2 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> As a general point, we should probably take a look at what pdfium is doing
> while we're investigating this bug to make sure it's doing the right thing
> as well so we don't end up having to do this all over again once we switch
> over to it.

We (Mortar, pdfium) now do it manually for development testing, and have a bug 1345816 to hook up with the default handler. Therefore, we probably need to do something similar here. I believe what we improve now will benefit Mortar.
Blocks: 1364015
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> PDFJS:Parent:isDefaultHandlerApp

We need to determine when this happens to judge how bad this is. Maybe Brendan knows?
Flags: needinfo?(bdahl)
It's been awhile since I've looked at this, but I believe there are only two places that ultimately trigger this code. 1) on startup[1] and 2) when some prefs or plugins are changed[2].

[1] = http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/components/nsBrowserGlue.js#898
[2] = http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/extensions/pdfjs/content/PdfJs.jsm#179
Flags: needinfo?(bdahl)
(Assignee)

Updated

25 days ago
Assignee: nobody → dothayer
Comment hidden (mozreview-request)

Comment 7

24 days ago
mozreview-review
Comment on attachment 8872829 [details]
Bug 1353029 - Pass PdfJs.enabled into child on change

https://reviewboard.mozilla.org/r/144344/#review148456
Attachment #8872829 - Flags: review?(bdahl) → review+

Comment 8

23 days ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2e141768c61
Pass PdfJs.enabled into child on change r=bdahl

Comment 9

23 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2e141768c61
Status: NEW → RESOLVED
Last Resolved: 23 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
In the future, you really need to work with the upstream pdf.js maintainers to make sure you patches land upstream as well or they're liable to get clobbered the next time we do an update.
Flags: needinfo?(dothayer)
(Assignee)

Comment 11

19 days ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> In the future, you really need to work with the upstream pdf.js maintainers
> to make sure you patches land upstream as well or they're liable to get
> clobbered the next time we do an update.

Ack, sorry - thank you for the heads up!
Flags: needinfo?(dothayer)
You need to log in before you can comment on or make changes to this bug.