628651 - [OSX] show notification bar offering to restart in 32-bit mode when content tries to use a 32-bit plugin using carbon based NPAPI

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

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

When running in 64-bit mode on OSX, Firefox requires that any 32-bit plugins use the Cocoa based NPAPI, and will fail to load those that use the Carbon based NPAPI, throwing a NPERR_MODULE_LOAD_FAILED_ERROR. This was introduced by the patch for bug 598223, see:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#1880

We need to:

 - expose that error to chrome
 - catch that error when we see it
 - react by showing the user a notification bar across the top of the page like we do if there's a missing plugin or a blocklisted plugin

The notification message should be yellow, and read:

This page requires a plugin that can only run in 32-bit mode  (Restart in 32-bit mode) (x)

When the user clicks on the restart in 32-bit mode, we should do a restart (ie: preserve session, like restarting for an add-on install) and pass the command line argument to start in 32-bit mode.

Comment 1

[(not currently active) Ted Mielczarek]

I think we can do that, but the API we use to launch in 32-bit mode wouldn't persist that setting, so the next time the user restarted they'd wind up back in 64-bit mode. Would you want this to permanently switch the user to 32-bit mode? (Until they manually changed the setting?)

Comment 2

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

One-time restart only, please, Vasily.

Comment 3

[David Dahl :ddahl]

(In reply to comment #0)

> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#1880
> 
> We need to:
> 
>  - expose that error to chrome
>  - catch that error when we see it
>  - react by showing the user a notification bar across the top of the page 

Out of curiosity, what would be the best way to handle this? notifyObservers of e.g.: 'npapi-prohibited-plugin-event', passing the the tab as aSubject? 

I have been looking at the code in PluginModuleChild and am wondering how to get the current tab or window. 

Do we also want to report the error to the console service? 

Then, on the front-end side we can just listen for the topic and open the notification bar.

I do not know how you would tell firefox to restart in 32bit mode, but it seems like bug 542122, where set an env var to restart in safe mode:

let environment = Components.classes["@mozilla.org/process/environment;1"].
                  getService(Components.interfaces.nsIEnvironment);
environment.set("MOZ_SAFE_MODE_RESTART", "1");
Application.restart();

Of course, by the time you read an env var you are already executing in one mode or another.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> (In reply to comment #0)
> 
> > http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#1880
> > 
> > We need to:
> > 
> >  - expose that error to chrome
> >  - catch that error when we see it
> >  - react by showing the user a notification bar across the top of the page 
> 
> Out of curiosity, what would be the best way to handle this? notifyObservers of
> e.g.: 'npapi-prohibited-plugin-event', passing the the tab as aSubject? 

The other cases fire a DOM event from the object tag which browser.js listens to

> I do not know how you would tell firefox to restart in 32bit mode, but it seems
> like bug 542122, where set an env var to restart in safe mode:
> 
> let environment = Components.classes["@mozilla.org/process/environment;1"].
>                   getService(Components.interfaces.nsIEnvironment);
> environment.set("MOZ_SAFE_MODE_RESTART", "1");
> Application.restart();
> 
> Of course, by the time you read an env var you are already executing in one
> mode or another.

You probably want to add a new flag to nsIAppStartup.Quit and then in the restart launching code in nsAppRunner.cpp where it cleans out the command line for a relaunch it would add the appropriate command line flag

Comment 5

[(not currently active) Ted Mielczarek]

LaunchChildMac is what we use to restart the app on Mac:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/MacLaunchHelper.mm#60

It currently contains code to restart the app with the current CPU architecture (to avoid launching the 64-bit code on 10.5 where we want to be 32-bit), but it'd be easy enough to modify it to pick an architecture based on something that was passed down.

Comment 6

[David Dahl :ddahl]

(In reply to comment #4)

> The other cases fire a DOM event from the object tag which browser.js listens
> to

So the specific code for this kind of crash does not fire an event? All I see there is:

 *rv = NPERR_MODULE_LOAD_FAILED_ERROR;

Do you know where the other cases are defined in the plugin code?

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> (In reply to comment #4)
> 
> > The other cases fire a DOM event from the object tag which browser.js listens
> > to
> 
> So the specific code for this kind of crash does not fire an event? All I see
> there is:
> 
>  *rv = NPERR_MODULE_LOAD_FAILED_ERROR;

Yes the current code does nothing useful yet I think

I'm guessing you'll be wanting to make use of the existing error event dispatch code here but josh may be able to guide you better: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1675

Comment 8

[David Dahl :ddahl]

Josh:

What is the best way to get a DOM error event to fire in this case?

Comment 9

[David Dahl :ddahl]

cjones:

In this related bug, you mentioned something about logging why the plugin was not loaded: https://bugzilla.mozilla.org/show_bug.cgi?id=598223#c3

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, I understand what you're looking for now.  The only way I know how to do is would be to cargo-cult off similar code we have for crashing.  If a plugin instances chooses Carbon and we reject, which happens at the PluginModuleChild.cpp link you pasted above, the error code gets to

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleParent.cpp#817

where we eat the specific code, then destroy the plugin instance.  We would need to not eat the code, to know why creating the instance failed.

When plugins crash, we propagate information out of dom/plugins/Plugin*Parent through

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleParent.cpp#333

which bounces up to

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsPluginHost.cpp#3919

where we dispatch an observer-service notification.  You might want to do something similar for Carbon mismatches.

After the observer-service notification, I don't recall exactly what happens, it's the frontend from there on.

Comment 11

[David Dahl :ddahl]

Filed bug 629401 for platform work.

Comment 12

[David Dahl :ddahl]

note on restart - ted dug up a nice snippet from breakpad:

http://code.google.com/p/google-breakpad/source/browse/trunk/src/client/mac/tests/spawn_child_process.h#101

Comment 13

[David Dahl :ddahl]

Attachment: 0.1 Front-end WIP patch

needs notificationBox element, tests and tweaks to LaunchChildMac

Comment 14

[David Dahl :ddahl]

Attachment: v 0.1.1 WIP saving work

Comment 15

[Johnathan Nightingale [:johnath]]

Strings risk means we want to lock this one down RSN - how's it feeling ddahl?

Comment 16

[David Dahl :ddahl]

will post my latest WIP for feedback later today

Comment 17

[David Dahl :ddahl]

Attachment: v 0.1 Front end code, with test

restart bits not quite done - i am not sure how to get env vars from the c++ side in LaunchChildMac.mm

Comment 18

[David Dahl :ddahl]

Attachment: v 0.1 Front end code, with test

Comment 19

[David Dahl :ddahl]

Comment on attachment 508913 [details] [diff] [review]
v 0.1 Front end code, with test

hg has borked this patch

Comment 20

[David Dahl :ddahl]

Attachment: v 0.1 Front end code, with test

BTW: since this patch uses the test plugin, and I could not mock it. My 32 bit mac does not make the event trigger, so I would love some feedback before I push to try server/ pass off the patch to someone to test.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 508926 [details] [diff] [review]
v 0.1 Front end code, with test

>+#ifdef XP_MACOSX
>+function arch32bitModeRestart()
>+{
>+  // prompt the user to confirm restarting 64-bit Mac OSX Firefox in 32-bit mode
>+  let promptTitle = gNavigatorBundle.getString("32bitModeRestartPromptTitle");
>+  let promptMessage =
>+    gNavigatorBundle.getString("32bitModeRestartPromptMessage");
>+  let rv = Services.prompt.confirm(window, promptTitle, promptMessage);
>+  if (rv) {
>+    let environment = Components.classes["@mozilla.org/process/environment;1"].
>+      getService(Components.interfaces.nsIEnvironment);
>+    environment.set("MOZ_MAC_32BIT_MODE_RESTART", "1");
>+    Application.restart();
>+  }
>+}

I've tinkered a little with the way you need to restart. Assuming my patch gets review you'll want to use the code from this function but also pass nsIAppStartup.eRestart32Bit to the quit method. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#201

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 508926 [details] [diff] [review]
v 0.1 Front end code, with test

>         // For non-object plugin tags, register a click handler to install the
>         // plugin. Object tags can, and often do, deal with that themselves,
>         // so don't stomp on the page developers toes.
>         if (!(plugin instanceof HTMLObjectElement))
>           self.addLinkClickCallback(plugin, "installSinglePlugin");
>         /* FALLTHRU */
>       case "PluginBlocklisted":
>       case "PluginOutdated":
>+#ifdef XP_MACOSX
>+      case "npapi-carbon-event-model-failure":
>+#endif
>         let hideBarPrefName = event.type == "PluginOutdated" ?
>                                 "plugins.hide_infobar_for_outdated_plugin" :
>                                 "plugins.hide_infobar_for_missing_plugin";
>         if (gPrefService.getBoolPref(hideBarPrefName))
>           return;

I'd move this pref checking code to the block for only PluginBlocklisted and PluginOutdated or the pref for plugin missing will hide the event mode info bar. Or I guess add a matching pref for that case.

>       // get the urls of missing plugins
>       var missingPluginsArray = gBrowser.selectedBrowser.missingPlugins;
>       if (missingPluginsArray) {
>         openDialog("chrome://mozapps/content/plugins/pluginInstallerWizard.xul",
>                    "PFSWindow", "chrome,centerscreen,resizable=yes",
>                    {plugins: missingPluginsArray, browser: gBrowser.selectedBrowser});
>       }
>     }
>+#ifdef XP_MACOSX
>+    function mismatchedPluginsRestartBrowser() {
>+      // restart Firefox in 32 bit mode MacOS X
>+      arch32bitModeRestart();
>+    }
>+#endif

Why have a function that just calls another function?

> 
>     let notifications = {
>       PluginBlocklisted : {
>                             barID   : "blocked-plugins",
>                             iconURL : "chrome://mozapps/skin/plugins/notifyPluginBlocked.png",
>                             message : gNavigatorBundle.getString("blockedpluginsMessage.title"),
>                             buttons : [{
>                                          label     : gNavigatorBundle.getString("blockedpluginsMessage.infoButton.label"),
>@@ -6652,17 +6666,30 @@ var gPluginHandler = {
>                             iconURL : "chrome://mozapps/skin/plugins/notifyPluginGeneric.png",
>                             message : gNavigatorBundle.getString("missingpluginsMessage.title"),
>                             buttons : [{
>                                          label     : gNavigatorBundle.getString("missingpluginsMessage.button.label"),
>                                          accessKey : gNavigatorBundle.getString("missingpluginsMessage.button.accesskey"),
>                                          popup     : null,
>                                          callback  : showPluginsMissing
>                                       }],
>-                          }
>+                            },
>+#ifdef XP_MACOSX
>+    "npapi-carbon-event-model-failure" : {
>+                              barID    : "mismatched-plugins",
>+                              iconURL  : "chrome://mozapps/skin/plugins/notifyPluginGeneric.png",
>+                              message  : gNavigatorBundle.getString("mismatchededpluginsMessage.title"),
>+                              buttons: [{
>+                                         label     : gNavigatorBundle.getString("mismatchedpluginsMessage.restartButton.label"),
>+                                         accessKey : gNavigatorBundle.getString("mismatchedpluginsMessage.restartButton.accesskey"),
>+                                         popup     : null,
>+                                         callback  : mismatchedPluginsRestartBrowser
>+                                      }],
>+      }
>+#endif

There is a lot of conflicting terms throughout that I think should be made consistent. We talk about carbon event model failure, mismatched, archMismatched, all for the same thing really. Maybe just carbonFailure would be the most appropriate? Gavin may have a take here though and it's likely he'll be doing the final review pass here.

Comment 23

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

Removing from b11 radar, but ETA to be on trunk for Friday.

Comment 24

[David Dahl :ddahl]

Attachment: v 0.2 feedback addressed

I still need to find out if the test is going to work at all

Comment 25

[David Dahl :ddahl]

Attachment: v 0.2 feedback addressed

forgot to re-enable properties

Comment 26

[David Dahl :ddahl]

Josh:

will we be able to automate the testing of these patches via the test plugin?

Comment 27

[Axel Hecht [:Pike]]

Restart stuff etc makes me think of mozmill tests, but I'm not sure if that's built up to detect restarts coming from the browser itself.

Comment 28

[David Dahl :ddahl]

(In reply to comment #27)
> Restart stuff etc makes me think of mozmill tests, but I'm not sure if that's
> built up to detect restarts coming from the browser itself.

Yeah. I'll request mozmill/litmus but I am mainly concerned about the plugin's carbon error event via the test plugin

Comment 29

[Josh Aas]

The test plugin uses Carbon in 32-bit binaries and Cocoa in 64-bit binaries. We don't currently have a test situation in which a 64-bit browser will attempt to load a 32-bit plugin. You could build a basic 32-bit plugin that negotiates Carbon and does nothing else, give it a unique MIME-type, and check the binary into the tree (don't have it build in-tree) such that it is always installed on Mac OS X, and then have a test that tries to load it by MIME type. I'm not sure it is worth the effort for what should be a one-major-release temporary measure, especially if we can coordinate solid manual testing.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Since we do not ship a default or test plugin with our builds anymore, it is kinda hard for us to test it with Mozmill. Or can we fake it? Otherwise we could test if the restart in 32bit mode was successful.

Comment 31

[David Dahl :ddahl]

Josh:

Which commonly available plugin do you recommend for manual testing?

Comment 32

[(not currently active) Ted Mielczarek]

The test plugin ships in the test package, FWIW.

Not having test coverage on a fairly-involved change like this late in the cycle scares the hell out of me, to be honest. ddahl: Josh's suggestion is probably doable. I would just take the existing test plugin in our tree, gut it to make it only use carbon + change the default mime type it accepts, then build it like normal in a 32-bit mac build, and check the binary in, and make it get copied around like we do the test plugin for tests:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#210

Comment 33

[Dave Townsend [:mossop]]

Comment on attachment 509133 [details] [diff] [review]
v 0.2 feedback addressed

>+#ifdef XP_MACOSX
>+function carbonFailurePluginsRestartBrowser()
>+{
>+  // prompt the user to confirm restarting 64-bit Mac OSX Firefox in 32-bit mode
>+  let promptTitle = gNavigatorBundle.getString("carbonFailureRestartPromptTitle");
>+  let promptMessage =
>+    gNavigatorBundle.getString("carbonFailureRestartPromptMessage");
>+  let rv = Services.prompt.confirm(window, promptTitle, promptMessage);
>+  if (rv) {
>+    Application.restart(Ci.nsIAppStartup.eRestart32Bit);
>+  }

Application.restart doesn't accept any flags, you need to use the code I pointed you to instead of Application.restart.

Comment 34

[David Dahl :ddahl]

Attachment: v 0.3 feedback addressed

Comment 35

[Dave Townsend [:mossop]]

Comment on attachment 509172 [details] [diff] [review]
v 0.3 feedback addressed

>+#ifdef XP_MACOSX
>+function carbonFailurePluginsRestartBrowser()
>+{
>+  // prompt the user to confirm restarting 64-bit Mac OSX Firefox in 32-bit mode
>+  let promptTitle = gNavigatorBundle.getString("carbonFailureRestartPromptTitle");
>+  let promptMessage =
>+    gNavigatorBundle.getString("carbonFailureRestartPromptMessage");
>+  let rv = Services.prompt.confirm(window, promptTitle, promptMessage);
>+  if (rv) {
>+    let as = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>+    as.quit(Ci.nsIAppStartup.eRestart32Bit);
>+  }
>+}
>+#endif

You need all of the code from the function I linked you to or the user won't get warned about cancelling downloads in progress etc.

Comment 36

[David Dahl :ddahl]

Attachment: v 0.4 added notifyObservers of quit-application-requested

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 509266 [details] [diff] [review]
v 0.4 added notifyObservers of quit-application-requested

Few comments but not much more I can review here, would like a browser peer to sign off on it and we need to figure out the tests.

>+#ifdef XP_MACOSX
>+function carbonFailurePluginsRestartBrowser()
>+{
>+  // prompt the user to confirm restarting 64-bit Mac OSX Firefox in 32-bit mode
>+  let promptTitle = gNavigatorBundle.getString("carbonFailureRestartPromptTitle");
>+  let promptMessage =
>+    gNavigatorBundle.getString("carbonFailureRestartPromptMessage");
>+  let rv = Services.prompt.confirm(window, promptTitle, promptMessage);
>+  if (rv) {

I generally prefer just returning early rather than indenting the rest of the function.

>+    // Notify all windows that an application quit has been requested.
>+    let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"].
>+                       createInstance(Ci.nsISupportsPRBool);
>+    Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null);
>+ 
>+    // Something aborted the quit process.
>+    if (cancelQuit.data)
>+      return;      
>+    let as = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
>+    as.quit(Ci.nsIAppStartup.eRestart32Bit);

You need to pass the flags eRestart and eAttemptQuit as well.

>diff --git a/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js b/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js

This test doesn't work so probably not much point in reviewing it. How is getting a carbon plugin going?

Comment 38

[David Dahl :ddahl]

(In reply to comment #37)
> >+    // Notify all windows that an application quit has been requested.
> >+    let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"].
> >+                       createInstance(Ci.nsISupportsPRBool);
> >+    Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null);
> >+ 
> >+    // Something aborted the quit process.
> >+    if (cancelQuit.data)
> >+      return;      
> >+    let as = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);
> >+    as.quit(Ci.nsIAppStartup.eRestart32Bit);
> 
> You need to pass the flags eRestart and eAttemptQuit as well.
> 
 pass them in as 2nd and 3rd args?

> >diff --git a/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js b/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js
> 
> This test doesn't work so probably not much point in reviewing it. How is
> getting a carbon plugin going?

I have duplicated (the directory) and removed all of the code from the test plugin's mac-specific file that would make the plugin use the cocoa event model and changed the mime-type to application/x-test-carbon. How do you build it? i tried 'make testplugin' (fail), but I am thinking you may have to build it from one directory up? not sure.

Comment 39

[Benjamin Smedberg]

Let me make the carbon-only testplugin, just a sec.

Comment 40

[Benjamin Smedberg]

Hrm. I can make a carbon-only testplugin (which has to be i386-only), but I suspect that we'll die in "unify" unless we can add a special exception for this file which shouldn't/doesn't need to be unified.

Comment 41

[(not currently active) Ted Mielczarek]

Yeah, if you have the same file with the same arch on both sides, unify will choke. However, if you have a file that only exists on one side, unify will copy it to the output (with a warning), so you could just build it on the i386 side and not on the x86-64 side.

Comment 42

[David Dahl :ddahl]

(In reply to comment #41)
> so you could just build it on the i386 side and not on the x86-64 side.

I have a 32 bit core duo mac here - will that help at all?

Comment 43

[David Dahl :ddahl]

(In reply to comment #38)

> > You need to pass the flags eRestart and eAttemptQuit as well.
> > 
>  pass them in as 2nd and 3rd args?

oh. using bitwise OR like in nsBlocklistSvc. gotcha.

Comment 44

[David Dahl :ddahl]

Attachment: v 0.5.2 Comments adressed

still working out the carbon only test plugin

Comment 45

[Benjamin Smedberg]

Attachment: Build a carbon testplugin

Comment 46

[Benjamin Smedberg]

Attachment: Build a carbon testplugin, rev. 2

That's what I get for copy-pasting from other makefiles: __LP64__ is not a makefile variable.

Comment 47

[David Dahl :ddahl]

Attachment: v 0.6 updated test for carbon plugin

The test actually shows the missing plugins notification banner on my 32 bit machine. maybe it'll work on a 64 bit mac.

Comment 48

[(not currently active) Ted Mielczarek]

That looks like it'll do the trick. ddahl: You'll want to make your test only run if a) You're in a universal build (nsIMacUtils.isUniversalBinary: http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMacUtils.idl) and b) The current arch is x86_64.

Comment 49

[David Dahl :ddahl]

(In reply to comment #48)
> That looks like it'll do the trick. ddahl: You'll want to make your test only
> run if a) You're in a universal build (nsIMacUtils.isUniversalBinary:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIMacUtils.idl) and
> b) The current arch is x86_64.

ted: thx. will make that change.

Comment 50

[David Dahl :ddahl]

Attachment: v 0.7 fixed test to only run on 64bit universal builds

Comment 51

[David Dahl :ddahl]

Attachment: v 0.7 fixed test to only run on 64bit universal builds

Comment 52

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

Comment on attachment 509444 [details] [diff] [review]
v 0.6 updated test for carbon plugin

This page requires a plugin that can only run in 32-bit mode  (Restart in
32-bit mode) (x)

>+carbonfailurepluginsMessage.title=Cannot load 32-bit plugins on 64-bit MacOS X

nit: camelcaps that entity name like the others? Also, I think I preferred the string I suggested in comment 0:

carbonFailurePluginsMessage.title=This page uses a plugin that can only run in 32-bit mode

>+carbonfailurepluginsMessage.restartButton.label=Restart in 32-bit modeâ¦
>+carbonfailurepluginsMessage.restartButton.accesskey=R

nit: camelcaps

>+# Mac OS X 32 bit mode restart
>+carbonFailureRestartPromptTitle=Restart in 32-bit modeâ¦
>+carbonFailureRestartPromptMessage=Are you sure you want to restart in 32-bit mode?

Why are we doing a second dialog, here? There's little to no dataloss potential of restarting in 32-bit mode. I suppose it's fine, but it feels like a "whatever" button to me. I don't think we do a confirm-restart window for things like application update, either. Let's leave it out.

uir=beltzner with those changes

Comment 53

[Dave Townsend [:mossop]]

Comment on attachment 509460 [details] [diff] [review]
v 0.7 fixed test to only run on 64bit universal builds

From josh on IRC we should be testing that we support Carbon plugins in i386 mode before displaying the notification bar. We can just test if its a universal binary and if not just follow the same path as a missing plugin.

Comment 54

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

(In reply to comment #52)
> I don't think we do a confirm-restart window for
> things like application update, either. Let's leave it out.

It's controlled by the warnOnRestart pref for the moment, which defaults to false. Probably not worth hooking this up to the pref, especially since we may be getting rid of it.

Comment 55

[David Dahl :ddahl]

Attachment: v 0.8 fixed strings, removed prompt and universal build checking

Comment 56

[Dave Townsend [:mossop]]

(In reply to comment #55)
> Created attachment 509484 [details] [diff] [review]
> v 0.8 fixed strings, removed prompt and universal build checking

This doesn't seem to do the universal build checking from comment 53

Comment 57

[David Dahl :ddahl]

(In reply to comment #56)
> (In reply to comment #55)
> > Created attachment 509484 [details] [diff] [review]
> > v 0.8 fixed strings, removed prompt and universal build checking
> 
> This doesn't seem to do the universal build checking from comment 53

i interpreted that to mean:

https://bugzilla.mozilla.org/attachment.cgi?id=509484&action=diff#a/browser/base/content/browser.js_sec6

Comment 58

[Dave Townsend [:mossop]]

Oops missed that, ignore me

Comment 59

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

Comment on attachment 509484 [details] [diff] [review]
v 0.8 fixed strings, removed prompt and universal build checking

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+#ifdef XP_MACOSX
>+      case "npapi-carbon-event-model-failure":
>+        hideBarPrefName = "plugins.hide_infobar_for_carbon_failure_plugin";
>+        if (gPrefService.getBoolPref(hideBarPrefName)) {
>+          return;
>+        }

I'm not sure we really need to add a pref for this... I guess it doesn't hurt much.

>+#ifdef XP_MACOSX
>+    let carbonFailureNotification = notificationBox.getNotificationWithValue("carbon-failure-plugins");
>+#endif

You should put this in the block that it's used, since it isn't used elsewhere.

>+#ifdef XP_MACOSX
>+function carbonFailurePluginsRestartBrowser()

Don't think you should put this in the global scope - just add it to gPluginHandler (easy since it doesn't care what its |this| is).

>+  if (cancelQuit.data)
>+    return;      
>+  let as = Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup);

nit: trim that trailing whitespace after "return", and add a newline before |let as|?

>diff --git a/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js b/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js

>+function test() {

>+    if (!abi.match(/64/)) {
>+      dump("CANCELING CARBON PLUGIN TEST, WRONG PLATFORM." + "\n");

use todo(false, "..."); (same for all early-exit paths).

>+  waitForExplicitFinish();
>+  addTab('data:text/html,<h1>Plugin carbon mismatch test</h1><embed id="test" style="width: 100px; height: 100px" type="application/x-test-carbon">');
>+  browser.addEventListener("load", onLoad, true);
>+  finish();

You don't actually want to finish() here, right? You need to wait for the load...

Comment 60

[David Dahl :ddahl]

Attachment: v 0.8 review comments addressed

also: the test was leaking on 32 bit mac os x because i was not removing an eventListener

Comment 61

[David Dahl :ddahl]

Attachment: v 0.8.1 fixed universalBinary check

Comment 62

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

Comment on attachment 509601 [details] [diff] [review]
v 0.8.1 fixed universalBinary check

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+      case "npapi-carbon-event-model-failure":
>+        hideBarPrefName = "plugins.hide_infobar_for_carbon_failure_plugin";
>+        if (gPrefService.getBoolPref(hideBarPrefName)) {
>+          return;
>+        }

nit: no braces. applies elsewhere in the patch for single-line if blocks.

>     else if (eventType == "PluginNotFound") {
>       if (missingNotification)
>         return;
> 
>       // Cancel any notification about blocklisting plugins
>       if (blockedNotification)
>         blockedNotification.close();
>     }
>+#ifdef XP_MACOSX
>+    else if (eventType == "npapi-carbon-event-model-failure") {

I think you should do this first, so that if you end up switching it to a PluginNotFound you still go through the code above (that returns if there's a missingNotification already, or closes blocked plugin notifications).

>+      if (carbonFailureNotification) {
>+         carbonFailureNotification.close();
>+      }

nit: no braces

>+  carbonFailurePluginsRestartBrowser: function carbonFailurePluginsRestartBrowser()

Should actually probably go next to showPluginsMissing.

>diff --git a/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js b/browser/base/content/test/browser_maconly_carbon_mismatch_plugin.js

>+function test() {
>+  try {
>+    let abi = Services.appinfo.XPCOMABI;
>+    if (!abi.match(/64/)) {
>+      todo(false, "canceling test, wrong platform");
>+      browser.removeEventListener("load", onLoad, false);

Don't need to remove the listener if you haven't added it (applies to the other early return cases)...

Comment 63

[David Dahl :ddahl]

Attachment: v 0.8.1 fixed test indention, incorrect event removal

Comment 64

[Dave Townsend [:mossop]]

Comment on attachment 509612 [details] [diff] [review]
v 0.8.1 fixed test indention, incorrect event removal

>+#ifdef XP_MACOSX
>+    "npapi-carbon-event-model-failure" : {
>+                              barID    : "carbon-failure-plugins",
>+                              iconURL  : "chrome://mozapps/skin/plugins/notifyPluginGeneric.png",
>+                              message  : gNavigatorBundle.getString("carbonFailurePluginsMessage.title"),
>+                              buttons: [{
>+                                         label     : gNavigatorBundle.getString("carbonFailurePluginsMessage.restartButton.label"),
>+                                         accessKey : gNavigatorBundle.getString("carbonFailurePluginsMessage.restartButton.accesskey"),
>+                                         popup     : null,
>+                                         callback  : carbonFailurePluginsRestartBrowser

carbonFailurePluginsRestartBrowser is not defined as it is now on the plugin handler object.

Comment 65

[David Dahl :ddahl]

Attachment: v 0.9 comments addressed

Comment 66

[Dave Townsend [:mossop]]

(In reply to comment #46)
> Created attachment 509441 [details] [diff] [review]
> Build a carbon testplugin, rev. 2
> 
> That's what I get for copy-pasting from other makefiles: __LP64__ is not a
> makefile variable.

The plugin isn't getting packaged properly, presumably we have to do something clever with unify here however it also doesn't seem to trigger the event, not sure if that is a plugin host problem or a plugin problem. When we attempt to load it we log:

This plugin supports only the carbon event model.
###!!! [Child][RPCChannel] Error: Processing error: message was deserialized, but the handler returned false (indicating failure)

Comment 67

[(not currently active) Ted Mielczarek]

Yeah, you need to jam it in the test package here:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/Makefile.in#178

sorry, forgot that. That should work fine, since when we run mochitest we stick the bin/plugins bit in the testing profile.

Comment 68

[Dave Townsend [:mossop]]

Comment on attachment 509621 [details] [diff] [review]
v 0.9 comments addressed

>+function onLoad() {
>+  browser.removeEventListener("load", arguments.callee, false);

You remove it with false but added it with true so the handler won't get remove at all.

>+  let notificationBox = gBrowser.getNotificationBox();
>+  let notificationBar = notificationBox.getNotificationWithValue("carbon-failure-plugins");
>+  ok(notificationBar, "Carbon Error plugin notification bar was found");
>+  finish();
>+}

In my tests this doesn't wait long enough for the notification bar, I guess because we have to wait for async OOP message passing. You may either need to have code that waits for a notification to appear.

Comment 69

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

Comment on attachment 509621 [details] [diff] [review]
v 0.9 comments addressed

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+#ifdef XP_MACOSX
>+    "npapi-carbon-event-model-failure" : {

nit: indentation is a bit messed up here

>+#ifdef XP_MACOSX
>+    else if (eventType == "npapi-carbon-event-model-failure") {

This needs to be a separate if block above this one - as is you can't ever fall in to the PluginNotFound case, because it's in an |else|. This code path should be testable manually in a non-64bit build, and probably should be before this lands.

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+carbonFailurePluginsMessage.restartButton.accesskey=R
> # Sanitize

nit: newline between these two.

r=me, but I haven't tested this at all. We need to make sure this is well tested manually, in addition to addressing Dave's comments and making sure the test is working correctly and providing adequate coverage.

Comment 70

[David Dahl :ddahl]

Attachment: v 0.9.1 nits and comments adressed

Ready for testing

Comment 71

[David Dahl :ddahl]

gavin: perhaps I should push this to try and have qa test it?

Comment 72

[David Dahl :ddahl]

also, we probably don't want to land this until the carbon test plugin is reviewed?

Comment 73

[David Dahl :ddahl]

Attachment: v 0.9.2 made the test listen for the npapi event

Mossop reminded me to listen for the actual NPAPI event in the test

Comment 74

[Dave Townsend [:mossop]]

Since josh isn't around and we want to expedite landing this I'm going to do a manual check today and then we'll get the code parts landed and follow up with the tests after. We want a litmus test for this regardless. Silverlight is a good example, I've been testing with this page:
http://bubblemark.com/silverlight2.html

Comment 75

[David Dahl :ddahl]

Attachment: v 0.9.3 disabled the test for now until carbin test plugin lands

Comment 76

[David Dahl :ddahl]

pushed with test disabled in Makefile:

http://hg.mozilla.org/mozilla-central/rev/6176d7a437b5

Comment 77

[Dave Townsend [:mossop]]

We'll call this fixed as code is in the tree but let's make sure not to let the test languish.

I've verified manually that the JS test should work fine if the carbon test plugin is packaged and working correctly.

Comment 78

[David Dahl :ddahl]

ok, I was holding off, but we will remember:)

Comment 79

[Josh Aas]

Comment on attachment 509441 [details] [diff] [review]
Build a carbon testplugin, rev. 2

This patch is not producing a plugin that uses Carbon. It is producing a plugin just fails to load, which basically means no event model at all and could happen in any plugin on any platform. That may work the way tests are written right now but it isn't what we're intending to test, which is when a plugin expects Carbon events and we can't provide them.

I don't see any reason to actually build a Carbon plugin in the tree. A little i386 binary that negotiates Carbon in NPP_New and does nothing else should be fine. Other uses for this seem very unlikely, can you think of a scenario in which we might use it?

Comment 80

[Josh Aas]

(In reply to comment #79)

> i386 binary that negotiates Carbon in NPP_New

Also, doing nothing and just returning NPERR_NO_ERROR means Carbon by default for i386. So we'd just need an i386 plugin that does nothing but return NPERR_NO_ERROR from NPP_New.

Comment 81

[Josh Aas]

(In reply to comment #79)
> Comment on attachment 509441 [details] [diff] [review]
> Build a carbon testplugin, rev. 2
> 
> This patch is not producing a plugin that uses Carbon. It is producing a plugin
> just fails to load, which basically means no event model at all

I was wrong about this, I read the diff incorrectly. Sorry!

I still don't think we need to actually build this plugin though.

Comment 82

[Benjamin Smedberg]

We currently continue to support carbon plugins and probably will for another 6 months at least. I think we shoul make the standard testplugin use cocoa all the time, and this one use carbon. Then we can remove the carbon testplugin when we remove carbon support.

Comment 83

[Josh Aas]

(In reply to comment #82)
> We currently continue to support carbon plugins and probably will for another 6
> months at least. I think we shoul make the standard testplugin use cocoa all
> the time, and this one use carbon. Then we can remove the carbon testplugin
> when we remove carbon support.

I've haven't seen a practical plan for running our existing tests twice, in two different modes. If we had such a plan, it would almost certainly be easier to use the same plugin and just have it negotiate something different the second time around (probably with an environment variable, which we already have to force Cocoa). There is no need to have a second plugin in order to run with a different event model.

Comment 84

[Benjamin Smedberg]

Having a single plugin means that all of your OOPP preferences cannot be munged separately. Using environment vars means that you cannot runs all of your tests in one standard mochitest/reftest process, but you have to create separate suites. That seems suboptimal. I don't understand why you object to the multiple testplugins anyway, since they are built from the same source and only affect the test package.

Comment 85

[Josh Aas]

Comment on attachment 509441 [details] [diff] [review]
Build a carbon testplugin, rev. 2

I don't want to make everyone build their own copy of what could be an identical binary every time. It's not a big enough issue to delay over though, so r+.

Comment 86

[Dave Townsend [:mossop]]

(In reply to comment #85)
> Comment on attachment 509441 [details] [diff] [review]
> Build a carbon testplugin, rev. 2
> 
> I don't want to make everyone build their own copy of what could be an
> identical binary every time. It's not a big enough issue to delay over though,
> so r+.

Josh, any idea why the test plugin doesn't seem to behave the same as Silverlight (see comment 66)?

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not fixed on my old MacBook with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110211 Firefox/4.0b12pre

Any version of Flash installed does not work with that version. I have to go to the Finder and switch to 32bit mode to make it work. There is no notification bar showing up in Firefox which reminds me to restart in 32bit mode.

Comment 88

[David Dahl :ddahl]

Henrik:

Can you test this with Silverlight? I am not sure flash is affected here. Mossop - is that right?

Comment 89

[Josh Aas]

This bar is not made to show up when a plugin doesn't work - it's only made to show up for the subset of cases where a plugin doesn't work specifically because it negotiated Carbon. It's possible that Flash is failing for a different reason.

Comment 90

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #88)
> Can you test this with Silverlight? I am not sure flash is affected here.
> Mossop - is that right?

When using Silverlight I see the notification bar showing up. I can restart in 32bit mode and the plugin works. Sadly we do not remember that setting and each time I start Minefield, it has to be restarted. Most likely a follow-up.

(In reply to comment #89)
> This bar is not made to show up when a plugin doesn't work - it's only made to
> show up for the subset of cases where a plugin doesn't work specifically
> because it negotiated Carbon. It's possible that Flash is failing for a
> different reason.

So better filing as a new bug? Sounds like that.

Comment 91

[David Dahl :ddahl]

(In reply to comment #90)

> So better filing as a new bug? Sounds like that.

I think so as well.

Comment 92

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As given by comment 90 it works fine for Silverlight. So marking this bug as verified fixed. I have filed bug 634096 for the remaining Flash issue.