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

VERIFIED FIXED in mozilla2.0b12

Status

()

defect
--
major
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: beltzner, Assigned: ddahl)

Tracking

({qawanted, user-doc-needed})

Trunk
mozilla2.0b12
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [strings][hardblocker][pushed-without-test-for-now][ETA Feb-04])

Attachments

(2 attachments, 21 obsolete attachments)

7.71 KB, patch
jaas
: review+
Details | Diff | Splinter Review
12.92 KB, patch
ddahl
: review+
Details | Diff | Splinter Review
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.
blocking2.0: --- → betaN+
Whiteboard: [strings][hardblocker]
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.

Updated

9 years ago
Keywords: user-doc-needed
Assignee

Comment 3

9 years ago
(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.
Assignee

Updated

9 years ago
Assignee: nobody → ddahl
Assignee

Comment 6

9 years ago
(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
Assignee

Comment 8

9 years ago
Josh:

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

Comment 9

9 years ago
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.
Assignee

Updated

9 years ago
Depends on: 629401
Assignee

Comment 11

9 years ago
Filed bug 629401 for platform work.
Assignee

Comment 13

9 years ago
Posted patch 0.1 Front-end WIP patch (obsolete) — Splinter Review
needs notificationBox element, tests and tweaks to LaunchChildMac
Assignee

Comment 14

9 years ago
Posted patch v 0.1.1 WIP saving work (obsolete) — Splinter Review
Attachment #507722 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Whiteboard: [strings][hardblocker] → [strings][hardblocker][has-wip-patch]
Strings risk means we want to lock this one down RSN - how's it feeling ddahl?
Assignee

Comment 16

9 years ago
will post my latest WIP for feedback later today
Whiteboard: [strings][hardblocker][has-wip-patch] → [strings][hardblocker][has-wip-patch][b11]
Assignee

Comment 17

9 years ago
restart bits not quite done - i am not sure how to get env vars from the c++ side in LaunchChildMac.mm
Attachment #507981 - Attachment is obsolete: true
Attachment #508909 - Flags: feedback?(dtownsend)
Assignee

Comment 18

9 years ago
Attachment #508909 - Attachment is obsolete: true
Attachment #508913 - Flags: feedback?(dtownsend)
Attachment #508909 - Flags: feedback?(dtownsend)
Assignee

Comment 19

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

hg has borked this patch
Attachment #508913 - Flags: feedback?(dtownsend)
Assignee

Comment 20

9 years ago
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.
Attachment #508913 - Attachment is obsolete: true
Attachment #508926 - Flags: review?(dtownsend)
Attachment #508926 - Flags: review?(dtownsend) → feedback?(gavin.sharp)
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.
Attachment #508926 - Flags: feedback?(gavin.sharp) → feedback-
Removing from b11 radar, but ETA to be on trunk for Friday.
Whiteboard: [strings][hardblocker][has-wip-patch][b11] → [strings][hardblocker][has-wip-patch][ETA Feb-04]
Assignee

Comment 24

9 years ago
Posted patch v 0.2 feedback addressed (obsolete) — Splinter Review
I still need to find out if the test is going to work at all
Attachment #508926 - Attachment is obsolete: true
Attachment #509131 - Flags: review?(gavin.sharp)
Assignee

Comment 25

9 years ago
Posted patch v 0.2 feedback addressed (obsolete) — Splinter Review
forgot to re-enable properties
Attachment #509131 - Attachment is obsolete: true
Attachment #509133 - Flags: review?(gavin.sharp)
Attachment #509131 - Flags: review?(gavin.sharp)
Assignee

Comment 26

9 years ago
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.
Assignee

Comment 28

9 years ago
(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

9 years ago
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.
Assignee

Comment 31

9 years ago
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.
Attachment #509133 - Flags: feedback-
Assignee

Comment 34

9 years ago
Posted patch v 0.3 feedback addressed (obsolete) — Splinter Review
Attachment #509133 - Attachment is obsolete: true
Attachment #509172 - Flags: review?(dtownsend)
Attachment #509133 - Flags: review?(gavin.sharp)
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.
Attachment #509172 - Flags: review?(dtownsend) → review-
Assignee

Comment 36

9 years ago
Attachment #509172 - Attachment is obsolete: true
Attachment #509266 - Flags: review?(dtownsend)
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?
Attachment #509266 - Flags: review?(gavin.sharp)
Attachment #509266 - Flags: review?(dtownsend)
Attachment #509266 - Flags: review+
Assignee

Updated

9 years ago
Whiteboard: [strings][hardblocker][has-wip-patch][ETA Feb-04] → [strings][hardblocker][has-patch][ETA Feb-04]

Updated

9 years ago
Whiteboard: [strings][hardblocker][has-patch][ETA Feb-04] → [strings][hardblocker][has patch][ETA Feb-04]
Assignee

Comment 38

9 years ago
(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.
Assignee

Comment 42

9 years ago
(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?
Assignee

Comment 43

9 years ago
(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.
Assignee

Comment 44

9 years ago
Posted patch v 0.5.2 Comments adressed (obsolete) — Splinter Review
still working out the carbon only test plugin
Attachment #509266 - Attachment is obsolete: true
Attachment #509436 - Flags: review?(gavin.sharp)
Attachment #509266 - Flags: review?(gavin.sharp)
Posted patch Build a carbon testplugin (obsolete) — Splinter Review
Attachment #509437 - Flags: review?(joshmoz)
That's what I get for copy-pasting from other makefiles: __LP64__ is not a makefile variable.
Attachment #509437 - Attachment is obsolete: true
Attachment #509441 - Flags: review?(joshmoz)
Attachment #509437 - Flags: review?(joshmoz)
Assignee

Comment 47

9 years ago
The test actually shows the missing plugins notification banner on my 32 bit machine. maybe it'll work on a 64 bit mac.
Attachment #509436 - Attachment is obsolete: true
Attachment #509444 - Flags: review?(gavin.sharp)
Attachment #509436 - Flags: review?(gavin.sharp)
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.
Assignee

Comment 49

9 years ago
(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.
Assignee

Comment 50

9 years ago
Attachment #509444 - Attachment is obsolete: true
Attachment #509459 - Flags: review?(gavin.sharp)
Attachment #509444 - Flags: review?(gavin.sharp)
Assignee

Comment 51

9 years ago
Attachment #509459 - Attachment is obsolete: true
Attachment #509460 - Flags: review?(gavin.sharp)
Attachment #509459 - Flags: review?(gavin.sharp)
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.
Assignee

Comment 55

9 years ago
Attachment #509460 - Attachment is obsolete: true
Attachment #509484 - Flags: review?(gavin.sharp)
Attachment #509460 - Flags: review?(gavin.sharp)
(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
Assignee

Comment 57

9 years ago
(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...
Attachment #509484 - Flags: review?(gavin.sharp) → review-
Assignee

Comment 60

9 years ago
also: the test was leaking on 32 bit mac os x because i was not removing an eventListener
Attachment #509484 - Attachment is obsolete: true
Attachment #509588 - Flags: review?(gavin.sharp)
Assignee

Comment 61

9 years ago
Attachment #509588 - Attachment is obsolete: true
Attachment #509601 - Flags: review?(gavin.sharp)
Attachment #509588 - Flags: review?(gavin.sharp)
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)...
Attachment #509601 - Flags: review?(gavin.sharp) → review-
Assignee

Comment 63

9 years ago
Attachment #509601 - Attachment is obsolete: true
Attachment #509612 - Flags: review?(gavin.sharp)
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.
Attachment #509612 - Flags: review?(gavin.sharp)
Assignee

Comment 65

9 years ago
Posted patch v 0.9 comments addressed (obsolete) — Splinter Review
Attachment #509612 - Attachment is obsolete: true
Attachment #509621 - Flags: review?(gavin.sharp)
(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.
Attachment #509621 - Flags: review?(gavin.sharp) → review+
Assignee

Comment 70

9 years ago
Ready for testing
Attachment #509621 - Attachment is obsolete: true
Attachment #509777 - Flags: review+
Assignee

Updated

9 years ago
Keywords: qawanted
Assignee

Comment 71

9 years ago
gavin: perhaps I should push this to try and have qa test it?
Assignee

Comment 72

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

Comment 73

9 years ago
Mossop reminded me to listen for the actual NPAPI event in the test
Attachment #509777 - Attachment is obsolete: true
Attachment #509807 - Flags: review+
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
Flags: in-testsuite?
Flags: in-litmus?
Assignee

Comment 76

9 years ago
pushed with test disabled in Makefile:

http://hg.mozilla.org/mozilla-central/rev/6176d7a437b5
Target Milestone: --- → mozilla2.0b12
Assignee

Updated

9 years ago
Whiteboard: [strings][hardblocker][has patch][ETA Feb-04] → [strings][hardblocker][pushed-without-test-for-now][ETA Feb-04]
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee

Comment 78

9 years ago
ok, I was holding off, but we will remember:)
Version: unspecified → Trunk

Comment 79

9 years ago
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

9 years ago
(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

9 years ago
(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.

Comment 83

9 years ago
(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 85

9 years ago
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+.
Attachment #509441 - Flags: review?(joshmoz) → review+
(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.
Severity: normal → major
Status: RESOLVED → REOPENED
Hardware: x86 → x86_64
Resolution: FIXED → ---
Assignee

Comment 88

9 years ago
Henrik:

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

Comment 89

9 years ago
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.
Assignee

Comment 91

9 years ago
(In reply to comment #90)

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

I think so as well.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED

Updated

8 years ago
Blocks: 676364
No longer blocks: 676364

Updated

6 years ago
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.