557225 - [OOPP] plugin processes should not show up in the dock

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Josh Aas]

We should use LSUIElement=1 to stop plugin-processes from showing up in the dock.

Comment 1

[Josh Aas]

Attachment: fix v1.0

I haven't heavily tested this but it appears to work alright in light testing. Because 'TransformProcessType' is a one-way API (no dock icon -> dock icon) it only helps us if we have no dock icon by default.

The other option is to have mozilla-runtime in a sub-bundle, where we can specify LSUIElement=1 directly.

Comment 2

[Josh Aas]

FYI, Chrome 5.0.342.9 uses a sub-bundle that defines LSUIElement=1.

Comment 3

[Benjamin Smedberg]

Comment on attachment 438105 [details] [diff] [review]
fix v1.0

I agree with the general sentiment, but NS_InitXPCOM is not the right place for the call. Probably XRE_main is, or maybe nsBrowserApp.cpp even, since this is Firefox-specific.

Comment 4

[Josh Aas]

I think we should make the runtime app a bundle, not fix this the way fix v1.0 does. Its cleaner, less disruptive.

Comment 5

[Josh Aas]

Blocking 1.9.3 beta1 as this is essential for out-of-process plugins.

Comment 6

[Michael]

This problem appears to have reappeared as of the June 4th or 5th nightly build.

Comment 7

[d.a.]

The plugin process has shown up in the Dock ever since OOPP was enabled, so I can't see how it could reappear if it never went away in the first place.

Comment 8

[RC]

It's currently not doing this for me. Like Michael, I too, have seen it come and go.

Comment 9

[d.a.]

Strange happens every time for me. 

Every time I go to a page with Flash the extra icon will show up in the dock.

Using Flash Version 10.1.81.24 (Gala Preview 2), also happened with Gala Preview 1.

Do you have a page where it doesn't create the extra dock icon or is it completely random?

Comment 10

[Michael]

I can trace it to pages that use flash, but not exclusively.  Killing the dock icon (right click on it, quit) causes Minefield to think that the process has crashed, and tells you what the plugin was.  I've had a flash plugin running and also gotten a *third* dock icon, presumably for some other plugin.

Comment 11

[d.a.]

The plugin-container will only be used for Flash 10.1 and Java by default. No other plugins do not cause the plugin-container to launch.

The OOPP plugins are controlled by the following settings:
dom.ipc.plugins.enabled.flash player.plugin
dom.ipc.plugins.enabled.javaplugin2_npapi.plugin

Was any of those plugins used when no extra dock showed up?

Comment 12

[(not currently active) Ted Mielczarek]

We know what the problem is here, we don't need any additional information. Thanks.

Comment 13

[(not currently active) Ted Mielczarek]

Attachment: Make NSPR use @loader_path instead of @executable_path

This is a prerequisite for the next patch.

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: Use @loader_path when building dylibs, move plugin-container to an app bundle

Here's the rest. This makes a similar change in the Moz build system, using @loader_path instead of @executable_path as the internal name for dylibs. It also builds an app bundle in ipc/app and sticks plugin-container in there.

This patch still doesn't work. The root problem here is that once we run plugin-container from a bundle, it has a hard time finding all its dependent libraries. I have some "install_name_tool" hacks in the Makefile that convert @loader_path -> @loader_path/../../.., so it's able to find all the libraries it depends on directly, but then when it loads libxul, libxul is linked directly to the NSS libs (the one thing I didn't fix), which still use @executable_path, which means that you get an error loading those libraries via libxul.

I'm a bit stumped right now, I have to think about this some more. (Unless someone else has a bright idea.)

Comment 15

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

--> beta2

Comment 16

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

--> beta2

Comment 17

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

So this is OS X only? When running our Mozmill tests against Minefield builds on all platforms, a couple of windows get mystically opened and blocks our execution. Is there another bug around?

Comment 18

[(not currently active) Ted Mielczarek]

Yes, this particular bug is OS X only. There could certainly be a different bug that you're seeing.

Comment 19

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

(In reply to comment #18)
> Yes, this particular bug is OS X only. There could certainly be a different bug
> that you're seeing.

Thanks Ted. I have filed it as bug 575935 now.

Comment 20

[Eduard Grebe]

I see this bug on Firefox 4.0b1. I don't think any of my open tabs actually use Flash content, but nevertheless a separate process launches for the Flash plugin container, with its own dock icon.

MaanMac:~ eduard$ ps -ax | grep Firefox
  360 ??         2:32.55 /Applications/Firefox.app/Contents/MacOS/firefox-bin -psn_0_180268
  364 ??         0:08.03 /Applications/Firefox.app/Contents/MacOS/plugin-container /Library/Internet Plug-Ins/Flash Player.plugin 360 plugin
  401 ttys000    0:00.00 grep Firefox

Comment 21

[Nick Thomas [:nthomas] (UTC+12)]

Eduard, that's a known issue. It's even in the release notes for 4.0b1.

Comment 28

[Benoit Girard (:BenWa)]

Attachment: Load the plugin-container from the App Bundle

This patch depends on the App Bundle build changes proposed by Ted. It loads plugin-container from the new App Bundle and set LSUIElement=1.

Comment 29

[(not currently active) Ted Mielczarek]

Attachment: Make a plugin-container bundle

This patch + Benoit's patch should be what we need. I'm going to split the @loader_path patches off to a new bug.

Comment 30

[(not currently active) Ted Mielczarek]

Benoit has this under control, and I'm leaving on vacation.

Comment 31

[Benoit Girard (:BenWa)]

Attachment: Load the plugin-container from the App Bundle (Using DYLD_LIBRARY_PATH)

Addition to the previous patch:
Ted pointed out that by setting 'DYLD_LIBRARY_PATH' to the value of 'path' then we no longer depend on the changes from the blocker bug(s). I've copied the Linux code to set the env variable.

Comment 32

[Benoit Girard (:BenWa)]

We're not longer blocking on these bugs since we are using 'DYLD_LIBRARY_PATH', we only need the 2 patches in this bug.

Comment 33

[Benjamin Smedberg]

beta2 freeze is *today*, will this land?

Comment 34

[Benoit Girard (:BenWa)]

I'm trying to solve a tryserver failure. I'm investigating if it's related to these patches:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | Error: couldn't open library - See following stack:
JS frame :: /Users/cltbld/talos-slave/tryserver-leopard-opt-u-xpcshell/build/xpcshell/tests/jsctypes-test/unit/test_jsctypes.js :: run_test :: line 92
JS frame :: /Users/cltbld/talos-slave/tryserver-leopard-opt-u-xpcshell/build/xpcshell/head.js :: _execute_test :: line 166

I will land this if I can resolve this error on time.

Comment 35

[Josh Aas]

This is marked as blocking beta2 and we should treat it that way - don't ship until we have this in. This is one of the worst, most visible Mac OS X OOPP bugs.

Comment 36

[Benoit Girard (:BenWa)]

Attachment: Fix TryServer Issues

The tryserver issues was caused by a bug in the xpcshell script not setting DYLD_LIBRARY_PATH.

This also fixes how we set environment variables when spawning the new process.

Comment 37

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

Comment on attachment 458532 [details] [diff] [review]
Fix TryServer Issues

Josh/Chris: can we get a review?

Comment 38

[Josh Aas]

Comment on attachment 458532 [details] [diff] [review]
Fix TryServer Issues

+  int varCount = combined_env_vars.size() + 1;

This should be renamed, it does not store the number of env variables, it stores an array length.

+  char** vars = new char*[varCount]; 

Trailing space.

Otherwise looks good, thanks.

Comment 39

[Benoit Girard (:BenWa)]

Attachment: Fix TryServer Issues v2

Thanks Josh, this patch should be good to go. I just submitted it to the try server.

Comment 40

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

Comment on attachment 458538 [details] [diff] [review]
Fix TryServer Issues v2

>diff --git a/ipc/chromium/src/base/process_util_mac.mm b/ipc/chromium/src/base/process_util_mac.mm
>--- a/ipc/chromium/src/base/process_util_mac.mm
>+++ b/ipc/chromium/src/base/process_util_mac.mm
>@@ -49,15 +49,44 @@
>   SetAllFDsToCloseOnExec();
> 
> #if defined(CHROMIUM_MOZILLA_BUILD)
>-    for (environment_map::const_iterator it = env_vars_to_set.begin();
>-         it != env_vars_to_set.end(); ++it) {
>-      if (setenv(it->first.c_str(), it->second.c_str(), 1/*overwrite*/))
>-        exit(127);
>+  // Copy _NSGetEnviron() to a new char array and add the variables
>+  // in env_vars_to_set.
>+  // Existing variables are overwritten by env_vars_to_set.
>+  int pos = 0;
>+  environment_map combined_env_vars = env_vars_to_set;
>+  while((*_NSGetEnviron())[pos] != NULL) {
>+    std::string varString = (*_NSGetEnviron())[pos];
>+    std::string varName = varString.substr(0, varString.find_first_of('='));
>+    std::string varValue = varString.substr(varString.find_first_of('=') + 1);
>+    if (combined_env_vars.find(varName) == combined_env_vars.end()) {
>+      combined_env_vars[varName] = varValue;
>     }
>+    pos++;
>+  }
>+  int varsLen = combined_env_vars.size() + 1;
>+
>+  char** vars = new char*[varsLen];
>+  int i = 0;
>+  for (environment_map::const_iterator it = combined_env_vars.begin();
>+       it != combined_env_vars.end(); ++it) {
>+    vars[i] = new char[it->first.size() + it->second.size() + 2];
>+    vars[i][0] = NULL;
>+    strcat(vars[i], it->first.c_str());
>+    strcat(vars[i], "=");
>+    strcat(vars[i], it->second.c_str());
>+    i++;

You need to allocate |strlen(first) + 1 + strlen('=') + 1 + strlen(second) + 1| bytes.  But why not let std::string do this for you

  string entry(it->first);
  entry += "=";
  entry += it->second;
  vars[i] = strcpy(entry.c_str());

See below...

>+  }
>+  vars[i] = NULL;
> #endif
> 
>   posix_spawn_file_actions_t file_actions;
>   if (posix_spawn_file_actions_init(&file_actions) != 0) {
>+#if defined(CHROMIUM_MOZILLA_BUILD)
>+    for(int j = 0; j < varsLen; j++) {
>+      delete[] vars[j];

with a |free vars[j];| here ...

>+    }  
>+    delete[] vars;
>+#endif
>     return false;
>   }
> 
>@@ -77,18 +106,36 @@
>       if (posix_spawn_file_actions_adddup2(&file_actions, src_fd, dest_fd) != 0)
>           {
>         posix_spawn_file_actions_destroy(&file_actions);
>+#if defined(CHROMIUM_MOZILLA_BUILD)
>+        for(int j = 0; j < varsLen; j++) {
>+          delete[] vars[j];

and here...

>+        }  
>+        delete[] vars;
>+#endif
>         return false;
>       }
>     }
>   }
> 
>+
>   int pid = 0;
>   int spawn_succeeded = (posix_spawnp(&pid,
>                                       argv_copy[0],
>                                       &file_actions,
>                                       NULL,
>                                       argv_copy,
>+#if defined(CHROMIUM_MOZILLA_BUILD)
>+                                      vars) == 0);
>+#else
>                                       *_NSGetEnviron()) == 0);
>+#endif
>+
>+#if defined(CHROMIUM_MOZILLA_BUILD)
>+  for(int j = 0; j < varsLen; j++) {
>+    delete[] vars[j];

... and here.  Really sucks to duplicate this code, but I don't have any better, quick suggestions for you.

Comment 41

[Benoit Girard (:BenWa)]

Attachment: Fix TryServer Issues v3

Comment 42

[Benoit Girard (:BenWa)]

Attachment: Load the plugin-container from the App Bundle (Using DYLD_LIBRARY_PATH) v2

Comment 43

[Benoit Girard (:BenWa)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/a02255a3f06a
http://hg.mozilla.org/mozilla-central/rev/8274ab91c074
http://hg.mozilla.org/mozilla-central/rev/0e61db192896

Re-added bug 578751 as dependant since I was using an older version of Ted's build changes that still contained the patch in bug 578751.

Comment 44

[Ed Lee :Mardak]

Comment on attachment 457309 [details] [diff] [review]
Make a plugin-container bundle

>@@ -575,7 +575,7 @@
Did something go missing in the first changeset?? This showed up in the commit message:
http://hg.mozilla.org/mozilla-central/rev/a02255a3f06a

Comment 45

[d.a.]

I just tried the latest hourly build and it becomes unresponsive any time it tries to load a page with Flash content on it, only way to fix it is by force quitting the Application. Even in safe mode the build will become unresponsive.

I had a peek the in the application bundle and I can find no reference to the plugin-process bundle. Was it checked in properly? Otherwise it would be a good idea to back this one out.

Comment 46

[Nick Thomas [:nthomas] (UTC+12)]

Confirmed with http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx/1279605383/, which is rev 0c116ba35956. I don't see any packaging changes landing in
 http://hg.mozilla.org/mozilla-central/file/default/browser/installer/package-manifest.in

Something like this ?
#ifdef CHROMIUM_MOZILLA_BUILD
#ifdef XP_MACOSX
@BINPATH@/plugin-container.app/
#endif
#endif

Comment 47

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

Morning, BenWa: we need an evaluation here. If we know what's going on, we need a fix. If not, we need to back this out and take our lumps. Beta 2's got to get moving again.

Comment 48

[d.a.]

The patch works if the plugin-bundle is actually included in the build. I made an app-bundle for the plugin-container with the plist created here and it worked fine.

Comment 49

[Benjamin Smedberg]

Attachment: Packaging fix

I think this is the correct packaging fix, I'm building it locally and running it through tryserver to make sure.

Comment 50

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/91da083f3478
and then http://hg.mozilla.org/mozilla-central/rev/737a08309d97

Comment 51

[d.a.]

The plugin process is currently named Minefield Software Update, this should be updated to show that it's the plugin-process. The name is defined in InfoPlist.strings in plugin-container.app/Contents/Resources/English.lproj

Comment 52

[Benjamin Smedberg]

d.a., can you please file that as a new bug, to keep this report focused?

Comment 53

[d.a.]

Filed bug 580406.

Comment 55

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b2pre) Gecko/20100720 Minefield/4.0b2pre