Closed Bug 1353029 Opened 5 years ago Closed 4 years ago
PDFjs should stop using sync messaging to check if it's enabled, and propagate that state asynchronously from the parent to the child instead
59 bytes, text/x-review-board-request
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.
According to telemetry, the PDFJS:Parent:isDefaultHandlerApp sync IPC's median time is 113ms on Nightly.
(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.
(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?
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 and 2) when some prefs or plugins are changed.  = http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/components/nsBrowserGlue.js#898  = http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/extensions/pdfjs/content/PdfJs.jsm#179
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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/d2e141768c61 Pass PdfJs.enabled into child on change r=bdahl
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.
(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!
You need to log in before you can comment on or make changes to this bug.