424137 - PFS does not restart FF, therefore plug-ins do not render after install

Status: VERIFIED FIXED

Description

[Vlad Alexander]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

In FF3, plug-in installs require a restart of FF but the PFS installer does not
prompt the user to restart FF. So the plug-in gets installed but it's not
visible. If the restart cannot be automatic, can you please make the restart
option part of the PFS dialog box, instead of the information bar. This will
make the install process more seamless.

You can use this page for testing on Windows:
http://xstandard.com/test.asp


Reproducible: Always

Steps to Reproduce:
1. On Windows, go to:
http://xstandard.com/test.asp

2. You will be prompted to install a plug-in via PFS. Install it.

3. Notice that after the install the plug-in is not rendered.
Actual Results:  
Plug-in is not rendered after install.

Expected Results:  
The plug-in would render just like in FF1 & FF2 or since FF3 requires a restart, the user would be prompted to restart FF.

If you need OS X plug-in for testing via PFS, we can set it up temporarily.

Comment 1

[Doron Rosenberg (IBM)]

Firefox has never required a restart to render newly installed plugins.

In fact, the plugin won't install in Firefox 3, as we have removed install.js support.

Comment 2

[Vlad Alexander]

>Firefox has never required a restart to render newly installed plugins.
It appears that FF3 requires a restart - unless we are doing something wrong.

We've modified our Web site to server a XPI to PFS that contains install.rdf when FF3 on Windows requests it. Here is the actual XPI file:

http://xstandard.com/download/win-2.0.0.0.xpi

Even when you enter the following URL into FF and install the plug-in that way, FF3 will require a re-start.

Comment 3

[Doron Rosenberg (IBM)]

That is an extension, so yes, the browser needs a restart.  PFS, however, has never required a restart.  

I am not sure how the new plugin changes will affect this, as no one has filed a bug on adding restart to PFS.

Comment 4

[Johnny Stenback (:jst)]

PFS did use to have a field in the RDF that we gave back to the browser that told the browser whether the plugin requires Firefox to restart or not. Nominating this just to make sure we make a decision either way here.

Comment 5

[Doron Rosenberg (IBM)]

(In reply to comment #4)
> PFS did use to have a field in the RDF that we gave back to the browser that
> told the browser whether the plugin requires Firefox to restart or not.
> Nominating this just to make sure we make a decision either way here.
> 

Did we have UI for this?

Comment 6

[Johnny Stenback (:jst)]

I *thought* so, IIRC there was a page in the dialog that said that the browser needs to restart before the installed plugin will work, if that was sent from the server. But I don't remember details :(

Comment 7

[Doron Rosenberg (IBM)]

(In reply to comment #6)
> I *thought* so, IIRC there was a page in the dialog that said that the browser
> needs to restart before the installed plugin will work, if that was sent from
> the server. But I don't remember details :(
> 

Right, we have text that says "you need to restart", but not a button.  And that probably won't show up now with the new xpi changes I bet.

Comment 8

[Vlad Alexander]

If I understand the problem correctly, you'd like to re-use existing text because it is already localized? If so, may I propose this. This is the last screen of the PFS:

http://misc.xstandard.com/mozilla/pfs1.gif

You could change it to look something like this:

http://misc.xstandard.com/mozilla/pfs2.gif

The text for this screen is available from the UI that comes up when you open a URL to a XPI files in FF as shown here:

http://misc.xstandard.com/mozilla/text-for-pfs.gif

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

I think we do need to block on this: Mossop and Madhava, does using that code / those strings look right to you?

Axel: should be same context as before, but marking late-l10n to get you in the loop.

Comment 10

[Doron Rosenberg (IBM)]

I think we also have the problem where we don't know now if the restart is required or not anymore.

Comment 11

[Axel Hecht [:Pike]]

Can I get a source pointer to the localized strings that you'd like to reuse?

I'm wondering if it'd be feasible to add a new entity and to set it to the old, and to create a patch for l10n to do so. That'd work for DTDs, but not for properties.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #9)
> I think we do need to block on this: Mossop and Madhava, does using that code /
> those strings look right to you?

I don't really know the PFS, but does it not pop up the add-ons manager when
attampting to install an xpi type plugin (not one of the old types)?

I'm not entirely sure how much of a consistent experience this is going to give
between the xpi and the exe case. Additionally because the PFS dialog is modal
we will be unable to restart from it or while it is open without evil hacks
because of bug 400479

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #11)
> Can I get a source pointer to the localized strings that you'd like to reuse?
> 
> I'm wondering if it'd be feasible to add a new entity and to set it to the old,
> and to create a patch for l10n to do so. That'd work for DTDs, but not for
> properties.
> 

http://mxr.mozilla.org/seamonkey/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd#158
http://mxr.mozilla.org/seamonkey/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#23
http://mxr.mozilla.org/seamonkey/source/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties#24

Comment 14

[Axel Hecht [:Pike]]

Hrm. .properties won't work as simple, but I could possibly generate an l10n patch anyhow. On the other hand, those strings look really generic to me, a localizer had to really make that string something different and specific to bust this.

Comment 15

[Mike Connor [:mconnor]]

PFS doesn't need to be modal, does it?

Dave, can you whack this a little harder? We need some sort of solution here...

Comment 16

[Dave Townsend [:mossop]]

Created attachment 312262 [details]
step 1

That's some beautiful UI right there...

Comment 17

[Dave Townsend [:mossop]]

Created attachment 312263 [details]
step 2

Nicely inconsistent. However yes if the pfs service indicates that a plugin requires a restart then the UI shows that. So apart from some cleaning up the only thing we might want to do is to add a restart now button.

Comment 18

[Mike Connor [:mconnor]]

What needs to be done here?

Comment 19

[Dave Townsend [:mossop]]

A minor patch to come with 2 string additions to make the pfs non-modal and introduce a restart button on the last page when indicated as necessary by the plugin results. The service will need to be checked to ensure it is indicating that restarting is necessary as appropriate.

Comment 20

[Dave Townsend [:mossop]]

Created attachment 313077 [details]
OSX

Comment 21

[Dave Townsend [:mossop]]

Created attachment 313078 [details]
Windows

Comment 22

[Dave Townsend [:mossop]]

Created attachment 313079 [details]
Linux

Comment 23

[Dave Townsend [:mossop]]

Created attachment 313080 [details] [diff] [review]
patch rev 1

On the last page if there are restarts just enable the cancel and name it "Close" and change the finish button to "Restart Firefox"

Comment 24

[Dave Townsend [:mossop]]

Created attachment 313152 [details] [diff] [review]
patch rev 2

This is a little simpler. When closing the pfs on the last page, when a restart is required but the user chooses not to then this doesn't dispatch the reload event, because there is no point.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 313152 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties b/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties

>+pluginInstallation.restart=Restart %S
>+pluginInstallation.close=Close

Shouldn't these get accesskeys? Or do the default accesskeys work with these strings too?

>diff --git a/toolkit/mozapps/plugins/content/pluginInstallerWizard.js b/toolkit/mozapps/plugins/content/pluginInstallerWizard.js

> function wizardFinish(){

>+  if (gPluginInstaller.mNeedsRestart) {
>+    var nsIAppStartup = Components.interfaces.nsIAppStartup;
>+    var appStartup = Components.classes["@mozilla.org/toolkit/app-startup;1"]
>+                               .getService(nsIAppStartup);
>+    appStartup.quit(nsIAppStartup.eAttemptQuit | nsIAppStartup.eRestart);

Do we not want to notify quit-application-requested observers? We do for software update, and this seems like pretty much the same situation.

Comment 26

[Dave Townsend [:mossop]]

Created attachment 313338 [details] [diff] [review]
patch rev 3

Correctly checks in with observers over whether to quit. There is also an issue where previously when programmatically advancing the page the state of the cancel, rewind and advance buttons would get overwritten after the load handler for the wizardpage ran. This removes that, the load handlers already correctly set the state of those buttons anyway so it is unnecessary.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 313338 [details] [diff] [review]
patch rev 3

>diff --git a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties 

>+pluginInstallation.close.label=Close
>+pluginInstallation.close.accesskey=C

This seems to conflict with the existing "Continue" accesskey on Mac, though I'm not sure exactly whether that matters. Presumably the pane where we're using this string only has a finish button and we set the label/accesskey on that anyways, but I can't find the logic that shows the "Finish" button and hides the "next" button.

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #27)
> (From update of attachment 313338 [details] [diff] [review])
> >diff --git a/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties 
> 
> >+pluginInstallation.close.label=Close
> >+pluginInstallation.close.accesskey=C
> 
> This seems to conflict with the existing "Continue" accesskey on Mac, though
> I'm not sure exactly whether that matters. Presumably the pane where we're
> using this string only has a finish button and we set the label/accesskey on
> that anyways, but I can't find the logic that shows the "Finish" button and
> hides the "next" button.
> 

The wizard sets lastpage="true" on the wizard element and then in the xbl binding that provides the buttons that gets applied as hidden on the continue button. This is I believe enough to disable that as a key listener and certainly the accesskey works as I'd expect it to.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 313338 [details] [diff] [review]
patch rev 3

a1.9=beltzner

Comment 30

[Dave Townsend [:mossop]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1019; previous revision: 1.1018
done
Checking in toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.properties,v  <--  plugins.properties
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/mozapps/plugins/content/pluginInstallerWizard.js;
/cvsroot/mozilla/toolkit/mozapps/plugins/content/pluginInstallerWizard.js,v  <--  pluginInstallerWizard.js
new revision: 1.23; previous revision: 1.22
done

Comment 31

[Robert Strong [:rstrong] (use needinfo to contact me)]

Hey Dave,
You forgot a ; in pluginInstallerWizard.js
this.mNeedsRestart = false

Comment 32

[Vlad Alexander]

I tested this with our plug-in, XStandard, and was not prompted to re-start and after pressing Finished, the plug-in was not loaded on a Web page.

I was testing using trunk build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040404 Minefield/3.0pre

Here is the test page:
http://xstandard.com/test.asp

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> I tested this with our plug-in, XStandard, and was not prompted to re-start and
> after pressing Finished, the plug-in was not loaded on a Web page.

The PFS is not indicating that the plugin needs a restart to install. We need to get the PFS results updated to correctly indicate when a restart is necessary.

Comment 34

[Vlad Alexander]

In FF3, do all plug-ins now require a re-start? Our plug-in did not require a re-start in FF1 and FF2. Is there are a way for us to create a XPI that does not require a re-start?

Comment 35

[Vlad Alexander]

I would very much like to test the fix for this bug. Can someone please implement comment #33 and answer my questions in comment #34. Thanks!

Comment 36

[Dave Townsend [:mossop]]

Filed bug 427842 to see about making sure the pfs service is returning the restart information correctly.

Comment 37

[Dave Townsend [:mossop]]

(In reply to comment #34)
> In FF3, do all plug-ins now require a re-start? Our plug-in did not require a
> re-start in FF1 and FF2. Is there are a way for us to create a XPI that does
> not require a re-start?

No you cannot create an xpi that does not require a restart.

Comment 38

[Vlad Alexander]

It's been 2 weeks since this has been fixed but we cannot test it because pfs.php has not been updated. It's been reported as Bug 427842 two weeks ago. Please, can someone update pfs.php so that we can test the fix for this bug. Thanks in advance.

Comment 39

[Michael Morgan [:morgamic]]

Vlad - PFS was updated today -- sorry for the delay.

Comment 40

[Vlad Alexander]

Thank you Michael!

I've make some comments on the PFS changes in FF3 in Bug 430853

Comment 41

[Al Billings [:abillings]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050806 Minefield/3.0pre. After install, it prompts for restart. After restart, the editor loads (for this test case) and the add-ons manager lists the new plugin.