Closed Bug 1179262 Opened 9 years ago Closed 9 years ago

Remove PlayPreview registration from PDF Viewer

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox39 --- verified
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- verified
firefox-esr38 --- verified

People

(Reporter: yury, Assigned: yury)

References

Details

Attachments

(5 files, 3 obsolete files)

(As a temporary solution?) that will make transition to OOP jsplugins easier. PlayPreview will be removed as a part of the jsplugins and moving PDF viewer to out-of-process architecture might take some time. Also it will be useful to have capturing of the original content stream implemented (bug 964435) -- the PlayPreview lacks of this functionality as well.

We want to change logic of EMBED tags to disable any instantiation of the native PDF plugins if the internal PDF viewer is enabled. That will allow its stream converter take a priority. This also allows to use the original data stream, and not re-create a new one.
So I tried to hack on this last night but discovered that at least on my Mac we're treating the Adobe PDF plug-in as always disabled, which is not very helpful for debugging.

If you can tell me how to detect the "internal PDF viewer is enabled" state, I can try to put up some patches that implement this proposal for testing...
Flags: needinfo?(ydelendik)
Attached patch WIP disables native PDF plugins (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #1)
> So I tried to hack on this last night but discovered that at least on my Mac
> we're treating the Adobe PDF plug-in as always disabled, which is not very
> helpful for debugging.
> 
> If you can tell me how to detect the "internal PDF viewer is enabled" state,
> I can try to put up some patches that implement this proposal for testing...

The PDF viewer detects if it is enabled via multiple configuration flags. See http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJs.jsm#275 . I think the best way to detect is PDF viewer is enabled is to check stream converter. I created a WIP patch (see attachment) -- I will check if the test page works on Windows with internval PDF viewer on/off soon.
Flags: needinfo?(ydelendik)
See Also: → 1044767
We need to check this for loads in a docshell too, I expect.  Basically in nsWebNavigationInfo::IsTypeSupported I expect.

Would it make sense to register a conversion (the same one, basically) to a more specific type (whatever type the conversion actually outputs) so we can check that and don't get confused by other converters that might get installed for application/pdf?
The WIP patch works in Windows and properly displays content is the Adobe NPAPI plugin (when it's selected instead of internal PDF Viewer)

(In reply to Boris Zbarsky [:bz] from comment #4)
> We need to check this for loads in a docshell too, I expect.  Basically in
> nsWebNavigationInfo::IsTypeSupported I expect.

Shall that just return OK for the 'application/pdf' when PDFJS enabled? If yes, is there any preference on location (file wise) for IsPDFJSEnabled() to avoid code duplication in nsObjectLoadingContent.cpp and nsWebNavigationInfo.cpp?

> Would it make sense to register a conversion (the same one, basically) to a
> more specific type (whatever type the conversion actually outputs) so we can
> check that and don't get confused by other converters that might get
> installed for application/pdf?

Tried the following without any luck:

diff --git a/browser/extensions/pdfjs/content/PdfStreamConverter.jsm b/browser/extensions/pdfjs/content/PdfStreamConverter.jsm
--- a/browser/extensions/pdfjs/content/PdfStreamConverter.jsm
+++ b/browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ -806,17 +806,17 @@ FindEventManager.prototype.unbind = func
   // properties required for XPCOM registration:
   classID: Components.ID('{d0c5195d-e798-49d4-b1d3-9324328b2291}'),
   classDescription: 'pdf.js Component',
-  contractID: '@mozilla.org/streamconv;1?from=application/pdf&to=*/*',
+  contractID: '@mozilla.org/streamconv;1?from=application/pdf&to=text/html',
 
diff --git a/dom/base/nsObjectLoadingContent.cpp b/dom/base/nsObjectLoadingContent.cpp
--- a/dom/base/nsObjectLoadingContent.cpp
+++ b/dom/base/nsObjectLoadingContent.cpp
@@ -527,17 +527,17 @@ IsSupportedImage(const nsCString& aMimeT
    bool canConvert = false;
    if (convServ) {
-     rv = convServ->CanConvert("application/pdf", "*/*", &canConvert);
+     rv = convServ->CanConvert("application/pdf", "text/html", &canConvert);
    }
    return NS_SUCCEEDED(rv) && canConvert;
> Shall that just return OK for the 'application/pdf' when PDFJS enabled?

Yes, just returning NS_OK after setting *aIsTypeSupported = nsIWebNavigationInfo::UNSUPPORTED would be good there.

> If yes, is there any preference on location (file wise) for IsPDFJSEnabled()

nsContentUtils?

> Tried the following without any luck:

You want to keep the registration under the old contract with */*.  You just _also_ want to add a registration for conversion "from=application/pdf&to=text/html", using the same classID and whatnot.
Comment on attachment 8628509 [details] [diff] [review]
WIP Remove PlayPreview usage from PDF viewer

>+  // Special case for PDF documents when internal PDF viewer is enabled.

This comment should explicitly say that we want to claim the type is unsupported so that the PDF viewer's stream converter will get used.

The rest looks pretty reasonable.
Attachment #8628509 - Flags: feedback?(bzbarsky) → feedback+
Fixed the comment.
Attachment #8628509 - Attachment is obsolete: true
For what it's worth, you can consider my feedback+ as r+ for the gecko bits.  Not sure what the right review situation is for the actual bits that stop using playpreview, but as long as pdfjs still works I'm happy to claim r=me on those too.  ;)
Comment on attachment 8628914 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer

(In reply to Boris Zbarsky [:bz] from comment #10)
> For what it's worth, you can consider my feedback+ as r+ for the gecko bits.
> Not sure what the right review situation is for the actual bits that stop
> using playpreview, but as long as pdfjs still works I'm happy to claim r=me
> on those too.  ;)

Cool, I checked pdf viewer (on/off) on Windows and it still works. I wanted to ping bdahl to check if he has different thoughts/ideas, but the patch is ready to land as is now.
Attachment #8628914 - Flags: feedback?(bdahl)
Attachment #8628914 - Flags: feedback?(bdahl) → feedback+
Good to land, using r+ from comment 10 .
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a9a92d6d78b
Assignee: nobody → ydelendik
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1182228
Yury, do you want to request uplift to Beta, Aurora and ESR38? Thanks.
Flags: needinfo?(ydelendik)
Approval Request Comment
[Feature/regressing bug #]: bug 558184
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: landed in m-c
[Risks and why]: affects only pdf content loaded via embed tag, removes dependency of unstable API
[String/UUID change made/needed]: contains new UUID
Flags: needinfo?(ydelendik)
Attachment #8644069 - Flags: approval-mozilla-aurora?
Comment on attachment 8643943 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff40)

Approval Request Comment
[Feature/regressing bug #]: bug 558184
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: landed in m-c
[Risks and why]: affects only pdf content loaded via embed tag, removes dependency on unstable API
[String/UUID change made/needed]: contains new UUID
Attachment #8643943 - Flags: approval-mozilla-beta?
Comment on attachment 8643945 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff38esr)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: removes dependency on unstable API 
User impact if declined: none
Fix Landed on Version: Firefox 42
Risk to taking this patch (and alternatives if risky): affects only pdf content loaded via embed tag 
String or UUID changes made by this patch: contains new UUID 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8643945 - Flags: approval-mozilla-esr38?
(In reply to Ritu Kothari (:ritu) from comment #17)
> Yury, do you want to request uplift to Beta, Aurora and ESR38? Thanks.

Yes. But I might need help with additional verifying the attached test on Windows, Mac OSX, and Linux platforms with/without native plugins installed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc449b4021a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6345bdecc362
https://treeherder.mozilla.org/#/jobs?repo=try&revision=122c3ef884ed
Attachment #8643945 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Attachment #8644069 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8643943 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8643943 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff40)

Meant to approve this for m-r since 40 is on that branch already
Attachment #8643943 - Flags: approval-mozilla-release+
Wrong file was used for aurora/ff41.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c451153b0d80

Shall I repeat approval request for it?
Attachment #8644069 - Attachment is obsolete: true
Attachment #8644113 - Flags: approval-mozilla-aurora?
Comment on attachment 8644113 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff41)

We had the wrong patch for 41. approving this one. Sorry!
Attachment #8644113 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Yury, does this fix require manual testing? If that is the case, could you please elaborate a bit on how Manual QA can help out here?
Flags: needinfo?(ydelendik)
(In reply to Andrei Vaida, QA [:avaida] from comment #27)
> Yury, does this fix require manual testing? If that is the case, could you
> please elaborate a bit on how Manual QA can help out here?

You can observe behavior of the embedded PDF via different tags at https://bugzilla.mozilla.org/attachment.cgi?id=8628302 (using EMBED tag in the middle). So steps to test:
  1. Open https://bugzilla.mozilla.org/attachment.cgi?id=8628302
  2. Notice PDF content for all three cases (object/embed/iframe)

Notice if native plugin is selected (instead of internal PDF viewer) on the test configuration, the native plugin shall be shown.
Flags: needinfo?(ydelendik)
(In reply to Yury Delendik (:yury) from comment #28)
> You can observe behavior of the embedded PDF via different tags at
> https://bugzilla.mozilla.org/attachment.cgi?id=8628302 (using EMBED tag in
> the middle). So steps to test:
>   1. Open https://bugzilla.mozilla.org/attachment.cgi?id=8628302
>   2. Notice PDF content for all three cases (object/embed/iframe)
> 
> Notice if native plugin is selected (instead of internal PDF viewer) on the
> test configuration, the native plugin shall be shown.

My understanding here is that this patch fixes how embedded PDFs are displayed in Firefox while the internal PDF Viewer (pdf.js) is disabled. For example, on an affected build - say 39.0 (20150630154324) - using Adobe Reader to view the testcase from Comment 2 while "pdfjs.disabled" is "true", will instead show nothing for the file embedded via <object> and a neterror for the one embedded via <iframe>, whereas the PDF Viewer (when active) displays all three of them properly inside the browser.

Thing is, the same behavior can be seen on a build where this is fixed - e.g. Nightly 42.0a1 (2015-08-05), using Adobe Reader. I've used Windows 7 (x64) to test this.
> My understanding here is that this patch fixes how embedded PDFs are displayed in Firefox while the internal PDF Viewer (pdf.js) is disabled. 

It shall work in both case with pdf.js enabled and disabled.

Please switch handler in options->applications (due to a bug the firefox might need to be restarted)

Notice that "pdfjs.disabled" shall not be used, at least without switching the handler.
This is verified fixed on Firefox 38.1.1esr (20150806001528), Firefox 39.0.3 (20150806001005) and Nightly 42.0a1 (2015-08-05), using Windows 8.1 x86, Windows 7 x64, Mac OS X 10.9.5 and Ubuntu 14.04 (x86).

PDFs embedded via <embed> are displayed properly with and without pdf.js enabled.
See Also: → 965003
Is there official confirmation whether the 31.8.0 ESR branch will or won't get a patch for this issue?  According to the roadmap, the 31.x line is supported until August 11 when 38.2.0 ESR is released.
Brian: Good question. I hope we can make that clear at least in this bug. The exploit we were addressing did not work in 31. So, no, we didn't patch 31.8.0 ESR.
Blocks: 1043778
Depends on: 1193155
Hi ,

Currently i am on a security Testing of a software product. I would like to know if this issue effects my software product. How do I test it ?

Regards,
Anwar
(In reply to Liz Henry (:lizzard) from comment #33)
> Brian: Good question. I hope we can make that clear at least in this bug.
> The exploit we were addressing did not work in 31. So, no, we didn't patch
> 31.8.0 ESR.

Hi Liz, can you mark the status-firefox-esr31 of Tracking Flags to clearly explain the comment, thanks you.
Depends on: 1252164
You need to log in before you can comment on or make changes to this bug.