Closed Bug 1443964 Opened 6 years ago Closed 6 years ago

Remove most RemoteAddonParent/Child shims

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
firefox60 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.
59 bytes, text/x-review-board-request
mconley
: review+
Honza
: review+
Details
Bug 1443964: Part 2 - Remove most add-on compatibility shims.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1443964: Part 3 - Remove no-cpows-in-tests rule.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
I'm going to leave the tabbrowser.addProgressListener shim for the moment, since fixing the browser_google_behavior.js test that relies on it is non-trivial.
Blocks: 1443983
Comment on attachment 8957019 [details]
Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.

https://reviewboard.mozilla.org/r/225980/#review232052

Tentative r+ - though you might want to get someone from DevTools to double-check the jsonview change, since I'm really not certain about things there.

::: devtools/client/jsonview/test/head.js:74
(Diff revision 1)
>    getFrameScript();
>    let rootDir = getRootDirectory(gTestPath);
>  
>    // Catch RequireJS errors (usually timeouts)
>    let error = tabLoaded.then(() => new Promise((resolve, reject) => {
> +    if (content) {

I'm not super familiar with this test, nor how this part (or most parts) of DevTools works... will this ever be true?

::: dom/html/test/browser_refresh_wyciwyg_url.js:22
(Diff revision 1)
>      aTab = BrowserTestUtils.addTab(gBrowser, testURL);
>      aBrowser = gBrowser.getBrowserForTab(aTab);
>      BrowserTestUtils.browserLoaded(aBrowser).then(() => {
> -        is(aBrowser.contentDocument.URL, testURL, "Make sure we start at the correct URL");
> +        is(aBrowser.currentURI.spec, testURL, "Make sure we start at the correct URL");
>  
> +        ContentTask.spawn(aBrowser, null, () => {

In the interests of unblocking ckerschb, let's take this - but this test should probably just be rewritten using add_task to make this whole thing a bit clearer.

Ideally, we could then use BrowserTestUtils.synthesizeMouseAtCenter for the clicks, instead of relying on ContentTask.spawn scripts.
Attachment #8957019 - Flags: review?(mconley) → review+
Comment on attachment 8957020 [details]
Bug 1443964: Part 2 - Remove most add-on compatibility shims.

https://reviewboard.mozilla.org/r/225982/#review232054

Another tentative r+ - the Sandbox code should get approval from someone else though - I can't really say with certainty that I understand what that change implies.

::: js/xpconnect/src/Sandbox.cpp:1147
(Diff revision 2)
> -    } else if (JSObject* obj = JS::CurrentGlobalOrNull(cx)) {
> +    } else if (principal == nsXPConnect::SystemPrincipal()) {
> +        if (JSObject* obj = JS::CurrentGlobalOrNull(cx)) {
> -        if (JSAddonId* id = JS::AddonIdOfObject(obj))
> +            if (JSAddonId* id = JS::AddonIdOfObject(obj))
> -            addonId = id;
> +                addonId = id;
> -    }
> +        }
> +    }

You might want somebody who's more familiar with this part of the codebase to review this part. I'm reluctant to sign-off on changes like this without fully understanding the implications.
Attachment #8957020 - Flags: review?(mconley) → review+
Comment on attachment 8957020 [details]
Bug 1443964: Part 2 - Remove most add-on compatibility shims.

https://reviewboard.mozilla.org/r/225982/#review232054

> You might want somebody who's more familiar with this part of the codebase to review this part. I'm reluctant to sign-off on changes like this without fully understanding the implications.

I'd normally ask for review from an XPConnect peer, but this is also essentially dead code that's only used by a few tests, and is going to be removed once we get rid of the allowCPOWSInAddon stuff.

Essentially, the problem is this:

When we create a sandbox, we try to propagate the addonId of the caller scope to that sandbox. When a scope has an add-on ID, and that add-on has shims enabled, we enable shims for the sandbox. But after removing shims, we have two problems:

1) When we create a sandbox through shims, the sandbox is actually created in the RemoteAddonsParent scope, or in a remote process. Either way, there's no add-on ID for it to inherit in the caller scope, so it doesn't.

2) When we create a scope with interpositions, we assert that that scope has the system principal.

What this adds up to is that when a scope with interpositions enabled tries to create a content-privileged sandbox, and the creation doesn't happen through the Cu.Sandbox shim, it mistakenly inherits the caller's addonId and interpositions-enabled flags, and asserts.

This change is just a stop-gap to prevent those sandboxes from inheriting an addonId (the same way they wouldn't if there were shims enabled) until we can remove this code entirely. Which should hopefully be soon.
Comment on attachment 8957019 [details]
Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.

https://reviewboard.mozilla.org/r/225980/#review232052

> I'm not super familiar with this test, nor how this part (or most parts) of DevTools works... will this ever be true?

It will be true when e10s is disabled.

I'll admit there was a certain amount of "if you're using shims like this at this point, if it breaks, it's your own fault" in my thinking...

> In the interests of unblocking ckerschb, let's take this - but this test should probably just be rewritten using add_task to make this whole thing a bit clearer.
> 
> Ideally, we could then use BrowserTestUtils.synthesizeMouseAtCenter for the clicks, instead of relying on ContentTask.spawn scripts.

That would have been my preference, but I was trying to preserve the original behavior where a reference to the original button node is stored and then clicked again after the document is replaced because of the document.write() call. I'm not sure how important that behavior is, so I don't really want to change it.
Attachment #8957019 - Flags: review?(jryans)
Comment on attachment 8957019 [details]
Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.

https://reviewboard.mozilla.org/r/225980/#review232052

> It will be true when e10s is disabled.
> 
> I'll admit there was a certain amount of "if you're using shims like this at this point, if it breaks, it's your own fault" in my thinking...

Hmm, I guess this means errors here will now be ignored when running these tests in e10s mode?  I think we should check with Honza who owns this tool to see if that's acceptable.  I am not sure how critical this part of the test framework is considered to be.
Attachment #8957019 - Flags: review?(jryans) → review?(odvarko)
Comment on attachment 8957019 [details]
Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.

https://reviewboard.mozilla.org/r/225980/#review232052

> Hmm, I guess this means errors here will now be ignored when running these tests in e10s mode?  I think we should check with Honza who owns this tool to see if that's acceptable.  I am not sure how critical this part of the test framework is considered to be.

We can always replace it with `gBrowser.contentWindowAsCPOW` as a short-term work-around, but I think using a CPOW for this is a terrible idea...
Comment on attachment 8957048 [details]
Bug 1443964: Part 3 - Remove no-cpows-in-tests rule.

https://reviewboard.mozilla.org/r/226006/#review232136

Cancelling review request, as discussed in IRC.
Attachment #8957048 - Flags: review?(mconley)
In an, in retrospect, unsurprising turn events, removing shims that send sync messages to the parent process turns out to affect timings enough to cause new intermittent failures in a bunch of tests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cca613b59b3c3711a9d1603b1dc90981f8395586
Bug 1443964: Follow-up: Another attempt at fixing intermittent timing issue after removal of sync messaging from shims. r=bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae9c864cc76fba9865a293490ac37e33deb753a
Bug 1443964: Follow-up: Attempt 3 to fix intermittent timing issues, this time with rr chaos mode.
Comment on attachment 8957019 [details]
Bug 1443964: Part 1 - Fix most tests that rely on add-on shims.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> We can always replace it with `gBrowser.contentWindowAsCPOW` as a short-term
> work-around, but I think using a CPOW for this is a terrible idea...
Sorry for the delay and yes agreed.

Honza
Attachment #8957019 - Flags: review?(odvarko) → review+
Comment on attachment 8957048 [details]
Bug 1443964: Part 3 - Remove no-cpows-in-tests rule.

Re-opening this for review since I think the uses-unsafe-cpows test annotation makes this obsolete.
Attachment #8957048 - Flags: review?(mconley)
Comment on attachment 8957048 [details]
Bug 1443964: Part 3 - Remove no-cpows-in-tests rule.

https://reviewboard.mozilla.org/r/226006/#review234190

Yeah, with bug 1445551 landed, this makes a lot more sense. I haven't done an exhaustive cross-check to see if there are any remaining eslint-disable-next-line mozilla/no-cpows-in-tests annotations leftover after this, but I suppose ESLint will let us know.
Attachment #8957048 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #22)
> Yeah, with bug 1445551 landed, this makes a lot more sense. I haven't done
> an exhaustive cross-check to see if there are any remaining
> eslint-disable-next-line mozilla/no-cpows-in-tests annotations leftover
> after this, but I suppose ESLint will let us know.

One is landing over in bug 1445991.
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f745d7c4e0a7b0c84be6ede6a20311304042c5
Bug 1443964: Follow-up: Remove no-cpows-in-test rule from files listed in .hgignore. r=bustage
Target Milestone: Firefox 60 → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: