The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 7

Status

()

Core
IPC
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: Josh Aas, Assigned: BenWa)

Tracking

({verified-aurora, verified-beta})

Trunk
mozilla8
All
Mac OS X
verified-aurora, verified-beta
Points:
---

Firefox Tracking Flags

(firefox7 fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
(Assignee)

Comment 1

7 years ago
I get 'Minefield Plugin Process' under Activity Monitor. Do we still want a patch for this?
(Reporter)

Comment 2

7 years ago
I think we still want to fix this but we won't block on it. I'd approve a patch.
Isn't this already fixed?
(Reporter)

Comment 4

7 years ago
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.
Summary: [OOPP] rename plugin processes on Mac OS X → [OOPP] plugin name should be reflected in Mac OS X plugin process names
(Assignee)

Comment 5

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

Comment 6

6 years ago
Would really like to see this fix finished. Willing to mentor someone fix up this patch. Otherwise I may look at it myself.
Whiteboard: [good first bug][mentor=benwa]
I really don't think we need to support localization for process names.
(Assignee)

Comment 8

6 years ago
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.
Assignee: nobody → bgirard
Whiteboard: [good first bug][mentor=benwa]
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

6 years ago
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.
Attachment #466209 - Attachment is obsolete: true
Attachment #539824 - Flags: review?(smichaud)
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);
> 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.
(Assignee)

Comment 12

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

Comment 13

6 years ago
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.
Attachment #539824 - Attachment is obsolete: true
Attachment #539824 - Flags: review?(smichaud)
(Assignee)

Comment 14

6 years ago
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>)".
Attachment #540462 - Attachment is obsolete: true
Attachment #541723 - Flags: review?(smichaud)
Comment on attachment 541723 [details] [diff] [review]
SetProcessName v3.0

This looks fine to me.

One very small nit:  formatedName should be formattedName :-)
Attachment #541723 - Flags: review?(smichaud) → review+
(Assignee)

Comment 16

6 years ago
Green try run, ready for checkin.
Keywords: checkin-needed
(Assignee)

Comment 17

6 years ago
Created attachment 541933 [details] [diff] [review]
SetProcessName v4.0 (Fixed typo)
Attachment #541723 - Attachment is obsolete: true
Conflicts with bug 587370.
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/f29f165edae6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
status-firefox7: --- → fixed
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.  ;-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

6 years ago
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.
Attachment #543003 - Flags: review?(smichaud)
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".
Created attachment 543313 [details] [diff] [review]
proposed patch
Attachment #543313 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #543003 - Attachment is obsolete: true
Attachment #543003 - Flags: review?(smichaud)
Attachment #543313 - Flags: review?(benjamin) → review?(bgirard)
(Assignee)

Updated

6 years ago
Attachment #543313 - Flags: review?(bgirard) → review+
Comment on attachment 543313 [details] [diff] [review]
proposed patch

Landed on inbound.
http://hg.mozilla.org/mozilla-central/rev/13644923aacf
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
QA tracking to verify in Firefox 7.
Whiteboard: [qa+]

Comment 27

6 years ago
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
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.