Work around OpenGL quirks in the plugin process

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: BenWa)

Tracking

(Depends on: 1 bug)

unspecified
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
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.
Attachment #566568 - Flags: review?
(Reporter)

Updated

6 years ago
Attachment #566568 - Flags: review?(smichaud)
Attachment #566568 - Flags: review?(bgirard)
Attachment #566568 - Flags: review?
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.
(Assignee)

Comment 2

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

Updated

6 years ago
Attachment #566568 - Flags: review?(bgirard) → review-
(Reporter)

Comment 3

6 years ago
(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.
(Reporter)

Comment 4

6 years ago
Created attachment 566795 [details] [diff] [review]
Trigger the quirks v2
Attachment #566568 - Attachment is obsolete: true
Attachment #566568 - Flags: review?(smichaud)
Attachment #566795 - Flags: review?(smichaud)
Attachment #566795 - Flags: review?(bgirard)
(Reporter)

Comment 5

6 years ago
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
Attachment #566795 - Attachment is obsolete: true
Attachment #566795 - Flags: review?(smichaud)
Attachment #566795 - Flags: review?(bgirard)
Attachment #566915 - Flags: review?(bgirard)
(Reporter)

Updated

6 years ago
Attachment #566915 - Flags: review?(smichaud)
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?
(Reporter)

Comment 7

6 years ago
Created attachment 567842 [details] [diff] [review]
Trigger the quirks v4
Attachment #566915 - Attachment is obsolete: true
Attachment #566915 - Flags: review?(smichaud)
Attachment #566915 - Flags: review?(bgirard)
Attachment #567842 - Flags: review?(smichaud)
Attachment #567842 - Flags: review?(bjacob)
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?
(Reporter)

Updated

6 years ago
Attachment #567842 - Flags: review?(bjacob) → review?(bgirard)
(Assignee)

Comment 9

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

Comment 13

6 years ago
(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.
(Reporter)

Comment 14

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

Comment 15

6 years ago
Created attachment 568056 [details] [diff] [review]
Trigger the quirks v5
Attachment #567842 - Attachment is obsolete: true
Attachment #567842 - Flags: review?(smichaud)
Attachment #567842 - Flags: review?(bgirard)
Attachment #568056 - Flags: review?(smichaud)
Attachment #568056 - Flags: review?(bgirard)
(Reporter)

Comment 16

6 years ago
Created attachment 568057 [details] [diff] [review]
Trigger the quirks v5

Forgot to qrefresh
Attachment #568056 - Attachment is obsolete: true
Attachment #568056 - Flags: review?(smichaud)
Attachment #568056 - Flags: review?(bgirard)
Attachment #568057 - Flags: review?(smichaud)
Attachment #568057 - Flags: review?(bgirard)
(Assignee)

Updated

6 years ago
Attachment #568057 - Flags: review?(bgirard) → review+
Jeff, what do you mean by "the quirks", and "loading the quirks"?
Attachment #568057 - Flags: review?(smichaud) → review+
(Reporter)

Comment 18

6 years ago
(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'
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".
(Assignee)

Comment 20

6 years ago
Created attachment 568519 [details] [diff] [review]
Whitelist plugin that can use OfflineRenderer features
https://hg.mozilla.org/mozilla-central/rev/17ef2ebb8ef9
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

6 years ago
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.
Attachment #568519 - Flags: review?(jmuizelaar)
(Assignee)

Comment 23

6 years ago
Created attachment 569493 [details] [diff] [review]
Part 3: Load plugin quirks during interpose
Attachment #569493 - Flags: review?(jmuizelaar)
(Reporter)

Comment 24

6 years ago
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.
Attachment #568519 - Flags: review?(jmuizelaar) → review-
(Reporter)

Comment 25

6 years ago
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
Attachment #569493 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 26

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

Comment 27

6 years ago
Created attachment 569891 [details] [diff] [review]
Part 2: Add OfflineRenderer Support to CARenderer
Assignee: jmuizelaar → bgirard
Attachment #568519 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #569891 - Flags: review?(jmuizelaar)
(Assignee)

Comment 28

6 years ago
Created attachment 569892 [details] [diff] [review]
Part 3: Load plugin quirks during interpose
Attachment #569892 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #569493 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #569892 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

6 years ago
Attachment #569891 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 29

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0fe9c38bd1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3aa6f254b7
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?
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.
(Assignee)

Comment 32

6 years ago
Created attachment 570258 [details] [diff] [review]
Follow up fix: Remove printf
Attachment #570258 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #570258 - Flags: review? → review?(jmuizelaar)
(Reporter)

Updated

6 years ago
Attachment #570258 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 33

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ec576d7b3b
https://hg.mozilla.org/mozilla-central/rev/05ec576d7b3b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 729446
You need to log in before you can comment on or make changes to this bug.