Closed Bug 1445551 Opened 7 years ago Closed 7 years ago

Remove compartment-per-add-on

Categories

(Firefox :: Extension Compatibility, enhancement)

Product:
Firefox
Component:
Extension Compatibility
Version:
57 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(8 files)

Bug 1445551: Part 1a - Add uses-unsafe-cpows annotation to mochitest harness.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1445551: Part 1b - Whitelist unsafe CPOW use in existing tests.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1445551: Part 1c - Fix browser tests that attempt to use importGlobalProperties.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1445551: Part 2 - Remove multiprocessCompatible flag.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1445551: Part 3 - Remove AllowCPOWsInAddon machinery.
59 bytes, text/x-review-board-request
mccr8
: review+
Details
Bug 1445551: Part 4 - Remove compartment-per-addon.
59 bytes, text/x-review-board-request
mccr8
: review+
Details
Bug 1445551: Part 5 - Remove add-on path service.
59 bytes, text/x-review-board-request
aswan
: review+
mccr8
: review+
Details
Bug 1445551: Part 6 - Remove JSAddonId type and addonId compartment flag.
59 bytes, text/x-review-board-request
mccr8
: review+
Details 
This requires removing support for unsafe-CPOWs-per-add-on, changing the mochitest harness to support unsafe CPOWs in a whitelisted set of tests, and fixing tests that depend on being run in a Sandbox rather than directly in a browser window.
Comment on attachment 8958695 [details]
Bug 1445551: Part 3 - Remove AllowCPOWsInAddon machinery.

https://reviewboard.mozilla.org/r/227614/#review233446
Attachment #8958695 - Flags: review?(continuation) → review+
Comment on attachment 8958694 [details]
Bug 1445551: Part 2 - Remove multiprocessCompatible flag.

https://reviewboard.mozilla.org/r/227612/#review233484

\o/
There are still a handful of test harnesses that set the preference that you removed, can you remove those references as well

::: toolkit/mozapps/extensions/content/extensions.js
(Diff revision 1)
>          document.getElementById("detail-warning").textContent = gStrings.ext.formatStringFromName(
>            "details.notification.incompatible",
>            [this._addon.name, gStrings.brandShortName, gStrings.appVersion], 3
>          );
>          document.getElementById("detail-warning-link").hidden = true;
> -      } else if (this._addon.appDisabled && !this._addon.multiprocessCompatible && !ALLOW_NON_MPC) {

The definition of ALLOW_NON_MPC at the top of this file can also go
Attachment #8958694 - Flags: review?(aswan) → review+
Comment on attachment 8958697 [details]
Bug 1445551: Part 5 - Remove add-on path service.

https://reviewboard.mozilla.org/r/227618/#review233488

The toolkit/ parts look great to me, defer to mccr8 on the rest
Attachment #8958697 - Flags: review?(aswan) → review+
Comment on attachment 8958696 [details]
Bug 1445551: Part 4 - Remove compartment-per-addon.

https://reviewboard.mozilla.org/r/227616/#review233530

::: js/xpconnect/src/xpcpublic.h:111
(Diff revision 1)
>      if (IsInContentXBLScope(obj))
>          return js::GetGlobalForObjectCrossCompartment(obj);
>      return GetXBLScope(cx, obj);
>  }
>  
> -// This function is similar to GetXBLScopeOrGlobal. However, if |obj| is a
> +// This function is similar to GetXBLScopeOrGlobal.

Is this method different than GetXBLScopeOrGlobal() now? It doesn't look like it is trivially the same. Please document what the difference is.
Attachment #8958696 - Flags: review?(continuation) → review+
Comment on attachment 8958697 [details]
Bug 1445551: Part 5 - Remove add-on path service.

https://reviewboard.mozilla.org/r/227618/#review233550

r=me for the XPConnect and DOM changes. I have no idea what is going on with the devtools changes, so I guess somebody needs to look at that...
Attachment #8958697 - Flags: review?(continuation) → review+
Please add a sentence to one of the commit messages about why it is okay to remove all of this stuff...
Comment on attachment 8958698 [details]
Bug 1445551: Part 6 - Remove JSAddonId type and addonId compartment flag.

https://reviewboard.mozilla.org/r/227620/#review233552

::: js/src/jsexn.cpp
(Diff revision 1)
> -    SprintfLiteral(histogramKey, "%s %s %s %u",
> -                   addonIdChars.get(),
> -                   funname,
> -                   filename,
> -                   (reportp ? reportp->lineno : 0) );
> -    cx->runtime()->addTelemetry(JS_TELEMETRY_ADDON_EXCEPTIONS, 1, histogramKey);

JS_TELEMETRY_ADDON_EXCEPTIONS is unused now. You should remove that, from both telemetry and the jsfriendapi.h stuff around exceptions.
Attachment #8958698 - Flags: review?(continuation) → review+
Comment on attachment 8958696 [details]
Bug 1445551: Part 4 - Remove compartment-per-addon.

https://reviewboard.mozilla.org/r/227616/#review233530

> Is this method different than GetXBLScopeOrGlobal() now? It doesn't look like it is trivially the same. Please document what the difference is.

Heh. Yeah, I wasn't happy with that comment when I re-read it either, but the original comment wasn't any better. I'll try to come up with something
(In reply to Andrew McCreight [:mccr8] from comment #13)
> I have no idea what is going on with the devtools changes, so I guess somebody needs to look at that...

I was relying on aswan for that part.

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Please add a sentence to one of the commit messages about why it is okay to
> remove all of this stuff...

Sorry, I forgot to go back and add long descriptions to the second half of the commits after I finished arranging them.
Comment on attachment 8958696 [details]
Bug 1445551: Part 4 - Remove compartment-per-addon.

https://reviewboard.mozilla.org/r/227616/#review233530

> Heh. Yeah, I wasn't happy with that comment when I re-read it either, but the original comment wasn't any better. I'll try to come up with something

Hm. OK, the two functions are the same now, but GetXBLScope is just an unparsable mess, so it's not obvious. I'll just merge the two.
Comment on attachment 8958697 [details]
Bug 1445551: Part 5 - Remove add-on path service.

https://reviewboard.mozilla.org/r/227618/#review233550

Oh r=me on the devtools changes
I've started reviewing this, but can I assume you've gotten a green try run out of these patches?
Flags: needinfo?(kmaglione+bmo)
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #20)
> I've started reviewing this, but can I assume you've gotten a green try run
> out of these patches?

Mostly. Every time I do a new try run, I wind up with more devtools tests to whitelist (I guess the summary view in treeherder gives up after a certain point), but aside from those, it's green at this point.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8958691 [details]
Bug 1445551: Part 1a - Add uses-unsafe-cpows annotation to mochitest harness.

https://reviewboard.mozilla.org/r/227606/#review233622

Thanks for doing this, kmag. This makes a lot of sense - I just have one suggestion that might help improve readability (see below).

::: testing/mochitest/browser-test.js:983
(Diff revision 1)
>      // Load the tests into a testscope
>      let currentScope = this.currentTest.scope = new testScope(this, this.currentTest, this.currentTest.expected);
>      let currentTest = this.currentTest;
>  
>      // Import utils in the test scope.
> -    this.currentTest.scope.EventUtils = this.EventUtils;
> +    let scope = this.currentTest.scope.__scope;

This is a little confusing. I have a suggestion - see below where you define __scope.

::: testing/mochitest/browser-test.js:1395
(Diff revision 1)
> +  //
> +  // Otherwise, load test files directly into the testScope instance.
> +  if (aTest.usesUnsafeCPOWs) {
> +    let sandbox = this._createSandbox();
> +    Cu.permitCPOWsInScope(sandbox);
> +    this.__scope = sandbox;

I'm not super crazy about naming this \_\_scope, simply because we're already in something called a testScope that's assigned to a property called scope. The word is losing all meaning.

It seems like what we really might want to do here is just return either the sandbox or this from the constructor function here. That way, the callers don't need to know about \_\_scope - they just have... well, a testScope instance where the constructor returned the right thing.

I'm not saying that the rest of this file is a super easy read in general, but I'd like to avoid adding to the confusion if possible.

::: testing/mochitest/tests/SimpleTest/EventUtils.js:181
(Diff revision 1)
>                         ctrlKeyArg, altKeyArg, shiftKeyArg, metaKeyArg,
>                         buttonArg, relatedTargetArg);
>  
> +  // If documentURIObject exists, we're in a chrome scope, and don't
> +  // need to go through SpecialPowers.
> +  if (!window.document || window.document.documentURIObject)

We're also checking if the document doesn't exist... can we assume that we're in the chrome scope in that case?

::: testing/mochitest/tests/SimpleTest/EventUtils.js:1249
(Diff revision 1)
>      aWindow = window;
>    }
>  
> +  // If documentURIObject exists, we're in a chrome scope, so don't
> +  // bother trying to go through SpecialPowers.
> +  if (!window.document || window.document.documentURIObject) {

Same as above - if there's no document, is it safe to assume we're in a chrome scope?
Attachment #8958691 - Flags: review?(mconley) → review-
Comment on attachment 8958691 [details]
Bug 1445551: Part 1a - Add uses-unsafe-cpows annotation to mochitest harness.

https://reviewboard.mozilla.org/r/227606/#review233622

> We're also checking if the document doesn't exist... can we assume that we're in the chrome scope in that case?

In practice, the assumption seems to work. When we're loaded into a regular or chrome mochitest, we always have a normal window object with a document. In browser mochitests, things are weird, and document doesn't exist.
Comment on attachment 8958692 [details]
Bug 1445551: Part 1b - Whitelist unsafe CPOW use in existing tests.

https://reviewboard.mozilla.org/r/227608/#review233648

Seems okay, though I'm a bit confused about the changes in browser_waitForFocus.js.

::: testing/mochitest/tests/browser/browser_waitForFocus.js:20
(Diff revision 1)
>    await BrowserTestUtils.openNewForegroundTab(gBrowser);
>  
>    gURLBar.focus();
>  
>    let browser = gBrowser.selectedBrowser;
> -  await SimpleTest.promiseFocus(browser.contentWindowAsCPOW, true);
> +  await SimpleTest.promiseFocus(browser, true);

I don't super understand the changes in this file. This test is run in e10s mode, but can also run in non-e10s mode, and you've enabled CPOWs in it.

But you've also removed the explicit CPOW usage, and attempt to directly access contentWindow, which (I don't think) will work in e10s mode because shims are gone.

So the browsers must sometimes be non-remote? I guess if we're running this in non-e10s mode?

Is that why the extra-test and the browser.contentWindow checks are done? In the event that we're running in non-e10s mode? Do we even still run in that mode in automation?
Attachment #8958692 - Flags: review?(mconley) → review+
Comment on attachment 8958693 [details]
Bug 1445551: Part 1c - Fix browser tests that attempt to use importGlobalProperties.

https://reviewboard.mozilla.org/r/227610/#review233658
Attachment #8958693 - Flags: review?(mconley) → review+
Comment on attachment 8958692 [details]
Bug 1445551: Part 1b - Whitelist unsafe CPOW use in existing tests.

https://reviewboard.mozilla.org/r/227608/#review233648

> I don't super understand the changes in this file. This test is run in e10s mode, but can also run in non-e10s mode, and you've enabled CPOWs in it.
> 
> But you've also removed the explicit CPOW usage, and attempt to directly access contentWindow, which (I don't think) will work in e10s mode because shims are gone.
> 
> So the browsers must sometimes be non-remote? I guess if we're running this in non-e10s mode?
> 
> Is that why the extra-test and the browser.contentWindow checks are done? In the event that we're running in non-e10s mode? Do we even still run in that mode in automation?

Sorry, I should have commented about this.

The uses-unsafe-cpows annotation can probably removed now. It didn't occur to me to remove it after I realized it would never have worked.

So, there are two ways promiseFocus can be used, either on a browser, or on an element. The browser version works with remote or non-remote browsers. The *element* version only works with non-remote browsers, which we get in non-e10s tests. If we try to pass it a CPOW, it throws, because CPOWs are only whitelisted in the test Sandbox, not the browser window that SimpleTest is loaded into.

We *could* theoretically try to load a separate copy of SimpleTest into a CPOW-enabled Sandbox, just fixing the other CPOW-based callers is much easier, and I don't really want to give people another reason to use unsafe CPOWs.
Comment on attachment 8958692 [details]
Bug 1445551: Part 1b - Whitelist unsafe CPOW use in existing tests.

https://reviewboard.mozilla.org/r/227608/#review233648

> Sorry, I should have commented about this.
> 
> The uses-unsafe-cpows annotation can probably removed now. It didn't occur to me to remove it after I realized it would never have worked.
> 
> So, there are two ways promiseFocus can be used, either on a browser, or on an element. The browser version works with remote or non-remote browsers. The *element* version only works with non-remote browsers, which we get in non-e10s tests. If we try to pass it a CPOW, it throws, because CPOWs are only whitelisted in the test Sandbox, not the browser window that SimpleTest is loaded into.
> 
> We *could* theoretically try to load a separate copy of SimpleTest into a CPOW-enabled Sandbox, just fixing the other CPOW-based callers is much easier, and I don't really want to give people another reason to use unsafe CPOWs.

> The uses-unsafe-cpows annotation can probably removed now. It didn't occur to me to remove it after I realized it would never have worked.

Okay, sure, let's do that.

Alright, I think I understand better - you've got branching for the e10s and non-e10s cases. Would it be possible to make that more explicit with some inline comments?
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #27)
> Alright, I think I understand better - you've got branching for the e10s and
> non-e10s cases. Would it be possible to make that more explicit with some
> inline comments?

Yup.
(Sorry if I screwed up the inter-diff. There was one devtools failure that looked like it was in my base revision, and I needed to rebase to confirm.)
In case anyone is curious, I had to whitelist 287 devtools tests for unsafe CPOW usage, and only 20 tests outside of devtools...
Comment on attachment 8958691 [details]
Bug 1445551: Part 1a - Add uses-unsafe-cpows annotation to mochitest harness.

https://reviewboard.mozilla.org/r/227606/#review233672

Yeah, this reads much better, thanks!
Attachment #8958691 - Flags: review?(mconley) → review+
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #38)
> In case anyone is curious, I had to whitelist 287 devtools tests for unsafe
> CPOW usage, and only 20 tests outside of devtools...

Thanks for the note.  I have filed several DevTools bugs about this under bug 1445804.  (I thought we had handled a lot of this already, but looks like there's clearly work remaining!)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf17754e8ab3b698ef04ec8f3d7b2f9e79cb45df
Bug 1445551: Part 1a - Add uses-unsafe-cpows annotation to mochitest harness. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/727dba5569a7569d38492051d62db4d94ba1fe0b
Bug 1445551: Part 1b - Whitelist unsafe CPOW use in existing tests. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3631cb49bef9426ba19c20c43f488eecdc7a9d6
Bug 1445551: Part 1c - Fix browser tests that attempt to use importGlobalProperties. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/49a2d8efed226311f1f18d095d38ea849757c521
Bug 1445551: Part 2 - Remove multiprocessCompatible flag. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/53ace32fdc84384f276614a0a26ec70c7293974f
Bug 1445551: Part 3 - Remove AllowCPOWsInAddon machinery. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3fe1c247c85d5443855233aca37b77dc5c5248
Bug 1445551: Part 4 - Remove compartment-per-addon. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/f52c1d6859be45887a2a868b31a4b46fc64e89a3
Bug 1445551: Part 5 - Remove add-on path service. r=aswan,mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/7585e7acb34b63d21504aefb24a4ba43f90fa169
Bug 1445551: Part 6 - Remove JSAddonId type and addonId compartment flag. r=mccr8
Comment on attachment 8958697 [details]
Bug 1445551: Part 5 - Remove add-on path service.

https://reviewboard.mozilla.org/r/227618/#review234052

::: devtools/server/actors/addon.js:220
(Diff revision 2)
>      } catch (e) {
>        // ignore
>      }
>  
>      if (global instanceof Ci.nsIDOMWindow) {
> -      return mapURIToAddonID(global.document.documentURIObject) == this.id;
> +      return global.document.nodePrincipal.addonId;

Shouldn't it be
```
      return global.document.nodePrincipal.addonId == this.id;
```
?
Comment on attachment 8958697 [details]
Bug 1445551: Part 5 - Remove add-on path service.

https://reviewboard.mozilla.org/r/227618/#review234052

> Shouldn't it be
> ```
>       return global.document.nodePrincipal.addonId == this.id;
> ```
> ?

Yes. Thanks. You'd think a test would have failed...
https://hg.mozilla.org/integration/mozilla-inbound/rev/d608e536cf3026249b1ae6008282ae75e4e74dda
Bug 1445551: Follow-up: Fix typo. r=emk DONTBUILD
https://hg.mozilla.org/mozilla-central/rev/d608e536cf30
Perf improvements win! \o/

== Change summary for alert #12161 (as of Wed, 14 Mar 2018 22:05:21 GMT) ==

Improvements:

  3%  JS linux64 opt stylo     99,945,979.34 -> 96,713,433.25
  3%  JS linux64-qr opt stylo  98,389,775.83 -> 95,358,438.00
  3%  JS linux64-stylo-sequential opt stylo-sequential99,811,818.75 -> 97,247,137.80
  2%  JS osx-10-10 opt stylo   106,650,944.23 -> 104,178,056.24
  2%  JS windows10-64 pgo stylo111,137,364.63 -> 108,578,415.57
  2%  JS windows10-64 opt stylo110,815,647.34 -> 108,496,762.98
  2%  JS windows7-32 pgo stylo 83,242,161.99 -> 81,566,488.53

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12161
I've been seeing this warning in the console:
[15588, Main Thread] WARNING: No CID found when attempting to map contract ID: file /home/icecold/mozilla-central/xpcom/components/nsComponentManager.cpp, line 508

The contract ID seems to be:
@mozilla.org/addon-path-service;1

It's still referenced in nsToolkitCompsModule.cpp/.h
Flags: needinfo?(kmaglione+bmo)
(In reply to Valentin Gosu [:valentin] from comment #48)
> I've been seeing this warning in the console:
> [15588, Main Thread] WARNING: No CID found when attempting to map contract
> ID: file
> /home/icecold/mozilla-central/xpcom/components/nsComponentManager.cpp, line
> 508
> 
> The contract ID seems to be:
> @mozilla.org/addon-path-service;1
> 
> It's still referenced in nsToolkitCompsModule.cpp/.h

Oops. Thanks.
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac9277c6aa998205da26e03e7facc7c1e9eca5b
Bug 1445551: Follow-up: Remove stray add-on path service module references. r=trivial DONTBUILD
https://hg.mozilla.org/mozilla-central/rev/3ac9277c6aa9
Blocks: 1469217
Blocks: 1404885
Blocks: 1616881
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: