435788 - Plugin finder service can't install plugins using installerLocation

Status: RESOLVED FIXED

(Toolkit Graveyard :: Plugin Finder Service, defect)

Description

[Michael Morgan [:morgamic]]

The plugin finder service, or PFS, (which includes "seamless" installation of Flash) can't install any plugins on Vista.

Installing XPIs no longer works in Firefox 3 for at least Vista.  Some of that is discussed in bug 352762.

However, installerLocation and installerHash, when used, produce a bad user experience for users (see bug 419928 attachment 314482 [details]).  The user does not know what is happening, and the PFS dialog actually tells them that the install failed even though the external installer from installerLocation (after checking installerHash) has been executed.

Expected:
1. user gets prompted by PFS to install
2. user chooses to install
3. user installs, PFS tells them it was successful and they can use their plugin

Actual:
1. user gets prompted by PFS to install
2. user chooses to install
3. PFS says the installation failed
4. external installer is still running

Comment 1

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

Jennifer, maybe you can take some time to help us build a better experience here? It feels like the full-bore modal dialog is probably something we might want to investigate ditching as well.

Comment 2

[Dave Townsend [:mossop]]

Just to note bug 430853 is open about the kind of whole PFS experience, I certainly want to see changes there though that kind of large scale change is not going to happen in a 3.0 timeframe obviously.

Comment 3

[Michael Morgan [:morgamic]]

So - to make things clear - in order to get things working with installerLocation and installerHash:
1) the user experience needs to be redone for PFS - bug 430853
2) the client code needs to change to follow the revised workflow - this bug

Comment 4

[Michael Morgan [:morgamic]]

I don't think bug 430853 really blocks this bug, so they are different and should be prioritized independently.  This bug is the real issue surrounding plugin installation in the current UI.

If that UI changes, this will still be a problem.

Comment 5

[Robert Strong (they/them - no direct email)]

I have a patch in progress that fixes this based on sdwilsh's work in Bug 429988. It launches the installer via nsIProcess on a separate thread so nsIProcess I'm making it thread safe. While looking over nsIProcess I see that Kill is not available at all for Windows and only works on Mac OS X when Run has command line args. For now I'd like to make Kill return NS_ERROR_NOT_IMPLEMENTED since it is to say the least hit or miss.

Comment 6

[Michael Morgan [:morgamic]]

Great news.  Let me know if there's anything I can do to help/test.

Comment 7

[Dave Townsend [:mossop]]

Is this just for branch? I understand that there are alternate plans afoot for pfs for 3.1

Comment 8

[Robert Strong (they/them - no direct email)]

Is there a bug?

Comment 9

[Dave Townsend [:mossop]]

I do not believe so however mconnor has a wealth of knowledge I am positive

Comment 10

[Jennifer Morrow [:Boriss] (UX)]

Morgamic - do you think it's worth fixing the UI anyway once the external installer problem is fixed?

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to comment #10)
> Morgamic - do you think it's worth fixing the UI anyway once the external
> installer problem is fixed?
There really isn't a problem with the external installer... it has to do with that we try to display success or failure based on the external installer's exit code and we just launch it without waiting on the installer to finish and then checking the exit code.

Comment 12

[Michael Morgan [:morgamic]]

@boriss - I think the UI fix is on a different timeline, and you should talk to mconnor about that.  I believe that either way this mechanism should be addressed -- it will have to support any future workflow PFS might have in the future that deals with external installers.

Comment 13

[Michael Morgan [:morgamic]]

Rob - any progress on this one?

Comment 14

[Robert Strong (they/them - no direct email)]

I've been busy with a couple of App Update bugs and haven't made much progress on this as of late.

One issue is that the standard methods for installing a plugin on at least Mac OS X e.g. .dmg's (not sure about Linux) don't provide a result code after the install is complete.

Comment 15

[Michael Morgan [:morgamic]]

Since mac+linux don't support external installers could we still support xpi's on those platforms to ensure a good user experience?

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> Since mac+linux don't support external installers could we still support xpi's
> on those platforms to ensure a good user experience?

We have completely removed from the platform support for the old style install.js xpi's that plugin vendors used to use. It also wouldn't really work, almost certainly on linux and probably mac for the same reasons as vista, the user has no write access to the install dir.

We can support new-style xpi based installation for plugins on all platforms (https://developer.mozilla.org/en/Deploying_a_Plugin_as_an_Extension) but it only allows installing into the profile folder.

We might be able to launch pkg's on mac which I think is the preferred delivery format for plugins there (ignoring a surrounding dmg). I'm surprised it isn't possible for an external installer to work on mac and linux though.

Comment 17

[Dave Townsend [:mossop]]

Attachment: stage 1 rev 1

Rob I forget if you know this code or not?

Anyway this is an entry level patch that fixes all the issues I could find with installer location use in plugins. It is applicable to the 3.0.x branch as well as trunk and includes a test to verify that we can download and at least launch an installer.

The main issue is an assumption that we need a proper filename in the url. I've removed this by just using a fixed filename of setup or setup.exe depending on the platform. There shouldn't be a need to use a particular filename per installer I think.

The UI was never fixed correctly to handle installers so it displays an error when one is used, this is fixed here.

I've added calling the plugin manager to do a reload since this doesn't happen automatically in some cases.

Also fixed the refreshBrowser method to actually get the browser.

Simplest way to test is to set pfs.datasource.url to http://people.mozilla.com/~dtownsend/testcases/bug435788/pfs.rdf and then visit http://people.mozilla.com/~dtownsend/testcases/bug435788/pfs.html on a windows machine without quicktime installed.

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 368568 [details] [diff] [review]
stage 1 rev 1

>diff --git a/toolkit/mozapps/plugins/tests/TestInstaller.cpp b/toolkit/mozapps/plugins/tests/TestInstaller.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/plugins/tests/TestInstaller.cpp
>@@ -0,0 +1,13 @@
>+#if defined(XP_WIN) && !defined(__GNUC__)
>+#include <windows.h>
>+
>+int WINAPI wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int)
>+{
>+    return 0;
>+}
>+#else
>+int main(int argc, char** argv)
>+{
>+    return 0;
>+}
>+#endif
Might be good to test both success and failure.

I suspect that this will cause elevation on Vista which we can't test for since the exe name will have "setup" in it per the resultFile.createUnique above as well as this file if launched directly since it has "Install" in it... I'll check this. Adding a manifest to this file with requestedExecutionLevel level="asInvoker" would resolve this.

I'll check the rest after I have a chance to compile the patch.

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #18)
> (From update of attachment 368568 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/plugins/tests/TestInstaller.cpp b/toolkit/mozapps/plugins/tests/TestInstaller.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/mozapps/plugins/tests/TestInstaller.cpp
> >@@ -0,0 +1,13 @@
> >+#if defined(XP_WIN) && !defined(__GNUC__)
> >+#include <windows.h>
> >+
> >+int WINAPI wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int)
> >+{
> >+    return 0;
> >+}
> >+#else
> >+int main(int argc, char** argv)
> >+{
> >+    return 0;
> >+}
> >+#endif
> Might be good to test both success and failure.

At least on 1.9.0.x branch which is where I want this patch to end up we can't get the exit code for the process, all we can do is assume it completed. A version targeted at 1.9.1 is in progress which will include that.

I hadn't realised vista used filenames to determine UAC, but we could just use a different name without issue I think.

Comment 20

[Robert Strong (they/them - no direct email)]

It uses the filename if the binary doesn't have a manifest so it will require elevation for installers that haven't been updated for Vista yet.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> It uses the filename if the binary doesn't have a manifest so it will require
> elevation for installers that haven't been updated for Vista yet.

just throwing it out there, but maybe that might be a good thing? If the installer hasn't been updated and it is still trying to do a global install then it'll need UAC to work right.

Though that would mean we need to add the manifest to the test installer to stop tests failing there.

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
> (In reply to comment #20)
> > It uses the filename if the binary doesn't have a manifest so it will require
> > elevation for installers that haven't been updated for Vista yet.
> 
> just throwing it out there, but maybe that might be a good thing? If the
> installer hasn't been updated and it is still trying to do a global install
> then it'll need UAC to work right.
> 
> Though that would mean we need to add the manifest to the test installer to
> stop tests failing there.
I'm not so sure. If they want to provide elevation they can provide a manifest that asks for elevation and most installers do this when necessary. I'd suggest using the same filename so the installer itself was able to control this.

Comment 23

[Dave Townsend [:mossop]]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > It uses the filename if the binary doesn't have a manifest so it will require
> > > elevation for installers that haven't been updated for Vista yet.
> > 
> > just throwing it out there, but maybe that might be a good thing? If the
> > installer hasn't been updated and it is still trying to do a global install
> > then it'll need UAC to work right.
> > 
> > Though that would mean we need to add the manifest to the test installer to
> > stop tests failing there.
> I'm not so sure. If they want to provide elevation they can provide a manifest
> that asks for elevation and most installers do this when necessary. I'd suggest
> using the same filename so the installer itself was able to control this.

Ok, so we either use a filename that doesn't trigger elevation, or we somehow get a filename from the downloaded installer. The problem with that is it places restrictions on the url that installer vendors can give us. The biggest problem with installing the java plugin was that Sun gave us the url http://java.com/firefoxjre_exe which gives us a bogus filename. I don't necessarily mind just going back to using the filename part of the url (maybe be a bit more sensible and stick .exe on the end if it isn't there on windows or something), we just need to make sure everyone is aware of the restrictions that entails.

Comment 24

[Robert Strong (they/them - no direct email)]

Comment on attachment 368568 [details] [diff] [review]
stage 1 rev 1

Looks good... just a couple of nits besides figuring out the naming which is the only reason I am minusing.

>diff --git a/toolkit/mozapps/plugins/Makefile.in b/toolkit/mozapps/plugins/Makefile.in
>--- a/toolkit/mozapps/plugins/Makefile.in
>+++ b/toolkit/mozapps/plugins/Makefile.in
>@@ -39,10 +39,19 @@ DEPTH   = ../../..
> topsrcdir = @top_srcdir@
> srcdir    = @srcdir@
> VPATH   = @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> EXTRA_COMPONENTS = pluginGlue.js
> 
>+ifdef ENABLE_TESTS
>+# OSX will display a security warning when running the downloaded installer
>+ifneq (Darwin,$(OS_TARGET))
>+# Linux fails to run the downloaded installer
Could you improve this comment? Doesn't it run when the user has permissions?

>diff --git a/toolkit/mozapps/plugins/content/pluginInstallerService.js b/toolkit/mozapps/plugins/content/pluginInstallerService.js
>--- a/toolkit/mozapps/plugins/content/pluginInstallerService.js
>+++ b/toolkit/mozapps/plugins/content/pluginInstallerService.js
>@@ -80,27 +80,30 @@ InstallerObserver.prototype = {
>   _init: function()
>   {
>     try {
>       var ios = Components.classes["@mozilla.org/network/io-service;1"].
>         getService(Components.interfaces.nsIIOService);
>       var uri = ios.newURI(this._plugin.InstallerLocation, null, null);
>       uri.QueryInterface(Components.interfaces.nsIURL);
> 
>-      var leafName = uri.fileName;
>-      if (leafName.indexOf('.') == -1)
>-        throw "Filename needs to contain a dot for platform-native launching to work correctly.";
>+      // Use a local filename appropriate for the OS
>+      var leafName = "setup";
>+      if (Components.classes["@mozilla.org/xre/app-info;1"]
>+                    .getService(Components.interfaces.nsIXULRuntime)
>+                    .OS == "WINNT")
>+        leafName += ".exe";

>diff --git a/toolkit/mozapps/plugins/content/pluginInstallerWizard.js b/toolkit/mozapps/plugins/content/pluginInstallerWizard.js
>--- a/toolkit/mozapps/plugins/content/pluginInstallerWizard.js
>+++ b/toolkit/mozapps/plugins/content/pluginInstallerWizard.js
>...
>@@ -664,19 +665,31 @@ function wizardFinish(){
>                                  .getService(nsIAppStartup);
>       appStartup.quit(nsIAppStartup.eAttemptQuit | nsIAppStartup.eRestart);
>       return true;
>     }
>   }
> 
>   // don't refresh if no plugins were found or installed
>   if ((gPluginInstaller.mSuccessfullPluginInstallation > 0) &&
>-      (gPluginInstaller.mPluginInfoArray.length != 0) &&
>-      gPluginInstaller.mBrowser) {
>-    // notify listeners that a plugin is installed,
>-    // so that they can reset the UI and update the browser.
>-    var event = document.createEvent("Events");
>-    event.initEvent("NewPluginInstalled", true, true);
>-    gPluginInstaller.mBrowser.dispatchEvent(event);
>+    (gPluginInstaller.mPluginInfoArray.length != 0)) {
nit: indentation

>diff --git a/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf b/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf
>@@ -0,0 +1,31 @@
>...
>+
>+</RDF:RDF>
>\ No newline at end of file
Please add a newline

Comment 25

[Robert Strong (they/them - no direct email)]

btw: I did run the mochitest and it did request elevation

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #24)
> >+ifdef ENABLE_TESTS
> >+# OSX will display a security warning when running the downloaded installer
> >+ifneq (Darwin,$(OS_TARGET))
> >+# Linux fails to run the downloaded installer
> Could you improve this comment? Doesn't it run when the user has permissions?

The API nsILocalFile.launch basically isn't made to launch executables. It seems to work ok on Windows and OSX, but on Linux it really can only open documents. Executables have no default handler registered in gnome and so it fails to run.

Comment 27

[Dave Townsend [:mossop]]

Attachment: stage 1 rev 2

Fixes the nits.

What I've decided to do for the filename is go back to using the fileName part of the uri, however on windows if the filename doesn't end with ".exe" then that is appended to make it into something that will definitely execute. This should allow plugin vendors to use an installer filename of their choice but also allow them to use any URL that they wish.

Comment 28

[Dave Townsend [:mossop]]

Attachment: stage 2 rev 1

This is a patch for the next stage of improvements. Unlike the stage 1 patch this will only be targetted at trunk and 1.9.1 branches. As such it tidies up more issues that aren't really critical for fixing on the 1.9.0.x branch. This patch will apply on top of the stage 1 patch and the patches in bug 480427.

Most of the issues are around handling points where the plugin search or install fails due to problems with the downloaded datasource however I've also added some extra points where errors are logged to the error console in exceptional circumstances. This should help us diagnose any errors users are seeing.

It also adds support for detecting when the installer has finished its work and it makes the progress bar undetermined for that period.

There's also a bunch of undeclared variables that I've tidied up here as well as a lot more tests.

Comment 29

[Dave Townsend [:mossop]]

Attachment: stage 1 rev 2

Cleans up the diffs a little

Comment 30

[Dave Townsend [:mossop]]

Attachment: stage 2 rev 1

Cleans up the diffs a little

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 369096 [details] [diff] [review]
stage 2 rev 1

Looks good but I haven't test this version

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 369096 [details] [diff] [review]
stage 2 rev 1

bah... approved the wrong one. I'm reviewing this right now

Comment 33

[Robert Strong (they/them - no direct email)]

Comment on attachment 369096 [details] [diff] [review]
stage 2 rev 1

>diff --git a/toolkit/mozapps/plugins/content/pluginInstallerDatasource.js b/toolkit/mozapps/plugins/content/pluginInstallerDatasource.js
>--- a/toolkit/mozapps/plugins/content/pluginInstallerDatasource.js
>+++ b/toolkit/mozapps/plugins/content/pluginInstallerDatasource.js
>@@ -82,17 +82,16 @@ nsRDFItemUpdater.prototype = {
>   onDatasourceLoaded: function pfs_onDatasourceLoaded (aDatasource, aPluginRequestItem){
nit: if you don't mind while you are cleaning this up add a space between ){ for the functions including the nsIRDFXMLSinkObserver functions.

>     var container = Components.classes["@mozilla.org/rdf/container;1"].
>                   createInstance(Components.interfaces.nsIRDFContainer);
nit: if you don't mind this file aligns on .

>     var resultRes = this._rdfService.GetResource("urn:mozilla:plugin-results:" + aPluginRequestItem.mimetype);
>     var pluginList = aDatasource.GetTarget(resultRes, this._rdfService.GetResource(PFS_NS+"plugins"), true);
> 
>     var pluginInfo = null;
>   
>-    container = Components.classes["@mozilla.org/rdf/container;1"].createInstance(Components.interfaces.nsIRDFContainer);
>     try {
>       container.Init(aDatasource, pluginList);
> 
>       var children = container.GetElements();
>       var target;
> 
>       // get the first item
>       var child = children.getNext();
>@@ -134,26 +133,31 @@ nsRDFItemUpdater.prototype = {
>           XPIHash: getPFSValueFromRDF("XPIHash"),
>           InstallerShowsUI: getPFSValueFromRDF("InstallerShowsUI"),
>           manualInstallationURL: getPFSValueFromRDF("manualInstallationURL"),
>           requestedMimetype: getPFSValueFromRDF("requestedMimetype"),
>           licenseURL: getPFSValueFromRDF("licenseURL"),
>           needsRestart: getPFSValueFromRDF("needsRestart")
>         };
>       }
>-      catch (ex){}
>+      catch (ex){
>+        Components.utils.reportError(ex);
>+      }
>     }
>-    catch (ex){}
>+    catch (ex){
nit: add a space between ){ for the two catches in this file

Comment 34

[Robert Strong (they/them - no direct email)]

With both patches applied I get the following

$ TEST_PATH=toolkit/mozapps/plugins/tests/browser_bug435788.js make -C /c/moz/_1_mozilla-central/ff-opt/ mochitest

INFO | (automation.py) | Application ran for: 0:00:03.026000
SUCCESS: The process with PID 5188 has been terminated.
WARNING refcount logging is off, so leaks can't be detected!
mochitest-browser-chrome failed:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/plugins
/tests/browser_bug435788.js | Should have been a successful install - Got Failed
, expected Installed
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/plugins
/tests/browser_bug435788.js | Should have been a failed install - Got Failed, ex
pected Installed
make: *** [mochitest-browser-chrome] Error 1

Comment 35

[Dave Townsend [:mossop]]

(In reply to comment #34)
> With both patches applied I get the following

Did you apply the patches in bug 480427 too?

If so I'll have to see if I can try to test this on Vista or Windows 7 tomorrow.

Comment 36

[Robert Strong (they/them - no direct email)]

I cleaned up my tree and forgot to reapply them.

Comment 37

[Robert Strong (they/them - no direct email)]

The strings that come to mind that will make Vista request elevation that you might be tempted to use are install, update, and setup. If the test binary names have any of these they will need a manifest with requestedExecutionLevel level="asInvoker" or you can change the names.

Comment 38

[Robert Strong (they/them - no direct email)]

btw: the string just has to be a substring of the file name

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: error screenshot

Also getting an error about not finding the MOZCRT19.dll

Comment 40

[Dave Townsend [:mossop]]

(In reply to comment #37)
> The strings that come to mind that will make Vista request elevation that you
> might be tempted to use are install, update, and setup. If the test binary
> names have any of these they will need a manifest with requestedExecutionLevel
> level="asInvoker" or you can change the names.

Damn, so TestInstaller will be triggering it. Ok I can rename that.

(In reply to comment #39)
> Created an attachment (id=369160) [details]
> error screenshot
> 
> Also getting an error about not finding the MOZCRT19.dll

Ugh, I guess I'm not building with jemalloc enabled here. I'll have to see if I can work out a way around that.

Comment 41

[Robert Strong (they/them - no direct email)]

(In reply to comment #40)
>...
> (In reply to comment #39)
> > Created an attachment (id=369160) [details] [details]
> > error screenshot
> > 
> > Also getting an error about not finding the MOZCRT19.dll
> 
> Ugh, I guess I'm not building with jemalloc enabled here. I'll have to see if I
> can work out a way around that.
Add the following to the plugins/tests/Makefile.in
USE_STATIC_LIBS = 1

Comment 42

[Robert Strong (they/them - no direct email)]

Why use
int WINAPI wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int)

instead of using

int main(int argc, char** argv)

for both?

Comment 43

[Dave Townsend [:mossop]]

(In reply to comment #42)
> Why use
> int WINAPI wWinMain(HINSTANCE, HINSTANCE, LPWSTR, int)
> 
> instead of using
> 
> int main(int argc, char** argv)
> 
> for both?

I was at one point seeing the console window appear when running the tests. It's possible changes I made since then make it unnecessary. I guess it's also true that it doesn't matter much for the tests if the console appears and disappears a few times.

Comment 44

[Robert Strong (they/them - no direct email)]

btw: I believe the USE_STATIC_LIBS = 1 should only be necessary for Windows

Comment 45

[Robert Strong (they/them - no direct email)]

Comment on attachment 369096 [details] [diff] [review]
stage 2 rev 1

Let's go with new patches on these two for the existing issues. The remaining content/*.js changes look good and all that's left are the tests

Comment 46

[Dave Townsend [:mossop]]

Attachment: stage 1 rev 3

I've changed the name of the installer to GoodPlugin and BadPlugin so we shouldn't be hitting UAC and indeed I have tested this on a windows 7 install and it gets through the tests fine.

Comment 47

[Dave Townsend [:mossop]]

Attachment: stage 2 rev 2 [checked in]

Now that bug 480427 has landed nothing else is required for these patches to apply to trunk.

Cleaned up the JS style wherever I could see a problem. I removed the winmain usage and it does give a console window for a few moments but that isn't really a problem I think.

Comment 48

[Robert Strong (they/them - no direct email)]

Comment on attachment 369276 [details] [diff] [review]
stage 1 rev 3

>diff --git a/toolkit/mozapps/plugins/content/pluginInstallerService.js b/toolkit/mozapps/plugins/content/pluginInstallerService.js
>--- a/toolkit/mozapps/plugins/content/pluginInstallerService.js
>+++ b/toolkit/mozapps/plugins/content/pluginInstallerService.js
>@@ -80,27 +80,31 @@ InstallerObserver.prototype = {
>   _init: function()
>   {
>     try {
>       var ios = Components.classes["@mozilla.org/network/io-service;1"].
>         getService(Components.interfaces.nsIIOService);
>       var uri = ios.newURI(this._plugin.InstallerLocation, null, null);
>       uri.QueryInterface(Components.interfaces.nsIURL);
> 
>+      // Use a local filename appropriate for the OS
>       var leafName = uri.fileName;
>-      if (leafName.indexOf('.') == -1)
>-        throw "Filename needs to contain a dot for platform-native launching to work correctly.";
>+      var os = Components.classes["@mozilla.org/xre/app-info;1"]
>+                         .getService(Components.interfaces.nsIXULRuntime)
>+                         .OS;
>+      if (os == "WINNT" && leafName.substr(-4) != ".exe")
>+        leafName += ".exe";
This should also check for .msi so it doesn't append .exe to msi's

>diff --git a/toolkit/mozapps/plugins/tests/GoodPlugin.cpp b/toolkit/mozapps/plugins/tests/GoodPlugin.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/plugins/tests/GoodPlugin.cpp
>@@ -0,0 +1,4 @@
>+int main(int argc, char** argv)
>+{
>+    return 0;
>+}
>diff --git a/toolkit/mozapps/plugins/tests/Makefile.in b/toolkit/mozapps/plugins/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/plugins/tests/Makefile.in
>@@ -0,0 +1,72 @@
>...
>+
>+DEPTH = ../../../..
>+topsrcdir = @top_srcdir@
>+srcdir = @srcdir@
>+VPATH = @srcdir@
>+relativesrcdir  = toolkit/mozapps/plugins/tests
>+TESTROOT = $(DEPTH)/_tests/testing/mochitest/browser/$(relativesrcdir)
>+USE_STATIC_LIBS = 1
>+
>+include $(DEPTH)/config/autoconf.mk
nit: most Makefiles in toolkit have an empty line following VPATH followed by the include for autoconf.mk followed by a space, etc.

With those fixed r=me

I'll also verify this with a debug build as soon as it finished compiling

Comment 49

[Robert Strong (they/them - no direct email)]

(In reply to comment #48)
>...
> nit: most Makefiles in toolkit have an empty line following VPATH followed by
> the include for autoconf.mk followed by a space, etc.
s/followed by a space/followed by an empty line/

Comment 50

[Robert Strong (they/them - no direct email)]

(In reply to comment #48)
>...
> I'll also verify this with a debug build as soon as it finished compiling
All tests look good on debug and non-debug builds.

Comment 51

[Robert Strong (they/them - no direct email)]

Comment on attachment 369278 [details] [diff] [review]
stage 2 rev 2 [checked in]

I verified these tests as well with debug and non-debug on Vista

Comment 52

[Dave Townsend [:mossop]]

(In reply to comment #48)
> (From update of attachment 369276 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/plugins/content/pluginInstallerService.js b/toolkit/mozapps/plugins/content/pluginInstallerService.js
> >--- a/toolkit/mozapps/plugins/content/pluginInstallerService.js
> >+++ b/toolkit/mozapps/plugins/content/pluginInstallerService.js
> >@@ -80,27 +80,31 @@ InstallerObserver.prototype = {
> >   _init: function()
> >   {
> >     try {
> >       var ios = Components.classes["@mozilla.org/network/io-service;1"].
> >         getService(Components.interfaces.nsIIOService);
> >       var uri = ios.newURI(this._plugin.InstallerLocation, null, null);
> >       uri.QueryInterface(Components.interfaces.nsIURL);
> > 
> >+      // Use a local filename appropriate for the OS
> >       var leafName = uri.fileName;
> >-      if (leafName.indexOf('.') == -1)
> >-        throw "Filename needs to contain a dot for platform-native launching to work correctly.";
> >+      var os = Components.classes["@mozilla.org/xre/app-info;1"]
> >+                         .getService(Components.interfaces.nsIXULRuntime)
> >+                         .OS;
> >+      if (os == "WINNT" && leafName.substr(-4) != ".exe")
> >+        leafName += ".exe";
> This should also check for .msi so it doesn't append .exe to msi's

Do you think maybe it might be better to just check if there was a "." in the leafname, and only append .exe if not?

Comment 53

[Robert Strong (they/them - no direct email)]

Comment on attachment 369278 [details] [diff] [review]
stage 2 rev 2 [checked in]

Looks good!

Comment 54

[Robert Strong (they/them - no direct email)]

(In reply to comment #52)
>... 
> Do you think maybe it might be better to just check if there was a "." in the
> leafname, and only append .exe if not?
That sounds like a good idea. The case with the JRE should be an exception and not the rule though handling it would be a good thing.

Comment 55

[Dave Townsend [:mossop]]

Attachment: stage 1 rev 4 [checked in]

Carrying across review to just take care of those two changes. Will land this tomorrow.

Comment 56

[Dave Townsend [:mossop]]

Comment on attachment 369374 [details] [diff] [review]
stage 1 rev 4 [checked in]

Checked in stage 1: http://hg.mozilla.org/mozilla-central/rev/8a50fbd3288e

Comment 57

[Dave Townsend [:mossop]]

Checked in stage 2: http://hg.mozilla.org/mozilla-central/rev/839fbc4a9e3d

Comment 58

[Dave Townsend [:mossop]]

Comment on attachment 369374 [details] [diff] [review]
stage 1 rev 4 [checked in]

I would like approval to land both of these patches on the 1.9.1 branch. They make it possible to install plugins using installers through the plugin finder service. Both patches come with tests to verify their behaviour.

Comment 59

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

Comment on attachment 369278 [details] [diff] [review]
stage 2 rev 2 [checked in]

a191=beltzner

Comment 60

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

Comment on attachment 369374 [details] [diff] [review]
stage 1 rev 4 [checked in]

a191=beltzner

Comment 61

[Dave Townsend [:mossop]]

Landed stage 1 on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5a120fca0323

Comment 62

[Dave Townsend [:mossop]]

Landed stage 2 on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43ae732dbc41

Comment 63

[Shawn Wilsher :sdwilsh]

I filed bug 488593 about possible randomness in a test this bug added.

Comment 64

[Robert Strong (they/them - no direct email)]

There is also bug 487489 (looks like bug 488593 is a dupe of that) and Bug 487717

Comment 65

[Dave Townsend [:mossop]]

Comment on attachment 369374 [details] [diff] [review]
stage 1 rev 4 [checked in]

This is a small fix that would allow us to start offering plugin installers to 3.0.x users. While technically outside the scope of a security/stability update it is a small patch that only touches the PFS and includes tests to verify its behaviour so I think it would be reasonable to take it on the branch.

Comment 66

[Daniel Veditz [:dveditz]]

Comment on attachment 369374 [details] [diff] [review]
stage 1 rev 4 [checked in]

Approved for 1.9.0.10, a=dveditz for release-drivers

Comment 67

[Dave Townsend [:mossop]]

Checked in stage 1 to 1.9.0.x branch:

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.1042; previous revision: 1.1041
done
Checking in toolkit/mozapps/plugins/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/plugins/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/mozapps/plugins/content/pluginInstallerService.js;
/cvsroot/mozilla/toolkit/mozapps/plugins/content/pluginInstallerService.js,v  <--  pluginInstallerService.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/plugins/content/pluginInstallerWizard.js;
/cvsroot/mozilla/toolkit/mozapps/plugins/content/pluginInstallerWizard.js,v  <--  pluginInstallerWizard.js
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/plugins/tests/GoodPlugin.cpp,v
done
Checking in toolkit/mozapps/plugins/tests/GoodPlugin.cpp;
/cvsroot/mozilla/toolkit/mozapps/plugins/tests/GoodPlugin.cpp,v  <--  GoodPlugin.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/plugins/tests/Makefile.in,v
done
Checking in toolkit/mozapps/plugins/tests/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/plugins/tests/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/plugins/tests/browser_bug435788.js,v
done
Checking in toolkit/mozapps/plugins/tests/browser_bug435788.js;
/cvsroot/mozilla/toolkit/mozapps/plugins/tests/browser_bug435788.js,v  <--  browser_bug435788.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf,v
done
Checking in toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf;
/cvsroot/mozilla/toolkit/mozapps/plugins/tests/pfs_bug435788_1.rdf,v  <--  pfs_bug435788_1.rdf
initial revision: 1.1
done

Comment 68

[Michael Morgan [:morgamic]]

Dave, is this in a 3.0.x version yet?  Was it released?

Comment 69

[Dave Townsend [:mossop]]

(In reply to comment #68)
> Dave, is this in a 3.0.x version yet?  Was it released?

Should be in 3.0.11 whenever that is released, or nightlies of that should have it.

Comment 70

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #17)
> 
> Simplest way to test is to set pfs.datasource.url to
> http://people.mozilla.com/~dtownsend/testcases/bug435788/pfs.rdf and then visit
> http://people.mozilla.com/~dtownsend/testcases/bug435788/pfs.html on a windows
> machine without quicktime installed.

Verified for 1.9.0.11 with the above test on Vista.