Enable dom.compartment_per_addon by default on nightly

RESOLVED FIXED in mozilla35

Status

()

Core
XPConnect
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: billm, Assigned: Bobby Holley (On Leave Until June 11th))

Tracking

({addon-compat, dev-doc-needed})

unspecified
mozilla35
x86_64
Linux
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm3+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

This bug will track any regressions caused by enabling dom.compartment_per_addon.
Assignee: nobody → wmccloskey
tracking-e10s: --- → ?
Blocks: 997462
tracking-e10s: ? → +
Priority: -- → P1
Created attachment 8463616 [details] [diff] [review]
enable-comp-per-addon

Let's try this.
Attachment #8463616 - Flags: review?(bobbyholley)
Attachment #8463616 - Flags: review?(bobbyholley) → review+
Comment on attachment 8463616 [details] [diff] [review]
enable-comp-per-addon

Er, wait. We want this nightly-only I think.
Attachment #8463616 - Flags: review+ → review-
Created attachment 8463629 [details] [diff] [review]
enable-comp-per-addon v2
Attachment #8463616 - Attachment is obsolete: true
Attachment #8463629 - Flags: review?(bobbyholley)
Attachment #8463629 - Flags: review?(bobbyholley) → review+
billm: are there any issues blocking you from enabling dom.compartment_per_addon on Nightly? bholley r+'d your patch last month.
Flags: needinfo?(wmccloskey)
This is not green on try. We actually run all the mochitests as part of an add-on, and turning this on exposed a number of problems. I haven't had time to debug yet.
Flags: needinfo?(wmccloskey)
Blocks: 1060466
Blocks: 1063156
Taking this, per discussion with Bill.
Assignee: wmccloskey → bobbyholley
Bill said he had a try push somewhere, but I can't find it. Here's a new one: https://tbpl.mozilla.org/?tree=Try&rev=cdc30036d96d
Depends on: 1042680
Depends on: 1065452
Moving old M2 P1 bugs to M3.
tracking-e10s: + → m3+
I assume you meant the opposite dependency relationship, right Bill?
Blocks: 1066838
No longer depends on: 1066838
The relevant parts of bug 1042680 (which should really be broken up into sub-bugs) have now landed. Clearing the blocker.
No longer depends on: 1042680
Created attachment 8491730 [details] [diff] [review]
QI the pref service to the appropriate interface. v1

nsIPrefService and nsIPrefBranch are implemented by the same XPCOM object, but
getBranch lives on nsIPrefService rather than nsIPrefBranch. The bug here is
only noticeable if nobody has already QIed the per-scope-singleton object to
nsIPrefService. If we create two scopes where there previously was one, that's
more likely to be the case, and manifest itself as the bc1 orange we see on TBPL.
Attachment #8491730 - Flags: review?(wmccloskey)
Attachment #8491730 - Flags: review?(wmccloskey) → review+
I realized that the way I turned off the assertion in AddonWindowOrNull makes the function useless for our purposes. The patch in bug 1068225 will fix that.
Depends on: 1068225
Ugh, sorry this is taking so long.

It looks like the one remaining failure is one which I hadn't noticed before, because it's linux-debug only: browser/devtools/debugger/test/browser_dbg_addon-modules.js

The failure looks pretty inscrutable, so I'm spinning up a linux VM now to see if I can reproduce it locally.
Doesn't reproduce on a linux VM, or with at least some of the surrounding tests. Here's a try run with some logging:

https://tbpl.mozilla.org/?tree=Try&rev=b31915e96a4d
Created attachment 8494305 [details] [diff] [review]
Fix up unwrapDebuggerObjectGlobal to handle addon scopes. v1 r=jimb

I got r=jimb on this yesterday over IRC. We should be good to go now.
Attachment #8494305 - Flags: review+
Looks like the dt1 orange came back, even though it looks definitively fixed in comment 22. I realized that the previous try run was probably relying on the dump() statements to force stringification and trigger the exception in the case of dead object proxies.

Followup to do that: https://hg.mozilla.org/integration/mozilla-inbound/rev/97d05c73c5a6
Blocks: 1074645

Updated

4 years ago
Depends on: 1081537

Updated

4 years ago
Depends on: 1083561
So we're seeing a behavior change in our add-on because of this. It's probably our bug, I just want to understand what's going on.

We had some code we were running async, and it was working before, but now some globals aren't defined anymore.

Before, the page went away, but the globals were still there, now some are, and some aren't.

Comment 28

4 years ago
Bill, whenever you file a bug, could you please make sure the original description is meaningful and self-explanatory? When I come here, I have no idea what this is about or why it breaks my addon.
Ally, can you help Mike find his missing globals in comment 27? We should document on MDN how the add-on compartments work and how add-on developers can control the visibility of their global variables.
Flags: needinfo?(ally)
Shims aren't related to global variables, and I don't know jack about the underlying implementation of js compartments. 

billm is a much better person to ask.
Flags: needinfo?(ally) → needinfo?(wmccloskey)
In this bug, we changed the global that add-ons run it to be different than the normal chrome window. However, we've tried to emulate the old behavior so that add-ons can still see each others globals as well as global properties on the window.

We don't intend for this bug to change any addon-visible behaviors (although we do expect bugs). If something isn't working, please file a bug with a description of what you're doing and the actual and expected results. Please have it block this bug and CC me and Bobby. Thanks, and sorry for the trouble.
Flags: needinfo?(wmccloskey)

Updated

4 years ago
Depends on: 1077973

Updated

4 years ago
Depends on: 1092156
Keywords: dev-doc-needed

Updated

3 years ago
Depends on: 1162392

Updated

2 years ago
Depends on: 1265884

Updated

2 years ago
Depends on: 1271947
You need to log in before you can comment on or make changes to this bug.