Closed Bug 596762 Opened 9 years ago Closed 9 years ago

allow specification of differing IPC preferences for each architecture in a universal binary

Categories

(Core :: Plug-ins, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: jaas, Assigned: sgreenlay)

References

Details

Attachments

(1 file, 14 obsolete files)

19.61 KB, patch
Details | Diff | Splinter Review
Right now, on Mac OS X, our default preferences for OOPP are to white-list Flash and Java and otherwise run plugins in-process. This is what we want for the i386 case but it isn't correct for the x86_64 case where we want to have OOPP enabled for all plugins by default. We need to design and implement a system that will allow differing IPC preferences for each architecture in a universal binary.

Note that we don't want to be hard-code this stuff because it needs to be user-configurable.
blocking2.0: --- → betaN+
For reference, the default prefs are specified here:

http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js

The code that interprets these prefs is in the function "nsNPAPIPlugin::RunPluginOOP".
Scott and I have decided that the best thing to do is probably to add architecture suffixes to the preferences on Mac OS X.

Rather than "dom.ipc.plugins.enabled" we'll have two prefs, one suffixed with ".i386" and one suffixed with ".x86_64". This suffix system will apply to whitelist preferences as well.
Blocks: 586584
> Rather than "dom.ipc.plugins.enabled" we'll have two prefs, one suffixed with
> ".i386" and one suffixed with ".x86_64". This suffix system will apply to
> whitelist preferences as well.

Right, I think that's the best way. We can have "dom.ipc.plugins.enabled.x86_64" default to true and white list plug-in individually for i386.
Attachment #475949 - Flags: review?(joshmoz)
Attachment #475949 - Attachment is patch: true
Attachment #475949 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 475949 [details] [diff] [review]
Architecture Specific Plug-in Preferences

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// x86_64 ipc preferences
>+pref("dom.ipc.plugins.enabled.x86_64", true);
>+pref("dom.ipc.plugins.enabled.x86_64.flash player.plugin", true);
>+pref("dom.ipc.plugins.enabled.x86_64.javaplugin2_npapi.plugin", true);

You don't need to white list anything for x86_64 because the default is already true.

>diff --git a/modules/plugin/base/src/nsNPAPIPlugin.cpp b/modules/plugin/base/src/nsNPAPIPlugin.cpp

>+#ifdef XP_MACOSX
>+    #if defined(__i386__)
>+      nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.i386.");
>+    #elif defined(__x86_64__)
>+      nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.x86_64.");
>+    #endif
>+#else
>   nsCAutoString prefGroupKey("dom.ipc.plugins.enabled.");
>+#endif

Style nits - don't indent macros, leave the indentation of the code itself normal (2-space).

>+#ifdef XP_MACOSX
>+  #if defined(__i386__)
>+    prefs->GetBoolPref("dom.ipc.plugins.enabled.i386", &oopPluginsEnabled);
>+  #elif defined(__x86_64__)
>+    prefs->GetBoolPref("dom.ipc.plugins.enabled.x86_64", &oopPluginsEnabled);
>+  #endif
>+#else
>     prefs->GetBoolPref("dom.ipc.plugins.enabled", &oopPluginsEnabled);
>+#endif

Can we store these strings as static literal C strings once so we don't have to write out the strings multiple times in this function?
Attachment #475949 - Flags: review?(joshmoz) → review-
Attachment #475949 - Attachment is obsolete: true
Attachment #475983 - Flags: review?(joshmoz)
Comment on attachment 475983 [details] [diff] [review]
Architecture Specific Plug-in Preferences (v1.1)

for some reason this seems to be breaking opt builds, so I am removing review
Attachment #475983 - Flags: review?(joshmoz)
Added ppc option (will be removed once releng removes ppc builds)
Attachment #475983 - Attachment is obsolete: true
Attachment #475998 - Flags: review?(joshmoz)
bumping to a beta 7 blocker so that we have proper prefs for 64-bit OOPP
blocking2.0: betaN+ → beta7+
Attachment #475998 - Flags: review?(joshmoz) → review+
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/080a38bd09c5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
re-opened for review of ipc pref reftest fixes
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #475998 - Attachment is obsolete: true
Attachment #476405 - Flags: review?(joshmoz)
Attachment #476405 - Flags: review?(joshmoz)
Currently running on try 3f6bd40f6e4c
Attachment #476405 - Attachment is obsolete: true
Attachment #476439 - Flags: review?(joshmoz)
Comment on attachment 476439 [details] [diff] [review]
Architecture Specific Plug-in Preferences (v1.4)

If this passes tests I'm fine with it. Thanks.
Attachment #476439 - Flags: review?(joshmoz) → review+
Attachment #476947 - Flags: review?(joshmoz)
Attachment #476439 - Attachment is obsolete: true
Comment on attachment 476947 [details] [diff] [review]
Architecture Specific Plug-in Preferences (v1.5)

Patch is missing changes to "browser/app/profile/firefox.js".
Attachment #476947 - Flags: review?(joshmoz) → review-
Attachment #476947 - Attachment is obsolete: true
Attachment #476963 - Flags: review?(joshmoz)
Attachment #476963 - Flags: review?(joshmoz) → review+
Pushed to mozilla-central:

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

Also pushed a patch to fix test line endings so the patch applies cleanly:

http://hg.mozilla.org/mozilla-central/rev/011168d1877d
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Backed out again. Patch needs to have a full, 100% clean try server run with the final revision before it is ready for check-in.

http://hg.mozilla.org/mozilla-central/rev/84a05bdabec5
http://hg.mozilla.org/mozilla-central/rev/d1a024525bda
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 476963 [details] [diff] [review]
Architecture Specific Plug-in Preferences (v1.6)

+        if (navigator.platform == "Mac") {
+            if (navigator.platform == "i386") {

I should have noticed in review that this isn't possible.

+            pref = prefservice.getBoolPref("dom.ipc.plugins.enabled")

This needs a semicolon.

I'm not sure how to deal with the architecture detection here.
Attachment #476963 - Flags: review+ → review-
Attached patch fix v1.7 (obsolete) — Splinter Review
This patch applies cleanly against current trunk code and fixes some of the known issues. It isn't done though, we need to find proper architecture checks which I've marked with TODO_* text.
Attachment #476963 - Attachment is obsolete: true
Attached patch fix v1.8 (obsolete) — Splinter Review
Lets try to use nsIXULRuntime's "XPCOMABI" if we can. I haven't actually run this code, it still needs to be tested.
Attachment #477009 - Attachment is obsolete: true
Josh: can we get an ETA here? Also, does this block making the Universal Binary for 64 and 32 bit builds? If so, can you mark that?
Attached patch fix v1.9 (obsolete) — Splinter Review
Attachment #477024 - Attachment is obsolete: true
I'm testing my latest patch right now, hopefully done today.
Comment on attachment 477505 [details] [diff] [review]
fix v1.9

This seems to pass on try server now.
Attachment #477505 - Flags: review?(sgreenlay)
Comment on attachment 477505 [details] [diff] [review]
fix v1.9

Looks good to me!
Attachment #477505 - Flags: review?(sgreenlay) → review+
pushed latest patch to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/401db5db054a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
backed out again, new orange that I didn't see on try server

http://hg.mozilla.org/mozilla-central/rev/7621ea7aae7e
http://hg.mozilla.org/mozilla-central/rev/26a166fe9007
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Lets not block beta 7 on this. Only the Flash and Java plugins will work, plus any other 64-bit plugins, but Silverlight wouldn't work anyway. If we do have a fix that'll stick for this in time we should take it.
blocking2.0: beta7+ → beta8+
Attached patch fix v2.0 (obsolete) — Splinter Review
Disable the problematic tests. These are problematic on other platforms too and fixing them should not hold up general testing of OOPP on Mac OS X.
Attachment #477505 - Attachment is obsolete: true
I've changed my mind about blocking beta 7, I think this needs to block. Running all plugins out of process by default on x86_64 is a major feature and we need it turned on for feature freeze. We also need to be testing OOPP on Mac OS X ASAP, which we are currently not doing. The patch here will cause us to run the test plugin out of process on Mac OS X 10.6.
blocking2.0: beta8+ → beta7+
Attached patch fix v2.1 (obsolete) — Splinter Review
Attachment #478185 - Attachment is obsolete: true
pushed to mozilla-central again, no new orange on tryserver

http://hg.mozilla.org/mozilla-central/rev/d52c4d088273
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
backed out again, ugh

http://hg.mozilla.org/mozilla-central/rev/4368a2d55d13

This is exposing a bug that causes a lot of intermittent orange.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch fix v3.0 (obsolete) — Splinter Review
It's really the included fix for bug 586584 (run test plugin OOP on for x86_64 Mac OS X) that is causing intermittent orange, not the fix for this bug. I think we can and should fix this bug now, for stability reasons, and put the OOPP testing off until we have a solution for the intermittent orange.

This new patch adds per-architecture prefs and puts all plugins out of process by default for x86_64 hosts but it keeps the test plugin in-process (blacklists test.plugin) to avoid tinderbox intermittent orange.
Attachment #478204 - Attachment is obsolete: true
Attachment #478711 - Flags: review?(sgreenlay)
Shouldn't --setpref=dom.ipc.plugins.enabled.ARCH.test.plugin=true in testsuite-targets.mk be set to false, or are you just disabling the tests?
Attached patch fix v3.1 (obsolete) — Splinter Review
Attachment #478711 - Attachment is obsolete: true
Attachment #478725 - Flags: review?(sgreenlay)
Attachment #478711 - Flags: review?(sgreenlay)
(In reply to comment #37)
> Shouldn't --setpref=dom.ipc.plugins.enabled.ARCH.test.plugin=true in
> testsuite-targets.mk be set to false, or are you just disabling the tests?

Do the tinderboxes actually run "mochitest-ipcplugins"? The point of that target is to explicitly run with the test plugin out of process and so long as the tinderboxes don't actually do that we should allow people to do it locally.
The tinderboxes run a test suite called mochitest-ipcplugins, but not via that Makefile target (and they actually run it with the pref inverted nowadays, to test the in-process plugin code.)
Comment on attachment 478725 [details] [diff] [review]
fix v3.1

In that case, this patch looks good (providing it passed the tryserver without issues).
Attachment #478725 - Flags: review?(sgreenlay) → review+
Attached patch fix v3.2Splinter Review
Attachment #478725 - Attachment is obsolete: true
pushed v3.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/e0d1f460215c
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Blocks: 600378
You need to log in before you can comment on or make changes to this bug.