Last Comment Bug 557226 - [OOPP] plugin name should be reflected in Mac OS X plugin process names
: [OOPP] plugin name should be reflected in Mac OS X plugin process names
Status: VERIFIED FIXED
[qa!]
: verified-aurora, verified-beta
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: All Mac OS X
: -- normal with 2 votes (vote)
: mozilla8
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-05 09:14 PDT by Josh Aas
Modified: 2011-09-23 02:46 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
unaffected
unaffected


Attachments
SetProcessName v0.9 (4.29 KB, patch)
2010-08-15 20:04 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
SetProcessName v1.0 (5.55 KB, patch)
2011-06-16 10:33 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
SetProcessName v2.0 (5.85 KB, patch)
2011-06-20 07:44 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
SetProcessName v3.0 (6.62 KB, patch)
2011-06-24 11:09 PDT, Benoit Girard (:BenWa)
smichaud: review+
Details | Diff | Splinter Review
SetProcessName v4.0 (Fixed typo) (6.63 KB, patch)
2011-06-25 08:36 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Remove static vars for clang (2.39 KB, patch)
2011-06-29 16:50 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
proposed patch (1.09 KB, patch)
2011-06-30 17:58 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
bgirard: review+
Details | Diff | Splinter Review

Comment 1 Benoit Girard (:BenWa) 2010-08-15 15:17:14 PDT
I get 'Minefield Plugin Process' under Activity Monitor. Do we still want a patch for this?
Comment 2 Josh Aas 2010-08-15 16:37:29 PDT
I think we still want to fix this but we won't block on it. I'd approve a patch.
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-08-15 16:38:47 PDT
Isn't this already fixed?
Comment 4 Josh Aas 2010-08-15 16:47:30 PDT
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) 2010-08-15 20:04:37 PDT
Created attachment 466209 [details] [diff] [review]
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) 2011-06-09 15:56:17 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-10 08:21:24 PDT
I really don't think we need to support localization for process names.
Comment 8 Benoit Girard (:BenWa) 2011-06-15 11:14:34 PDT
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) 2011-06-16 10:33:50 PDT
Created attachment 539824 [details] [diff] [review]
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) 2011-06-16 15:41:04 PDT
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) 2011-06-17 07:58:59 PDT
> 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) 2011-06-20 06:43:35 PDT
(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) 2011-06-20 07:44:41 PDT
Created attachment 540462 [details] [diff] [review]
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) 2011-06-24 11:09:30 PDT
Created attachment 541723 [details] [diff] [review]
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) 2011-06-24 12:05:40 PDT
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) 2011-06-25 08:34:07 PDT
Green try run, ready for checkin.
Comment 17 Benoit Girard (:BenWa) 2011-06-25 08:36:11 PDT
Created attachment 541933 [details] [diff] [review]
SetProcessName v4.0 (Fixed typo)
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-06-26 09:22:06 PDT
Conflicts with bug 587370.
Comment 19 Joe Drew (not getting mail) 2011-06-28 18:52:52 PDT
http://hg.mozilla.org/mozilla-central/rev/f29f165edae6
Comment 20 :Ehsan Akhgari 2011-06-29 14:08:52 PDT
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) 2011-06-29 16:50:11 PDT
Created attachment 543003 [details] [diff] [review]
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-30 09:09:14 PDT
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) 2011-06-30 17:58:49 PDT
Created attachment 543313 [details] [diff] [review]
proposed patch
Comment 24 :Ehsan Akhgari 2011-07-12 10:31:54 PDT
Comment on attachment 543313 [details] [diff] [review]
proposed patch

Landed on inbound.
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:20:45 PDT
QA tracking to verify in Firefox 7.
Comment 27 Vlad [QA] 2011-09-23 02:46:27 PDT
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

Note You need to log in before you can comment on or make changes to this bug.