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

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, enhancement)

Description

[:Gijs (he/him)]

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.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 2

[(no longer active)]

According to telemetry, the PDFJS:Parent:isDefaultHandlerApp sync IPC's median time is 113ms on Nightly.

Comment 3

[Evelyn Hung]

(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.

Comment 4

[Andrew Overholt [:overholt]]

(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?

Comment 5

[Brendan Dahl [: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

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1353029 - Pass PdfJs.enabled into child on change

isDefaultHandler in PdfJs.jsm appears to only be called on
startup and when the settings for pdfs (either the pref or the
setting in about:preferences) are changed. During startup, it's
only the parent process which makes this call, which it uses
to conditionally load a script in the content process. On change,
the parent process controls notifying the content process, so
it can simply pass along the enabled boolean. This change simply
shifts to pass this boolean along to the child, and adds some
guards to assert that we're only checking the actual values in
the parent process.

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

Comment 7

[Brendan Dahl [:bdahl]]

Comment on attachment 8872829 [details]
Bug 1353029 - Pass PdfJs.enabled into child on change

https://reviewboard.mozilla.org/r/144344/#review148456

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2e141768c61
Pass PdfJs.enabled into child on change r=bdahl

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Alex Thayer [:alexical] (she/her)]

(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!