Remove PlayPreview registration from PDF Viewer

RESOLVED FIXED in Firefox 39

Status

()

Firefox
PDF Viewer
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: yury, Assigned: yury)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 verified, firefox40 fixed, firefox41 fixed, firefox42 verified, firefox-esr38 verified)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
(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)
(Assignee)

Comment 2

2 years ago
Created attachment 8628302 [details]
simple test case with object/embed/iframe usage
(Assignee)

Comment 3

2 years ago
Created attachment 8628309 [details] [diff] [review]
WIP disables native PDF plugins

(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)
(Assignee)

Updated

2 years ago
See Also: → bug 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?
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 7

2 years ago
Created attachment 8628509 [details] [diff] [review]
WIP Remove PlayPreview usage from PDF viewer

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb5a4381e72a)
Attachment #8628309 - Attachment is obsolete: true
Attachment #8628509 - Flags: feedback?(bzbarsky)
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+
(Assignee)

Comment 9

2 years ago
Created attachment 8628914 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer

Fixed the comment.
Attachment #8628509 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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.  ;)
(Assignee)

Comment 11

2 years ago
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)

Updated

2 years ago
Attachment #8628914 - Flags: feedback?(bdahl) → feedback+
(Assignee)

Comment 12

2 years ago
Good to land, using r+ from comment 10 .
Keywords: checkin-needed

Comment 13

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/9a9a92d6d78b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a9a92d6d78b
Assignee: nobody → ydelendik
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1182228
(Assignee)

Comment 15

2 years ago
Created attachment 8643943 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff40)
(Assignee)

Comment 16

2 years ago
Created attachment 8643945 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff38esr)
Yury, do you want to request uplift to Beta, Aurora and ESR38? Thanks.
Flags: needinfo?(ydelendik)
(Assignee)

Comment 18

2 years ago
Created attachment 8644069 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff41)

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

Comment 19

2 years ago
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?
(Assignee)

Comment 20

2 years ago
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?
(Assignee)

Comment 21

2 years ago
(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+
(Assignee)

Comment 23

2 years ago
Created attachment 8644113 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff41)

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?
esr38 (default): http://hg.mozilla.org/releases/mozilla-esr38/rev/5be76431120a
esr38 (GECKO3810esr_2015062417_RELBRANCH): http://hg.mozilla.org/releases/mozilla-esr38/rev/b384d22bbb60
release (default):http://hg.mozilla.org/releases/mozilla-release/rev/33b63ac4046e
release (GECKO390_2015063018_RELBRANCH): http://hg.mozilla.org/releases/mozilla-release/rev/0342f4d02d3e
beta: http://hg.mozilla.org/releases/mozilla-beta/rev/33b63ac4046e
aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/e9d391fdf0be
status-firefox39: --- → fixed
status-firefox40: --- → fixed
status-firefox41: --- → fixed
status-firefox-esr38: --- → fixed
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+
Backed out the incorrect aurora patch in https://hg.mozilla.org/releases/mozilla-aurora/rev/e25ebb70a80f
Landed the correct aurora patch in https://hg.mozilla.org/releases/mozilla-aurora/rev/6b7f6fdb97f0
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)
(Assignee)

Comment 28

2 years ago
(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.
(Assignee)

Comment 30

2 years ago
> 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.
status-firefox39: fixed → verified
status-firefox42: fixed → verified
status-firefox-esr38: fixed → verified
(Assignee)

Updated

2 years ago
See Also: → bug 965003

Comment 32

2 years ago
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

Updated

2 years ago
Depends on: 1193155

Comment 34

2 years ago
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

Comment 35

2 years ago
(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.

Updated

a year ago
Depends on: 1252164
You need to log in before you can comment on or make changes to this bug.