1030420 - Enable dom.compartment_per_addon by default on nightly

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This bug will track any regressions caused by enabling dom.compartment_per_addon.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: enable-comp-per-addon

Let's try this.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8463616 [details] [diff] [review]
enable-comp-per-addon

Er, wait. We want this nightly-only I think.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: enable-comp-per-addon v2

Comment 4

[Chris Peterson [:cpeterson]]

billm: are there any issues blocking you from enabling dom.compartment_per_addon on Nightly? bholley r+'d your patch last month.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bobby Holley (:bholley)]

Taking this, per discussion with Bill.

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=dedc2cf3d5a2

Comment 9

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=44569a01a092

Comment 10

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=aed7e9abe325

Comment 11

[Chris Peterson [:cpeterson]]

Moving old M2 P1 bugs to M3.

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8417dc1c1a7d

Comment 13

[Bobby Holley (:bholley)]

I assume you meant the opposite dependency relationship, right Bill?

Comment 14

[Bobby Holley (:bholley)]

The relevant parts of bug 1042680 (which should really be broken up into sub-bugs) have now landed. Clearing the blocker.

Comment 15

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 16

[Bobby Holley (:bholley)]

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.

Comment 17

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=da928e020b69

Comment 18

[Bobby Holley (:bholley)]

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.

Comment 19

[Bobby Holley (:bholley)]

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

Comment 20

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c4a379180178

Comment 21

[Bobby Holley (:bholley)]

I think I have a fix:

https://tbpl.mozilla.org/?tree=Try&rev=8f82452664a6

Comment 22

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0a382eddf901

Comment 23

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 24

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c28d9772cd2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b419fc2a66bc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35dcddea2c7a

Comment 25

[Bobby Holley (:bholley)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c28d9772cd2
https://hg.mozilla.org/mozilla-central/rev/b419fc2a66bc
https://hg.mozilla.org/mozilla-central/rev/35dcddea2c7a
https://hg.mozilla.org/mozilla-central/rev/97d05c73c5a6

Comment 27

[Mike Kaply [:mkaply]]

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

[Ben Bucksch (:BenB)]

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.

Comment 29

[Chris Peterson [:cpeterson]]

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.

Comment 30

[Allison Naaktgeboren :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.

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.