Last Comment Bug 839239 - Prepare a hotfix to disable pdf.js, targeting FF18 and 19 (all platforms, release channel)
: Prepare a hotfix to disable pdf.js, targeting FF18 and 19 (all platforms, rel...
Status: RESOLVED INVALID
: qawanted
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: :Felipe Gomes (needinfo me!)
: juan becerra [:juanb]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-07 13:32 PST by Alex Keybl [:akeybl]
Modified: 2013-03-04 12:41 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Copy from 20120910 (779 bytes, patch)
2013-02-11 10:12 PST, :Felipe Gomes (needinfo me!)
dtownsend: review+
Details | Diff | Splinter Review
Disable pdf.js for Firefox 19 (4.68 KB, patch)
2013-02-11 10:12 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Disable pdf.js for Firefox 19 (4.63 KB, patch)
2013-02-11 10:18 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Hotfix .xpi (18.50 KB, application/x-xpinstall)
2013-02-11 12:07 PST, :Felipe Gomes (needinfo me!)
no flags Details
Disable pdf.js for Firefox 19 (4.64 KB, patch)
2013-02-11 12:59 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Hotfix .xpi (18.50 KB, application/x-xpinstall)
2013-02-11 13:02 PST, :Felipe Gomes (needinfo me!)
no flags Details
Disable pdf.js for Firefox 18/19 (5.95 KB, patch)
2013-02-13 21:26 PST, :Felipe Gomes (needinfo me!)
bdahl: feedback-
Details | Diff | Splinter Review
Disable pdf.js for Firefox 18/19 (6.79 KB, patch)
2013-02-15 10:13 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Disable pdf.js for Firefox 18/19 release users (7.00 KB, patch)
2013-02-15 10:32 PST, :Felipe Gomes (needinfo me!)
dtownsend: review+
Details | Diff | Splinter Review
Hotfix .xpi (19.11 KB, application/x-xpinstall)
2013-02-15 12:51 PST, :Felipe Gomes (needinfo me!)
no flags Details
Follow-up with new strategy (4.35 KB, patch)
2013-02-25 11:13 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Follow-up with new strategy (4.54 KB, patch)
2013-02-25 11:23 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review

Description Alex Keybl [:akeybl] 2013-02-07 13:32:54 PST
We need to prepare a hotfix for pdfjs.disabled=true, targeting FF19 (all platforms). This doesn't need to target FF19.0a1/a2. We may not end up using this hotfix, but we should prepare/test it in case we need to utilize it.
Comment 1 Matthew N. [:MattN] (PM me if requests are blocking you) 2013-02-08 13:57:58 PST
This can probably start as an hg copy from https://hg.mozilla.org/releases/firefox-hotfixes/file/412dde7968ff/v20120910.01 and then change the platforms, versions and pref name.
Comment 2 :Felipe Gomes (needinfo me!) 2013-02-11 10:12:02 PST
Created attachment 712499 [details] [diff] [review]
Copy from 20120910

Base code - copy from 20120910
Comment 3 :Felipe Gomes (needinfo me!) 2013-02-11 10:12:56 PST
Created attachment 712500 [details] [diff] [review]
Disable pdf.js for Firefox 19

Changes to target FF19 and disable the pdf.js pref
Comment 4 :Felipe Gomes (needinfo me!) 2013-02-11 10:18:51 PST
Created attachment 712506 [details] [diff] [review]
Disable pdf.js for Firefox 19

Attached wrong ver
Comment 5 Alex Keybl [:akeybl] 2013-02-11 10:42:42 PST
Thanks for taking this Felipe! Hoping Juan can test the add-on when he gets the chance :)
Comment 6 :Felipe Gomes (needinfo me!) 2013-02-11 12:07:48 PST
Created attachment 712555 [details]
Hotfix .xpi
Comment 7 Alex Keybl [:akeybl] 2013-02-11 12:26:46 PST
Quick question - if we were to target FF18 as well, would it be possible to prevent FF18 users updating to FF19 from ever seeing PDF.js?
Comment 8 :Felipe Gomes (needinfo me!) 2013-02-11 12:59:35 PST
Created attachment 712587 [details] [diff] [review]
Disable pdf.js for Firefox 19

After talking to Akeybl I changed this to target only 19.0.* instead of 19.* as that wasn't necessary.
After talking to Matt and Mossop earlier today I think this is all that is needed for this hotfix.

One thing worth pointing out is that if we use it, then for Fx 20 we will need to change pdf.js to use a different pref since the pref will be flipped for all users.
Comment 9 :Felipe Gomes (needinfo me!) 2013-02-11 13:02:25 PST
Created attachment 712589 [details]
Hotfix .xpi

This file should be good for testing, Juan.  This currently only targets 19.
Comment 10 juan becerra [:juanb] 2013-02-11 16:21:41 PST
I've tested this across OSs using Fx19b5 with new profiles. Before I install the hotfix the behavior defaults to opening a PDF document in the browser. After the hotfix is installed, when I try to open a PDF document it just saves it.

When I tried installing the fix on 18.0.2 and 20.0a it popped up a notification saying it is incompatible.

So I think this hotfix is working as expected.
Comment 11 Dave Townsend [:mossop] 2013-02-11 16:27:33 PST
Is "Firefox Update Hotfix" the correct name for this hotfix?
Comment 12 :Felipe Gomes (needinfo me!) 2013-02-12 08:41:10 PST
(In reply to Dave Townsend (:Mossop) from comment #11)
> Is "Firefox Update Hotfix" the correct name for this hotfix?

Oh I did not notice that each hotfix had a different name, I thought this was a standard name. I should change the name. Some of the hotfixes were simply named "Firefox Hotfix", so maybe that?
Comment 13 Alex Keybl [:akeybl] 2013-02-12 10:30:53 PST
(In reply to :Felipe Gomes from comment #12)
> (In reply to Dave Townsend (:Mossop) from comment #11)
> > Is "Firefox Update Hotfix" the correct name for this hotfix?
> 
> Oh I did not notice that each hotfix had a different name, I thought this
> was a standard name. I should change the name. Some of the hotfixes were
> simply named "Firefox Hotfix", so maybe that?

Generically naming it "Firefox Hotfix" is good. Can we move the minimum version to include FF18.0.* as well? Felipe let me know that targeting FF18 would prevent users from ever seeing PDF.js, upon update to FF19.
Comment 14 Dave Townsend [:mossop] 2013-02-12 10:38:18 PST
Comment on attachment 712587 [details] [diff] [review]
Disable pdf.js for Firefox 19

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

As the versions are changing I won't review this further till then

::: v20130211.01/bootstrap.js
@@ +52,5 @@
>      return false;
>    }
>  
>    try {
> +    if (Services.prefs.getBoolPref(PDFJS_DISABLED_PREF) === true) {

Is this check necessary? The version range above will make us abort if its not a version to be fixed. I don't think there are cases where we'll be in the right version and the pref won't exist.
Comment 15 Brendan Dahl [:bdahl] 2013-02-12 10:39:22 PST
If pdf.js was enabled and we do the hotfix to disable do we also want to restore the user's previous behavior for how pdfs are handled?

If we don't restore these settings and we run the hotfix to disable when a user clicks on a pdf, the pdf will be automatically downloaded to the downloads folder. 

To do this we would just need to do something similar to http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJs.jsm#140 and the settings before pdf.js are enabled are stored in:
pdfjs.previousHandler.alwaysAskBeforeHandling
pdfjs.previousHandler.preferredAction
Comment 16 :Felipe Gomes (needinfo me!) 2013-02-13 21:26:42 PST
Created attachment 713794 [details] [diff] [review]
Disable pdf.js for Firefox 18/19

Talked to Brendan and included the part to restore the previous mime handler information. It looks like this is all that we need to cover to restore the state before pdf.js.

I also included a pref to store the previous value of pdfjs.disabled in case the user had manually changed it and we want to restore that choice. Not sure if that is wanted but I included it here, it can be easily removed. (I'll let Brendan give it a thought)

We also discussed strategies about how to re-enable pdf.js in a following Firefox version. There are various alternatives, one being using the Firefox UI migration code, another using the PdfJs version migration code, or also changing the pref name. We didn't decide which one would be the best since it's not something to worry by now, just wanted to ensure it was possible to do.

This version also cover FF18 + 19.

(In reply to Dave Townsend (:Mossop) from comment #14)
> Comment on attachment 712587 [details] [diff] [review]
> Disable pdf.js for Firefox 19
> 
> Review of attachment 712587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As the versions are changing I won't review this further till then
> 
> ::: v20130211.01/bootstrap.js
> @@ +52,5 @@
> >      return false;
> >    }
> >  
> >    try {
> > +    if (Services.prefs.getBoolPref(PDFJS_DISABLED_PREF) === true) {
> 
> Is this check necessary? The version range above will make us abort if its
> not a version to be fixed. I don't think there are cases where we'll be in
> the right version and the pref won't exist.

Yeah I don't think it was necessary in that version, except perhaps saving a small amount of work on setting the pref again.

But now I modified it to be more accurate (checks if this hotfix was already run by any chance) since it also restores the previous mime handler values and we don't want that part running twice.

Flagging Brendan to take a feedback look here before requesting review.
Comment 17 Brendan Dahl [:bdahl] 2013-02-14 09:33:54 PST
Comment on attachment 713794 [details] [diff] [review]
Disable pdf.js for Firefox 18/19

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

When I was going over the code I unfortunately remembered we should also re-enable pdf plugins.  This is done by removing the pdf mimetype from the disabled plugins preference (the opposite of http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJs.jsm#146)

::: v20130211.01/bootstrap.js
@@ +47,5 @@
> +
> +    if (Services.prefs.getPrefType(PREVIOUS_ASK_PREF) ==  Services.prefs.PREF_BOOL) {
> +      mimeHandler.alwaysAskBeforeHandling = Services.prefs.getBoolPref(PREVIOUS_ASK_PREF);
> +    }
> +

We also need to store the above settings to make it active see http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJs.jsm#143
Comment 18 :Felipe Gomes (needinfo me!) 2013-02-15 10:13:03 PST
Created attachment 714471 [details] [diff] [review]
Disable pdf.js for Firefox 18/19

All feedback incorporated
Comment 19 :Felipe Gomes (needinfo me!) 2013-02-15 10:23:31 PST
Comment on attachment 714471 [details] [diff] [review]
Disable pdf.js for Firefox 18/19

(just a sec, i'll add something else)
Comment 20 :Felipe Gomes (needinfo me!) 2013-02-15 10:32:40 PST
Created attachment 714480 [details] [diff] [review]
Disable pdf.js for Firefox 18/19 release users

This version will only hotfix users on the release channel, as discussed with akeybl
Comment 21 :Felipe Gomes (needinfo me!) 2013-02-15 12:51:26 PST
Created attachment 714544 [details]
Hotfix .xpi

Juan, this is the final version that targets FF18 and 19. I've tested it locally on OSX 10.8 but it's a good idea for you to also give it a go. To properly test it you need to have the pref "app.update.channel" set to "release", otherwise the hotfix won't apply.

During testing it would be good if in addition to the proper behavior check as you did in comment 10, if you could verify the following pref changes:

pdfjs.disabled changing from false to true
pdfjs.beforeHotfix.disabled gets created and set to false
"application/pdf" is removed from the list in plugin.disable_full_page_plugin_for_types (the pref will probably be blank afterwards)

And also make sure the "Firefox Hotfix" add-on isn't listed in the add-ons manager afterwards.
Comment 22 :Felipe Gomes (needinfo me!) 2013-02-18 12:24:03 PST
Since I already did some testing on the add-on I pushed it to the repo, and if Juan finds any problems I can push follow-ups.

https://hg.mozilla.org/releases/firefox-hotfixes/rev/cfe1636058d0
Comment 23 juan becerra [:juanb] 2013-02-19 09:32:29 PST
(In reply to :Felipe Gomes from comment #22)
> Since I already did some testing on the add-on I pushed it to the repo, and
> if Juan finds any problems I can push follow-ups.
> 
> https://hg.mozilla.org/releases/firefox-hotfixes/rev/cfe1636058d0

I've tested across OS on 18 and 19, and I went through the checks in comment #21. It seems to work.

I noticed there's a difference in 18 vs 19 in that the pref called "plugin.diabled_full..." isn't listed in 18. Is that of concern?

Also, the hotfix installation doesn't leave a trace of its last version. I don't remember if it should. If I remember correctly it was left out as a preference called extensions.hotfix.lastVersion which may be due to the fact that this wasn't installed through the staging server.

If that's ok we are good to go.
Comment 24 juan becerra [:juanb] 2013-02-19 09:50:13 PST
Actually, I just checked one more thing:

If you have Fx18.0.1 installed (fresh) and apply the hotfix, when you update to Fx19 and try to open a PDF it will open it inline. PDFjs doesn't get disabled, and if you install the hotfix again on 19, nothing changes and it will keep opening it inline.

It also happens that if you had enabled pdf.js in 18 and install the hotfix, the hotfix will work, but going through the update to 19 will undo the hotfix magic. Again, reinstalling the hotfix on 19 won't work.
Comment 25 juan becerra [:juanb] 2013-02-19 10:10:38 PST
One more thing, perhaps we can limit this to Fx19, because if you don't apply the hotfix to Fx18 and then upgrade to Fx19, and apply it in Fx19, the feature is disabled as expected.
Comment 26 :Felipe Gomes (needinfo me!) 2013-02-19 13:33:31 PST
So what happens is that on Firefox 18, the default for pdfjs.disabled is true.  When the hotfix sets it to true (even if it was false), it now counts as having the default value (i.e., not a user value). And prefs with user values are maintained between versions, while prefs with default values are migrated to the new default value (which happens to be false in 19).

So I think the best way to handle it that won't require a hack or changes to the pdf.js code is to target this hotfix only to 19. Alex, what do you think?
Comment 27 Clemens Eisserer 2013-02-20 11:09:32 PST
Please push this update as soon as possible.

I tested pdf.js for about 1 hour and already had to file quite a few reports: 842957, 842962, 842969, 843166, 843184, 843203, 843206, 843211

Actually I don't get how this stuff made it to get enabled in its current state in a stable release. Of the pdfs I tested, more were broken than rendered correctly.
Comment 28 Florian Bender 2013-02-20 14:05:31 PST
I agree, there are two big problems which severely affect UX: Not being able to save a PDF except via the confusing (for some, at least) download icon, and the broken rendering of some localized PDFs (the other bugs are not that severe). PDF.js should be disabled for another train and those two issues fixed and tested.
Comment 29 Alex Keybl [:akeybl] 2013-02-21 09:09:13 PST
(In reply to :Felipe Gomes from comment #26)
> So I think the best way to handle it that won't require a hack or changes to
> the pdf.js code is to target this hotfix only to 19. Alex, what do you think?

Could we have the add-on check on each browser startup whether the Firefox version is FF19 and pdfjs.disabled==false, then set it to true once and uninstall itself?
Comment 30 :Felipe Gomes (needinfo me!) 2013-02-21 11:17:07 PST
So I had an alternate idea that I think would work well. The important point is: when the hotfix runs for a user in FF18, it should work to prevent that user from getting pdf.js in 19.

To do this, I think we can:
- not bother with the pdfjs.disabled pref
- reset the pdf file association / re-enable plugins for pdf
- set pdfjs.migrationVersion to 1. This will prevent the first-run of FF19 into becoming a handler for pdfs again, independent of the value of the pdfjs.disabled pref

And this has the following consequences:
- After the hotfix is run, if a user wants to re-enable it, they *do not* have to go to about:config. Instead they have to go to Preferences -> Applications, and set PDF files to "Preview in Firefox".

- For the Firefox version when we want to re-enable it (i.e., possibly FF20 which is in beta), we need to bump the version here: http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfJs.jsm#113, and also change the check on line 119 below to be "< 3" instead of "< 2". So it's a pretty simple change that will make it be the default pdf handler again

Brendan, what do you think of this approach? Is there any problem in leaving the pref unchanged? (I'm guessing not as long as the file association is not "handle internally")

Alex, are you fine with users having to go to the Preferences dialog instead of about:config if they want to manually enable it?
Comment 31 :Felipe Gomes (needinfo me!) 2013-02-25 11:13:57 PST
Created attachment 717986 [details] [diff] [review]
Follow-up with new strategy

So I went ahead and implemented what's proposed on comment 30 as I think this is a good way to handle how we would want to disable it for 18/19 users.
Comment 32 :Felipe Gomes (needinfo me!) 2013-02-25 11:23:02 PST
Created attachment 717990 [details] [diff] [review]
Follow-up with new strategy

(Properly qrefresh'ed, sry for the bug churn)
Comment 33 Alex Keybl [:akeybl] 2013-02-26 10:41:16 PST
Fortunately for the release, Support and Release Management have signed off on leaving this feature enabled after combing through feedback over the last week. Unfortunately for everybody working on this bug, it's now Resolved/Invalid.

I want to thank Felipe, Dave, Juan, and everybody else who worked on this bug. If we did need to quickly disable the feature without spinning a 19.0.1, we would have been ready. We had no idea how the feature was going to go over with release channel users, so you all enabled us to push out with confidence.

Thanks again!!

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