As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1179262 - Remove PlayPreview registration from PDF Viewer
: Remove PlayPreview registration from PDF Viewer
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: PDF Viewer (show other bugs)
: Trunk
: Unspecified Unspecified
: -- normal (vote)
: Firefox 42
Assigned To: Yury Delendik (:yury)
:
: Brendan Dahl [:bdahl]
Mentors:
Depends on: 1182228 1193155 1252164
Blocks: jsplugins-base 1043778
  Show dependency treegraph
 
Reported: 2015-07-01 08:36 PDT by Yury Delendik (:yury)
Modified: 2016-02-29 11:48 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
fixed
fixed
verified
verified


Attachments
simple test case with object/embed/iframe usage (431 bytes, text/html)
2015-07-01 09:11 PDT, Yury Delendik (:yury)
no flags Details
WIP disables native PDF plugins (3.88 KB, patch)
2015-07-01 09:19 PDT, Yury Delendik (:yury)
no flags Details | Diff | Splinter Review
WIP Remove PlayPreview usage from PDF viewer (8.43 KB, patch)
2015-07-01 15:22 PDT, Yury Delendik (:yury)
bzbarsky: feedback+
Details | Diff | Splinter Review
Remove PlayPreview usage from PDF viewer (8.50 KB, patch)
2015-07-02 09:30 PDT, Yury Delendik (:yury)
bdahl: feedback+
Details | Diff | Splinter Review
Remove PlayPreview usage from PDF viewer (for ff40) (8.20 KB, patch)
2015-08-05 14:02 PDT, Yury Delendik (:yury)
lhenry: approval‑mozilla‑beta+
lhenry: approval‑mozilla‑release+
Details | Diff | Splinter Review
Remove PlayPreview usage from PDF viewer (for ff38esr) (8.17 KB, patch)
2015-08-05 14:05 PDT, Yury Delendik (:yury)
lhenry: approval‑mozilla‑esr38+
Details | Diff | Splinter Review
Remove PlayPreview usage from PDF viewer (for ff41) (8.18 KB, patch)
2015-08-05 18:02 PDT, Yury Delendik (:yury)
lhenry: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Remove PlayPreview usage from PDF viewer (for ff41) (8.21 KB, patch)
2015-08-05 20:43 PDT, Yury Delendik (:yury)
lhenry: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Yury Delendik (:yury) 2015-07-01 08:36:36 PDT
(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.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-01 08:55:11 PDT
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...
Comment 2 User image Yury Delendik (:yury) 2015-07-01 09:11:15 PDT
Created attachment 8628302 [details]
simple test case with object/embed/iframe usage
Comment 3 User image Yury Delendik (:yury) 2015-07-01 09:19:50 PDT
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.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-01 11:07:30 PDT
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?
Comment 5 User image Yury Delendik (:yury) 2015-07-01 13:18:00 PDT
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;
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-01 13:32:07 PDT
> 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 7 User image Yury Delendik (:yury) 2015-07-01 15:22:53 PDT
Created attachment 8628509 [details] [diff] [review]
WIP Remove PlayPreview usage from PDF viewer

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb5a4381e72a)
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-01 21:10:00 PDT
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.
Comment 9 User image Yury Delendik (:yury) 2015-07-02 09:30:45 PDT
Created attachment 8628914 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer

Fixed the comment.
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2015-07-02 09:39:00 PDT
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 11 User image Yury Delendik (:yury) 2015-07-02 09:50:11 PDT
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.
Comment 12 User image Yury Delendik (:yury) 2015-07-02 12:12:53 PDT
Good to land, using r+ from comment 10 .
Comment 14 User image Phil Ringnalda (:philor) 2015-07-03 18:58:04 PDT
https://hg.mozilla.org/mozilla-central/rev/9a9a92d6d78b
Comment 15 User image Yury Delendik (:yury) 2015-08-05 14:02:52 PDT
Created attachment 8643943 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff40)
Comment 16 User image Yury Delendik (:yury) 2015-08-05 14:05:41 PDT
Created attachment 8643945 [details] [diff] [review]
Remove PlayPreview usage from PDF viewer (for ff38esr)
Comment 17 User image Ritu Kothari (:ritu) 2015-08-05 16:35:37 PDT
Yury, do you want to request uplift to Beta, Aurora and ESR38? Thanks.
Comment 18 User image Yury Delendik (:yury) 2015-08-05 18:02:08 PDT
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
Comment 19 User image Yury Delendik (:yury) 2015-08-05 18:04:15 PDT
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
Comment 20 User image Yury Delendik (:yury) 2015-08-05 18:07:53 PDT
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.
Comment 21 User image Yury Delendik (:yury) 2015-08-05 18:17:11 PDT
(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
Comment 22 User image Liz Henry (:lizzard) (needinfo? me) 2015-08-05 18:51:16 PDT
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
Comment 23 User image Yury Delendik (:yury) 2015-08-05 20:43:59 PDT
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?
Comment 25 User image Liz Henry (:lizzard) (needinfo? me) 2015-08-05 21:18:21 PDT
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!
Comment 26 User image Wes Kocher (:KWierso) 2015-08-05 21:23:29 PDT
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
Comment 27 User image Andrei Vaida, QA [:avaida] – please ni? me 2015-08-06 04:57:27 PDT
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?
Comment 28 User image Yury Delendik (:yury) 2015-08-06 06:10:44 PDT
(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.
Comment 29 User image Andrei Vaida, QA [:avaida] – please ni? me 2015-08-06 07:27:45 PDT
(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.
Comment 30 User image Yury Delendik (:yury) 2015-08-06 07:33:06 PDT
> 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.
Comment 31 User image Andrei Vaida, QA [:avaida] – please ni? me 2015-08-06 08:32:58 PDT
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.
Comment 32 User image brian howson 2015-08-07 08:15:18 PDT
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.
Comment 33 User image Liz Henry (:lizzard) (needinfo? me) 2015-08-07 09:31:20 PDT
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.
Comment 34 User image anwarhussains 2015-08-11 00:24:24 PDT
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 User image YF (Yang) 2015-08-11 02:42:04 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.