Last Comment Bug 694039 - Work around OpenGL quirks in the plugin process
: Work around OpenGL quirks in the plugin process
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Benoit Girard (:BenWa)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 729446
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-12 09:23 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-02-23 12:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Trigger the quirks (4.30 KB, patch)
2011-10-12 09:23 PDT, Jeff Muizelaar [:jrmuizel]
b56girard: review-
Details | Diff | Splinter Review
Trigger the quirks v2 (3.69 KB, patch)
2011-10-13 06:18 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Trigger the quirks v3 (3.97 KB, patch)
2011-10-13 13:13 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Trigger the quirks v4 (4.44 KB, patch)
2011-10-18 12:30 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Trigger the quirks v5 (4.45 KB, patch)
2011-10-19 07:47 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Trigger the quirks v5 (4.47 KB, patch)
2011-10-19 07:48 PDT, Jeff Muizelaar [:jrmuizel]
b56girard: review+
smichaud: review+
Details | Diff | Splinter Review
Whitelist plugin that can use OfflineRenderer features (13.16 KB, patch)
2011-10-20 14:26 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Splinter Review
Part 3: Load plugin quirks during interpose (8.01 KB, patch)
2011-10-25 13:57 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Splinter Review
Part 2: Add OfflineRenderer Support to CARenderer (14.40 KB, patch)
2011-10-26 19:59 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 3: Load plugin quirks during interpose (15.69 KB, patch)
2011-10-26 20:00 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Follow up fix: Remove printf (1.39 KB, patch)
2011-10-28 07:19 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-10-12 09:23:30 PDT
Created attachment 566568 [details] [diff] [review]
Trigger the quirks

Certain OpenGL behaviour on 10.6 depends on a bundle id of org.mozilla.firefox. We want that behaviour for our plugin process as well. This patch also ensure that we get this bundle id on Nightly and Aurora.

Note: I'd appreciate a thorough check of the reference counting behaviour as I'm not certain I did it correctly.
Comment 1 Steven Michaud [:smichaud] (Retired) 2011-10-12 09:35:53 PDT
Comment on attachment 566568 [details] [diff] [review]
Trigger the quirks

I may not be able to get to this until next week.

When I do, I'll test to make sure this strategy doesn't have the problem described in bug 689764 comment #10.
Comment 2 Benoit Girard (:BenWa) 2011-10-12 14:26:55 PDT
Comment on attachment 566568 [details] [diff] [review]
Trigger the quirks

Review of attachment 566568 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/nsAppRunner.cpp
@@ +2625,5 @@
>    NS_TIME_FUNCTION;
>  
> +        CFBundleRef mainBundle = CFBundleGetMainBundle();
> +        CFMutableDictionaryRef dict = (CFMutableDictionaryRef)CFBundleGetInfoDictionary(mainBundle);
> +        CFDictionarySetValue(dict, CFSTR("CFBundleIdentifier"), CFSTR("org.mozilla.firefox"));

Should this be removed?

@@ +3787,5 @@
> +  mib[0] = CTL_KERN;
> +  mib[1] = KERN_OSRELEASE;
> +  char release[7];
> +  size_t len = sizeof(release);
> +  int ret = sysctl(mib, 2, release, &len, NULL, 0);

The usage of sysctl doesn't appear to be correct. Parameter 3 is an output parameter so I'm not sure why you would pass the sizeof(char[7]). 

This is the best usage example I could find (but I'm in a car so that doesn't stand for much):
http://cocoadevcentral.com/articles/000067.php

Shouldn't we call it once before to make sure len is <= 7.

::: toolkit/xre/nsEmbedFunctions.cpp
@@ -303,3 +303,5 @@
> >  }
> >  #endif
> >  
> > +#ifdef XP_MACOSX
> > +#endif

This isn't needed
Comment 3 Jeff Muizelaar [:jrmuizel] 2011-10-12 14:48:25 PDT
(In reply to Benoit Girard (:BenWa) from comment #2)
> > +        CFDictionarySetValue(dict, CFSTR("CFBundleIdentifier"), CFSTR("org.mozilla.firefox"));
> 
> Should this be removed?

Yes. In fact I thought I had.
> 
> The usage of sysctl doesn't appear to be correct. Parameter 3 is an output
> parameter so I'm not sure why you would pass the sizeof(char[7]). 

sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp, size_t newlen);

"The size of the buffer is given by the location specified by oldlenp before the call, and that location gives the amount of data copied after a successful call and after a call that returns with the error code ENOMEM."

So it's an input parameter and an output parameter.

> 
> Shouldn't we call it once before to make sure len is <= 7.

Nope the result is truncated and if it's truncated we get ENOMEM back.
Comment 4 Jeff Muizelaar [:jrmuizel] 2011-10-13 06:18:55 PDT
Created attachment 566795 [details] [diff] [review]
Trigger the quirks v2
Comment 5 Jeff Muizelaar [:jrmuizel] 2011-10-13 13:13:33 PDT
Created attachment 566915 [details] [diff] [review]
Trigger the quirks v3

This fixes the version compare logic to do the right thing and adds some more comments
Comment 6 Joe Drew (not getting mail) 2011-10-13 13:24:19 PDT
Comment on attachment 566915 [details] [diff] [review]
Trigger the quirks v3

Review of attachment 566915 [details] [diff] [review]:
-----------------------------------------------------------------

Couple of drive-bys.

::: toolkit/xre/nsAppRunner.cpp
@@ +3783,5 @@
> +
> +  mib[0] = CTL_KERN;
> +  mib[1] = KERN_OSRELEASE;
> +  // we won't support versions greater than 10.8.99
> +  char release[sizeof("10.99.0")];

These two don't match, and while they're the same length, they ought to be the same exactly.

@@ +3788,5 @@
> +  size_t len = sizeof(release);
> +  // sysctl will return ENOMEM if the release string is longer than sizeof(release)
> +  int ret = sysctl(mib, 2, release, &len, NULL, 0);
> +  // we only want to trigger this on OS X 10.6, on versions 10.6.8 or newer
> +  if (ret == 0 && NS_CompareVersions(release, "10.8.0") >= 0 && NS_CompareVersions(release, "11") < 0) {

Add a comment saying that Darwin version 10 corresponds to OS X version 10.6.

::: toolkit/xre/nsAppRunner.h
@@ +193,5 @@
>   * and the JIT debugger on Windows, and install unix signal handlers.
>   */
>  void SetupErrorHandling(const char* progname);
>  
> +void TriggerQuirks();

Add a comment?
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-10-18 12:30:25 PDT
Created attachment 567842 [details] [diff] [review]
Trigger the quirks v4
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-10-18 13:42:45 PDT
Comment on attachment 567842 [details] [diff] [review]
Trigger the quirks v4

I haven't yet tested this patch (though I'm about to).  But I do have
a question:

You say you need to call Gestalt to "load the quirks table"
(presumably meaning that you need it to	"register" the change you've
just made to the main bundle's info dictionary).  And right after the
call to Gestalt	you restore the main bundle's info dictionary to its
original state.

But Gestalt is called from many different places in the	tree, some of
which may happen after you've restored the main bundle's info
dictionary.  Might these calls to Gestalt "unregister" the change
you've made in TriggerQuirks?
Comment 9 Benoit Girard (:BenWa) 2011-10-18 15:28:43 PDT
Comment on attachment 567842 [details] [diff] [review]
Trigger the quirks v4

Review of attachment 567842 [details] [diff] [review]:
-----------------------------------------------------------------

Steven do you know if this requires an autorelease pool on place? How do we know if the methods invoked from here require one?

::: toolkit/xre/nsAppRunner.cpp
@@ +3811,5 @@
> +  // http://en.wikipedia.org/wiki/Darwin_(operating_system)#Release_history
> +  if (ret == 0 && NS_CompareVersions(release, "10.8.0") >= 0 && NS_CompareVersions(release, "11") < 0) {
> +    CFBundleRef mainBundle;
> +    CFStringRef bundleID;
> +    mainBundle = CFBundleGetMainBundle();

We need to check this call:

http://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFBundleRef/Reference/reference.html#//apple_ref/c/func/CFBundleGetMainBundle
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-10-18 15:32:25 PDT
Comment on attachment 567842 [details] [diff] [review]
Trigger the quirks v4

This is an interesting patch.  But I doubt it will be useful to you in
its current state.  And changes to make it useful could run up against
bug 689764:  Permanently changing the plugin-container bundle ID to
"org.mozilla.firefox" will confuse apps that expect to be able to send
Apple events to the app corresponding to that bundle ID -- they'll
often end up sending Apple events to plugin-container instead of to
firefox-bin.

What's interesting about this patch is that it's possible to
programmatically change a running app's bundle ID.  You don't need to
Gestalt to do this -- it works just fine to use
CFBundleGetInfoDictionary to get the main bundle's dictionary, then
use CFDictionarySetValue to change the bundle ID in that dictionary.
However	you can't leave	it set to this value --	otherwise you see the
same problems as when you change plugin-container's ID to
"org.mozilla.firefox" in plugin-container's Info.plist (described in
bug 689764 comment #10).

Additional work is (probably) needed to find out exactly when you need
to change plugin-container's bundle ID to "org.mozilla.firefox", and
when to change it back.	

Where did you get the idea of using Gestalt?  Have you tested to make
sure that it "works"?  Is the idea (or hope) that when it's called for
the first time, it causes the loading of certain libraries?
Comment 11 Steven Michaud [:smichaud] (Retired) 2011-10-18 15:35:41 PDT
(In reply to comment #9)

> Steven do you know if this requires an autorelease pool on place?

As best I can tell it doesn't, because it doesn't use any Cocoa methods (or Objective-C code).

> How do we know if the methods invoked from here require one?

As I said, I'm quite sure we don't need an autorelease pool.  I'm also pretty sure that if we did need one (and didn't have one), we'd see autorelease error messages in the system log.
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-10-18 16:03:33 PDT
Comment on attachment 567842 [details] [diff] [review]
Trigger the quirks v4

Another problem:  The version checking in TriggerQuirks doesn't work properly -- it doesn't do anything on OS X 10.7.2.

I'll need to figure that out tomorrow.
Comment 13 Jeff Muizelaar [:jrmuizel] 2011-10-18 21:28:43 PDT
(In reply to Steven Michaud from comment #8)
> Comment on attachment 567842 [details] [diff] [review] [diff] [details] [review]
> Trigger the quirks v4
> 
> But Gestalt is called from many different places in the	tree, some of
> which may happen after you've restored the main bundle's info
> dictionary.  Might these calls to Gestalt "unregister" the change
> you've made in TriggerQuirks?

No, I checked, and the quirks are only loaded once per process. We only need the bundle-id to be org.mozilla.firefox during this process, after that it doesn't matter anymore.

(In reply to Steven Michaud from comment #10)
> Additional work is (probably) needed to find out exactly when you need
> to change plugin-container's bundle ID to "org.mozilla.firefox", and
> when to change it back.

I already did this work. Calling Gestalt triggers the loading of the quirks. After the quirks are loaded the bundle ID can be switched back.

> Where did you get the idea of using Gestalt?  Have you tested to make
> sure that it "works"?  Is the idea (or hope) that when it's called for
> the first time, it causes the loading of certain libraries?

I ran across the fact that Gestalt loads the quirks when trying to do version detection for this patch. I've tested this and it works fine.
Comment 14 Jeff Muizelaar [:jrmuizel] 2011-10-18 21:29:33 PDT
(In reply to Steven Michaud from comment #12)
> Comment on attachment 567842 [details] [diff] [review] [diff] [details] [review]
> Trigger the quirks v4
> 
> Another problem:  The version checking in TriggerQuirks doesn't work
> properly -- it doesn't do anything on OS X 10.7.2.
> 
> I'll need to figure that out tomorrow.

We only need this on 10.6, so it is by design that nothing happens on 10.7.2
Comment 15 Jeff Muizelaar [:jrmuizel] 2011-10-19 07:47:35 PDT
Created attachment 568056 [details] [diff] [review]
Trigger the quirks v5
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-10-19 07:48:50 PDT
Created attachment 568057 [details] [diff] [review]
Trigger the quirks v5

Forgot to qrefresh
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-10-19 07:54:18 PDT
Jeff, what do you mean by "the quirks", and "loading the quirks"?
Comment 18 Jeff Muizelaar [:jrmuizel] 2011-10-19 08:01:41 PDT
(In reply to Steven Michaud from comment #17)
> Jeff, what do you mean by "the quirks", and "loading the quirks"?

It seems like CoreServices keeps a list of behaviour changes keyed off of bundle-id. This is "loaded" by asking CoreServices for them with the function 'GetBugsForOurBundleIDFromCoreservicesd'
Comment 19 Steven Michaud [:smichaud] (Retired) 2011-10-19 08:53:23 PDT
Thanks for the info.

The GetBugsForOurBundleIDFromCoreservicesd method is (on SnowLeopard
at least) implemented in the CarbonCore framework, which is a
subframework of the CoreServices framework.  It's called many times
while an app is running, from many different methods, including
Gestalt (I tested in Safari, and in Firefox both with and without your
patch).  So I'd guess that you only need the bundle ID set
appropriately for the first call to
GetBugsForOurBundleIDFromCoreservicesd.

In my tests with your patch, I find that
GetBugsForOurBundleIDFromCoreservicesd *is* called for the first time
(in firefox-bin) from the call to Gestalt in TriggerQuirks.  I'm
unable to test this with plugin-container.  But your patch does call
TriggerQuirks very early from plugin-container -- so there's a good
chance it's called "early enough".
Comment 20 Benoit Girard (:BenWa) 2011-10-20 14:26:03 PDT
Created attachment 568519 [details] [diff] [review]
Whitelist plugin that can use OfflineRenderer features
Comment 21 Marco Bonardo [::mak] 2011-10-21 08:33:09 PDT
https://hg.mozilla.org/mozilla-central/rev/17ef2ebb8ef9
Comment 22 Benoit Girard (:BenWa) 2011-10-24 14:22:37 PDT
Comment on attachment 568519 [details] [diff] [review]
Whitelist plugin that can use OfflineRenderer features

Patch works fine for Flash. We allow Quicktime but they don't appear to allow offline renderer properly.

We should also contact GoogleTalk to properly support this.
Comment 23 Benoit Girard (:BenWa) 2011-10-25 13:57:49 PDT
Created attachment 569493 [details] [diff] [review]
Part 3: Load plugin quirks during interpose
Comment 24 Jeff Muizelaar [:jrmuizel] 2011-10-26 12:48:08 PDT
Comment on attachment 568519 [details] [diff] [review]
Whitelist plugin that can use OfflineRenderer features

Review of attachment 568519 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +1991,5 @@
> +
> +#ifdef XP_MACOSX
> +    // Whitelist Flash and Quicktime to support offline renderer
> +    NS_NAMED_LITERAL_CSTRING(flash, "application/x-shockwave-flash");
> +    NS_NAMED_LITERAL_CSTRING(quicktime, "QuickTime Plugin.plugin");

You shouldn't need to use LITERAL_CSTRING here.

::: dom/plugins/ipc/PluginUtilsOSX.h
@@ +85,5 @@
>    bool HasFrontSurface();
>    bool HasCALayer();
>  
>    void SetCALayer(void *aCALayer);
> +  bool InitFrontSurface(size_t aWidth, size_t aHeight, bool aAllowOfflineRenderer);

I think it would be better to use an enum here. That will make it obvious from the caller what is being asked for.
Comment 25 Jeff Muizelaar [:jrmuizel] 2011-10-26 12:52:11 PDT
Comment on attachment 569493 [details] [diff] [review]
Part 3: Load plugin quirks during interpose

Review of attachment 569493 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/interpose/plugin_child_quirks.mm
@@ +52,5 @@
> +
> +PRInt32
> +NS_CompareVersions(const char *A, const char *B);
> +
> +int static_init() {

I'd rather we share this function in a header. It's sort of crazy so I'd rather have it in a single place.

@@ +81,5 @@
> +        // Calling Gestalt will trigger a load of the quirks table for org.mozilla.firefox
> +        SInt32 major;
> +        ProcessSerialNumber psn;
> +        ::GetCurrentProcess(&psn);
> +        ::Gestalt(gestaltSystemVersionMajor, &major);

I would use an #ifdef on the platform here along with a comment about how one works on x64 and the other on x86
Comment 26 Benoit Girard (:BenWa) 2011-10-26 15:01:19 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> Comment on attachment 568519 [details] [diff] [review] [diff] [details] [review]
> Whitelist plugin that can use OfflineRenderer features
> 
> Review of attachment 568519 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/PluginModuleChild.cpp
> @@ +1991,5 @@
> > +
> > +#ifdef XP_MACOSX
> > +    // Whitelist Flash and Quicktime to support offline renderer
> > +    NS_NAMED_LITERAL_CSTRING(flash, "application/x-shockwave-flash");
> > +    NS_NAMED_LITERAL_CSTRING(quicktime, "QuickTime Plugin.plugin");
> 
> You shouldn't need to use LITERAL_CSTRING here.

This is used all throughout the plugin quirks. I think we're better off keeping the code homogenous.
Comment 27 Benoit Girard (:BenWa) 2011-10-26 19:59:48 PDT
Created attachment 569891 [details] [diff] [review]
Part 2: Add OfflineRenderer Support to CARenderer
Comment 28 Benoit Girard (:BenWa) 2011-10-26 20:00:20 PDT
Created attachment 569892 [details] [diff] [review]
Part 3: Load plugin quirks during interpose
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2011-10-28 03:14:03 PDT
Comment on attachment 569892 [details] [diff] [review]
Part 3: Load plugin quirks during interpose

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation

It isn't, it's MoFo.

>+int static_init() {
>+  printf("test\n");

Eh?
Comment 31 Ed Morley [:emorley] 2011-10-28 04:39:28 PDT
https://hg.mozilla.org/mozilla-central/rev/c0fe9c38bd1b
https://hg.mozilla.org/mozilla-central/rev/ce3aa6f254b7

Benoit, I'm presuming comment 30 needs dealing with yeah? Leaving open for that.
Comment 32 Benoit Girard (:BenWa) 2011-10-28 07:19:36 PDT
Created attachment 570258 [details] [diff] [review]
Follow up fix: Remove printf
Comment 34 Marco Bonardo [::mak] 2011-11-03 08:27:43 PDT
https://hg.mozilla.org/mozilla-central/rev/05ec576d7b3b

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