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

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Ekanan Ketunuti]

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 <[e-mail]>
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=[e-mail]

Comment 1

[Georg Fritzsche [:gfritzsche]]

Is this with the most recent or a specific version of the Acrobat plugin?

Comment 2

[Ekanan Ketunuti]

Adobe Acrobat 11.0.3.37 (I haven't tested earlier versions.)

Comment 3

[Alice0775 White]

Almost same Bug 855666 again  again  again  again,
Automation test should be provided!

Comment 4

[rshimazu]

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

Comment 5

[Alex Keybl [:akeybl]]

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.

Comment 6

[Alice0775 White]

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

Comment 7

[Benjamin Smedberg]

What is the repro URL?

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Fix.

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

Comment 10

[Ekanan Ketunuti]

(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

Comment 11

[rshimazu]

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

Comment 12

[Ekanan Ketunuti]

(In reply to Johnny Stenback (:jst, [e-mail]) 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/[e-mail]-92ab62a604f3/

Comment 13

[rshimazu]

> I can verify that this patch build does indeed fix the problem.
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 92ab62a604f3/

I also verified that this patch worked.
But I wonder if Aurora(Firefox 24) will be updated or not.

Comment 14

[Johnny Stenback (:jst)]

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.

Comment 15

[Masatoshi Kimura [:emk]]

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

Comment 16

[Brendan Dahl [:bdahl]]

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

[Brendan Dahl [:bdahl]]

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

Comment 18

[bhavana bajaj [:bajaj]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af805ebcb52e

Comment 20

[u279076]

Mihaela, please check the latest mozilla-central build as soon as this lands.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ekanan Ketunuti]

Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20130712 Firefox/25.0

verified fixed nightly 25.0a1 buildid 20130712030203

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c06bdff31cff

Comment 24

[Ekanan Ketunuti]

Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20130717 Firefox/24.0

Verified fixed Aurora 24.0a2 buildID 20130717004002

Comment 25

[u279076]

Thanks for your help Ekanan.

Comment 26

[Benjamin Smedberg]

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

Comment 27

[Mihaela Velimiroviciu (:mihaelav)]

I added a manual testcase for this in Moztrap: https://moztrap.mozilla.org/manage/cases/?filter-id=9085

Comment 28

[u279076]

Benjamin and Mihaela, in the future please use the in-moztrap flag for manual test creation requests. Thanks.