557226 - [OOPP] plugin name should be reflected in Mac OS X plugin process names

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Josh Aas]

We should rename plugin processes on Mac OS X.

http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/base/mac_util.mm&q=package:%22src.chromium.org/svn/trunk%22%20setprocessname&sa=N&cd=4&ct=rc&l=377

Comment 1

[Benoit Girard (:BenWa)]

I get 'Minefield Plugin Process' under Activity Monitor. Do we still want a patch for this?

Comment 2

[Josh Aas]

I think we still want to fix this but we won't block on it. I'd approve a patch.

Comment 3

[Benjamin Smedberg]

Isn't this already fixed?

Comment 4

[Josh Aas]

I should have been more specific - I meant for this bug to be about making the process name reflect the particular plugin that the process is for.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: SetProcessName v0.9

Uses the method linked by Josh. I just need to know what format we want to use and how to support localization?

Comment 6

[Benoit Girard (:BenWa)]

Would really like to see this fix finished. Willing to mentor someone fix up this patch. Otherwise I may look at it myself.

Comment 7

[Benjamin Smedberg]

I really don't think we need to support localization for process names.

Comment 8

[Benoit Girard (:BenWa)]

I have this working renaming the process to the plugin's name, such as 'Shockwave Flash'. Do we have any preference for the exact name?

'Plugin Container: Shockwave Flash'?

I'd like to get this landed soon.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: SetProcessName v1.0

Let's get the patch reviewed even I'm still waiting on feedback for the process title.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 539824 [details] [diff] [review]
SetProcessName v1.0

This is basically fine with me, but I think it could be improved.

1) Why not make getASNFunc and setInformationItemFunc static, so they
   only have to be initialized once?

2) What about the case where aProcessName is NULL or (more likely) an
   empty string?  This is probably unusual, but I've seen it myself in
   the last 2-3 days:  On one of my partititions, the QuickTime
   plugin's nsPluginInfo.fName somehow got set to an empty string (I
   saw this in Tools : Add Ons).

3) Unlike the original Chromium code (from SetProcessName() in
   mac_util.mm) your patch doesn't explain why it uses
   GetCurrentProcess().  With no explanation, the call to
   GetCurrentProcess() seems completely unnecessary.

Finally, for the record, I found where this code exists in WebKit --
in the non-open-source WebKitLibraries, under the following thin
wrapper (defined in WebKitSystemInterface.h):

void WKSetVisibleApplicationName(CFStringRef);

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> 1) Why not make getASNFunc and setInformationItemFunc static, so
>    they only have to be initialized once?

Now that I think more about it, this code will probably only be called
once.  If so, this change isn't needed.

Comment 12

[Benoit Girard (:BenWa)]

(In reply to comment #11)
> > 1) Why not make getASNFunc and setInformationItemFunc static, so
> >    they only have to be initialized once?
> 
> Now that I think more about it, this code will probably only be called
> once.  If so, this change isn't needed.

I'm going to make the change anyways since this code will probably get moved and reused when we have content processes by default.

I haven't heard any suggestion about the name format. Should I post to dev.platform?

Comment 13

[Benoit Girard (:BenWa)]

Attachment: SetProcessName v2.0

Thanks for the review Steven. Just waiting on a decision for the process name and I will upload a final version for review.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: SetProcessName v3.0

I fixed 1, 2. I tried removing 3 but the rename failed so I added a comment to indicate that this is required. I'm not sure why it is required.

I couldn't get the appName from the info service in the plugin process so I opted to use cocoa to get the current process name.

The format for the process name is '<PROCESSNAME> (<PLUGIN NAME>)".

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 541723 [details] [diff] [review]
SetProcessName v3.0

This looks fine to me.

One very small nit:  formatedName should be formattedName :-)

Comment 16

[Benoit Girard (:BenWa)]

Green try run, ready for checkin.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: SetProcessName v4.0 (Fixed typo)

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Conflicts with bug 587370.

Comment 19

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/f29f165edae6

Comment 20

[(no longer active)]

With this patch, I get a compile error using clang:

/Users/ehsanakhgari/bin/clang/bin/clang++ -o PluginUtilsOSX.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Darwin10.8.0\" -DOSARCH=Darwin -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_MACOSX=1 -DOS_POSIX=1  -DFORCE_PR_LOG -I/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/../base -I/Users/ehsanakhgari/moz/inbound/xpcom/base/  -I/Users/ehsanakhgari/moz/inbound/ipc/chromium/src -I/Users/ehsanakhgari/moz/inbound/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/ehsanakhgari/moz/inbound/obj-ff-dbg/dist/include/nspr -I/Users/ehsanakhgari/moz/inbound/obj-ff-dbg/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -fno-strict-aliasing -fno-common -fshort-wchar -pthread -DNO_X11 -pipe  -DDEBUG -D_DEBUG -DTRACING -g -DNO_X11   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/PluginUtilsOSX.pp -fobjc-exceptions /Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm
/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm:177:48: error: no member named 'sApplicationASN' in namespace 'mozilla::plugins::PluginUtilsOSX'
static void *mozilla::plugins::PluginUtilsOSX::sApplicationASN = NULL;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
/Users/ehsanakhgari/moz/inbound/dom/plugins/ipc/PluginUtilsOSX.mm:178:48: error: no member named 'sApplicationInfoItem' in namespace 'mozilla::plugins::PluginUtilsOSX'
static void *mozilla::plugins::PluginUtilsOSX::sApplicationInfoItem = NULL;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
2 errors generated.

I think clang is right here, these two variables have not been declared *anywhere* <http://mxr.mozilla.org/mozilla-central/search?string=sApplicationASN>.  I have no idea why gcc doesn't choke on this.

Benoit, can you please fix this?  This is currently blocking me from building, which kind of sucks.  ;-)

Comment 21

[Benoit Girard (:BenWa)]

Attachment: Remove static vars for clang

I'm not entirely sure why the previous were not proper declaration of global variables with static linkage. If someone cares to explain I'd love to know?

I don't think you cared much for having it static so I figured I would take the simplest approach and revert to what I previous had since it has no impact. We're trading off storage size to cache something that wont be called again.

Comment 22

[Benjamin Smedberg]

You're trying to define a variable which was never declared. It has to be declared in the class, e.g.

class PluginUtilsOSX
{
  static void* mozilla::plugins::PluginUtilsOSX::sApplicationASN;
};

And then you define it in the .cpp file without a "static".

Comment 23

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: proposed patch

Comment 24

[(no longer active)]

Comment on attachment 543313 [details] [diff] [review]
proposed patch

Landed on inbound.

Comment 25

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/13644923aacf

Comment 26

[u279076]

QA tracking to verify in Firefox 7.

Comment 27

[Vlad [QA]]

Setting resolution to Verified Fixed on MacOS X 10.6 and 10.7.

STR
To verify that the process from the description is renamed, I have loaded http://www.shockwave.com/home.jsp and opened Activity Monitor. The 'Firefox','Aurora','Nightly' Plugin Process (Shockwave Flash) process is present and renamed. 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0a1) Gecko/20110919 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:8.0a2) Gecko/20110921 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110922 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110921 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0