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.

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?)

One-time restart only, please, Vasily.

(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.

(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

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.

(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?

(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

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

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

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.

Filed bug 629401 for platform work.

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

needs notificationBox element, tests and tweaks to LaunchChildMac

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

will post my latest WIP for feedback later today

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

Comment on attachment 508913 [details] [diff] [review] v 0.1 Front end code, with test hg has borked this patch

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 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 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.

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

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

forgot to re-enable properties

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

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.

(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

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.

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.

Josh: Which commonly available plugin do you recommend for manual testing?

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 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 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 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?

(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.

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

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.

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.

(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?

(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.

still working out the carbon only test plugin

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

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

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.

(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 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 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.

(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.

(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

(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

Oops missed that, ignore me

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...

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

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 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.

(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)

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 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 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.

Ready for testing

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

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

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

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

pushed with test disabled in Makefile: http://hg.mozilla.org/mozilla-central/rev/6176d7a437b5

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.

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

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?

(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.

(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.

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.

(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.

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 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+.

(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)?

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.

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

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.

(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.

(In reply to comment #90) > So better filing as a new bug? Sounds like that. I think so as well.

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.