Firefox Nighty 24.0a1 opens blank page instead of calling the third-party PDF reader plugin

VERIFIED FIXED in Firefox 24

Status

()

defect
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: ananuti, Assigned: jst)

Tracking

({regression})

24 Branch
Firefox 25
x86
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ verified, firefox25+ verified)

Details

(Whiteboard: [pdfjs-c-integration])

Attachments

(1 attachment)

STR:
1. start Nightly with virgin profile
2. enable CTP plugin, flip plugins.click_to_play to true and restart
3. set acrobat plugin to "Ask to Activate" 
4. set Portable Document Format (PDF) (in Options::Application) to Use Adobe Acrobat (in Nightly) and restart


m-c:
last good: mozilla-central changeset 6edffc15e2dc
first bad: mozilla-central changeset 198e38876f7e Merge m-i and m-c 


m-i:
last good: mozilla-inbound changeset cd7ca5763eac
first bad: mozilla-inbound changeset ec45e0246b83

user:        Johnny Stenback <jst@mozilla.com>
date:        Wed Mar 27 22:19:41 2013 -0700
files:       browser/base/content/pageinfo/permissions.js dom/plugins/base/nsIPluginTag.idl dom/plugins/base/nsPluginTags.cpp dom/plugins/test/mochitest/test_refresh_navigator_plugins.html toolkit/mozapps/extensions/PluginProvider.jsm
description:
Bug 855613. Stop exposing nsIDOMMimeType in nsIPluginTag. r=joshmoz@gmail.com
Priority: -- → P3
Is this with the most recent or a specific version of the Acrobat plugin?
Adobe Acrobat 11.0.3.37 (I haven't tested earlier versions.)
Keywords: regression
Version: Trunk → 24 Branch
Almost same Bug 855666 again  again  again  again,
Automation test should be provided!
>Almost same Bug 855666 again  again  again  again,
>Automation test should be provided!

Yes, there should be an Automation test.

But taking it in account that this bug appears again and again, we should rethink whether the fix for Bug 738967, which I think is the root cause of this regression, is really necessary at all.
In my opinion, to support for embedded PDF with pdf.js is not essential at all.
We're still not shipping a visible pref for this, correct Georg? Is step 2 necessary to repro? If yes to both, this won't track.
(In reply to Alex Keybl [:akeybl] from comment #5)
> We're still not shipping a visible pref for this, correct Georg? Is step 2
> necessary to repro? If yes to both, this won't track.

Not need to change preferences(step1-3) in about:config.
It is necessary "step4" only.
What is the repro URL?
To answer akeybl, there is no visible pref in 24 (although I'm thinking about asking for uplift if I can get bug 888510 in time). In 25 we flipped the default so that click-to-activate is visible in the addon manager.
Posted patch Fix.Splinter Review
This fixes 'a' bug in PDF.js, it might be this one, or it might be something that manifests itself in different ways. Found by code inspection here. This PDF.js change came into m-c after my changes landed that changed what nsIPluginTag.getMimeTypes() returns, and this needs to be fixed upstream in PDF.js as well, however that process is managed. Brendan, please roll this up where needed, whether it fixes this bug or not (and please help test too, I don't have a testcase for this bug readily available).
Attachment #771057 - Flags: review?(bdahl)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> What is the repro URL?

http://people.mozilla.com/~lco/CtP/130415%20CtP%20design%20spec.pdf
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> What is the repro URL?
Actually all pdfs are reproducible.

How to reproduce:
Just set Portable Document Format (PDF) (in Options::Application) to Use Adobe Acrobat (in Nightly) and access to any PDF in the web.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #9)
> Created attachment 771057 [details] [diff] [review]
> Untested but possible fix.

I can verify that this patch build does indeed fix the problem.
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ananuti@gmail.com-92ab62a604f3/
> I can verify that this patch build does indeed fix the problem.
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ananuti@gmail.com-
> 92ab62a604f3/

I also verified that this patch worked.
But I wonder if Aurora(Firefox 24) will be updated or not.
Comment on attachment 771057 [details] [diff] [review]
Fix.

Thanks for testing the fix! I'll leave it up to bdahl etc to decide whether this needs to be uplifted. But I will say that this is about as safe and as low impact as bug fixes get.
Attachment #771057 - Attachment description: Untested but possible fix. → Fix.
Comment on attachment 771057 [details] [diff] [review]
Fix.

>-        return mimeType.type === PDF_CONTENT_TYPE;
>+        return mimeType === PDF_CONTENT_TYPE;

For upstream, it should be |(mimeType.type || mimeType) === PDF_CONTENT_TYPE|. We need to support older versions. (It would be sufficient for Aurora internal viewer.)
Component: Plug-ins → PDF Viewer
Priority: P3 → --
Product: Core → Firefox
(In reply to Masatoshi Kimura [:emk] from comment #15)
> Comment on attachment 771057 [details] [diff] [review]
> Fix.
> 
> >-        return mimeType.type === PDF_CONTENT_TYPE;
> >+        return mimeType === PDF_CONTENT_TYPE;
> 
> For upstream, it should be |(mimeType.type || mimeType) ===
> PDF_CONTENT_TYPE|. We need to support older versions. (It would be
> sufficient for Aurora internal viewer.)

Pdfjs.jsm isn't used for the extension so the current fix should be sufficient.
Comment on attachment 771057 [details] [diff] [review]
Fix.

Review of attachment 771057 [details] [diff] [review]:
-----------------------------------------------------------------

Upstream PR:
https://github.com/mozilla/pdf.js/pull/3450

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 879161
User impact if declined: for pdf plugins if click to play is enabled the user will not be able to use third party plugins
Testing completed (on m-c, etc.): tested locally, we could land on MC first then aurora if wanted
Risk to taking this patch (and alternatives if risky): very low, simple check
String or IDL/UUID changes made by this patch: none
Attachment #771057 - Flags: review?(bdahl)
Attachment #771057 - Flags: review+
Attachment #771057 - Flags: approval-mozilla-aurora?
Priority: -- → P1
Whiteboard: [pdfjs-c-integration]
(In reply to Brendan Dahl from comment #17)
> Comment on attachment 771057 [details] [diff] [review]
> Fix.
> 
> Review of attachment 771057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Upstream PR:
> https://github.com/mozilla/pdf.js/pull/3450
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 879161
> User impact if declined: for pdf plugins if click to play is enabled the
> user will not be able to use third party plugins
> Testing completed (on m-c, etc.): tested locally, we could land on MC first
> then aurora if wanted
> Risk to taking this patch (and alternatives if risky): very low, simple check
> String or IDL/UUID changes made by this patch: none

Yep can we please go ahead with m-c landing first.Also adding qawanted,verifyme once this lands.
Keywords: qawanted, verifyme
Mihaela, please check the latest mozilla-central build as soon as this lands.
QA Contact: mihaela.velimiroviciu
https://hg.mozilla.org/mozilla-central/rev/af805ebcb52e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20130712 Firefox/25.0

verified fixed nightly 25.0a1 buildid 20130712030203
Attachment #771057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20130717 Firefox/24.0

Verified fixed Aurora 24.0a2 buildID 20130717004002
Status: RESOLVED → VERIFIED
Thanks for your help Ekanan.
Keywords: verifyme
Given that this has recurred twice, it seems that we need a manual test for it (it's basically impossible to test in the automated suite without installing a PDF plugin on the build slaves, which is inadvisable).
Flags: needinfo?(mihaela.velimiroviciu)
I added a manual testcase for this in Moztrap: https://moztrap.mozilla.org/manage/cases/?filter-id=9085
Flags: needinfo?(mihaela.velimiroviciu)
Keywords: qawanted
Benjamin and Mihaela, in the future please use the in-moztrap flag for manual test creation requests. Thanks.
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.