Closed Bug 1233497 Opened 5 years ago Closed 5 years ago

Make unsafe CPOWs from non-addon browser code throw by default

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s m8+ ---
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(19 files)

58 bytes, text/x-review-board-request
mconley
: review+
Details
9.79 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
Yoric
: review+
Details
58 bytes, text/x-review-board-request
Felipe
: review+
Details
58 bytes, text/x-review-board-request
mikedeboer
: review+
Details
58 bytes, text/x-review-board-request
mconley
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
9.22 KB, patch
mrbkap
: review+
mconley
: feedback+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
florian
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
billm
: review+
Details
58 bytes, text/x-review-board-request
mak
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
jryans
: review+
Details
58 bytes, text/x-review-board-request
ato
: review+
Details
58 bytes, text/x-review-board-request
mrbkap
: review+
Details
20.88 KB, application/zip
Details
The pref to do this was added in bug 1215167.

So we just need to set it to true in firefox.js, and bob's your uncle.

We could also, alternatively, set it true by default in the toolkit prefs file. I'm not sure it matters, seeing as Firefox is probably the only XUL app using CPOWs right now.
I'm seeing a ton of orange for this patch. Looking at the failures, it looks like there are a bunch of unsafe CPOW throws from within Mochitest code.

So is the pref accidentally shutting off CPOWs for add-ons as well? I would think that Mochitest would still be able to use unsafe CPOWs.
Flags: needinfo?(mrbkap)
I think Bill was noticing something like this a while back where mochitest code was seemingly not being treated like addons.
Flags: needinfo?(mrbkap) → needinfo?(wmccloskey)
Attached patch patchSplinter Review
The patch in bug 1233497 was not tested, so not surprisingly it was totally broken. There were two problems:

1. All CPOWs live in the junk scope, so the current compartment check will always return the junk scope, which is not part of an add-on. This really sucks, and I couldn't think of a good way to fix it. I added this stack walking thing, which really is awful. I'd be interested in suggestions for better ideas.

2. The code in RemoteAddonsParent.jsm still needs to use CPOWs even though it's not part of an add-on. I added a whitelisting thing to get around that.

I also looked further into which test suites actually get shims. Currently:

- browser chrome tests are loaded via loadSubScript from the mochikit extension, so they're treated as an add-on.
- chrome tests are loaded as separate XUL files from chrome://mochitest/ URLs. This chrome package is not considered part of an add-on, so the test also isn't.
- I didn't look closely at plain mochitests, but I wouldn't expect them to be part of an add-on since they're served over HTTP.

This is mostly fine since we don't use chrome tests much for e10s. However, it does mean that we need to do something special for test_cpows.xul, which is a chrome test. I disabled the pref during that test (except during the part where we're testing the pref).

I'm not sure if this fixes everything. Here's a new try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=394f5f7b5186
Assignee: nobody → wmccloskey
Attachment #8699576 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Attachment #8702409 - Flags: review?(mrbkap)
The try push seems to be showing a legitimate problem where we're calling contentDocumentAsCPOW from browser code. I haven't looked at it closely, but a DXR search shows a few callsites that we'll need to fix.
I'll give this back to Mike to finish eliminating CPOWs.
Assignee: wmccloskey → mconley
Flags: needinfo?(mconley)
At least one of the violations I'm seeing in this stack is due to BrowserTestUtils usage:


19:11:58     INFO -  5 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_close.js | Uncaught exception - at resource://testing-common/BrowserTestUtils.jsm:428 - Error: unsafe CPOW usage forbidden
19:11:58     INFO -  Stack trace:
19:11:58     INFO -      waitForEvent/<@resource://testing-common/BrowserTestUtils.jsm:428:7
19:11:58     INFO -      waitForEvent@resource://testing-common/BrowserTestUtils.jsm:427:1
19:11:58     INFO -      dummyTabTask@chrome://mochitests/content/browser/browser/base/content/test/alerts/browser_notification_close.js:17:11
19:11:58     INFO -      test_notificationClose@chrome://mochitests/content/browser/browser/base/content/test/alerts/browser_notification_close.js:11:9
19:11:58     INFO -      test_notificationClose@chrome://mochitests/content/browser/browser/base/content/test/alerts/browser_notification_close.js:11:9
19:11:58     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:756:9
19:11:58     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:676:7
19:11:58     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
19:11:58     INFO -      Tester_execTest@chrome://mochikit/content/browser-test.js:756:9
19:11:58     INFO -      Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:676:7
19:11:58     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59

I can try to eliminate the other usages, but we're definitely going to need to be able to use CPOWs inside that BrowserTestUtils module for mochitests. Is that at all possible, or at we at the point where we have to forbid CPOW usage from within that module?
Flags: needinfo?(wmccloskey)
We should be able to call permitCPOWsInScope(this) from BrowserTestUtils. Unfortunately there's also a ton of devtools crap in there. It looks like it's passing a CPOW as the result of a promise, and our promise code is doing some property operations on the result. I'm not sure what to do about that.
Flags: needinfo?(wmccloskey)
Would JS promises (bug 911216) have any impact on that? I believe that's reasonably close to landing (though the meat of the implementation isn't up for review yet).
Attachment #8699576 - Attachment description: MozReview Request: Bug 1233497 - Make unsafe CPOWs from non-addon browser code throw by default. r?felipe → MozReview Request: Fix unsafe CPOW restriction
Attachment #8699576 - Attachment is obsolete: false
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28285/diff/1-2/
Comment on attachment 8702409 [details] [diff] [review]
patch

Review of attachment 8702409 [details] [diff] [review]:
-----------------------------------------------------------------

I can't think of a better way to do this. :-/

::: js/xpconnect/idl/xpccomponents.idl
@@ +450,5 @@
> +     * add-ons can use CPOWs. This function allows a non-addon scope
> +     * to opt into CPOWs. It's necessary for the implementation of
> +     * RemoteAddonsParent.jsm.
> +     */
> +    void permitCPOWsInScope(in jsval obj);

Need to bump the IID.
Attachment #8702409 - Flags: review?(mrbkap) → review+
I just talked to Bobby about this and he suggested using GetIncumbentGlobal. I will try that.
Alright - halting work on fixing the tests until we can try the GetIncumbentGlobal patch.
Flags: needinfo?(wmccloskey)
I don't think that should affect the tests at all.
Flags: needinfo?(wmccloskey)
Although, I'm curious why we need some of these fixes. Why are the changes to the translation tests needed, for example?
(In reply to Bill McCloskey (:billm) from comment #23)
> Although, I'm curious why we need some of these fixes. Why are the changes
> to the translation tests needed, for example?

Those tests were passing a contentDocument CPOW to TranslationDocument and Bing/YandexTranslator JSMs for processing and manipulation, and this was throwing.

(In reply to Bill McCloskey (:billm) from comment #22)
> I don't think that should affect the tests at all.

Ah - in at least one case, I was using Cu.permitCPOWsInScope. Can I assume then that GetIncumbentGlobal is just an implementation detail that permitCPOWsInScope will use, and the API will stay the same?
Flags: needinfo?(mconley) → needinfo?(wmccloskey)
> Those tests were passing a contentDocument CPOW to TranslationDocument and
> Bing/YandexTranslator JSMs for processing and manipulation, and this was throwing.

Ah, I see, that makes sense.

> Ah - in at least one case, I was using Cu.permitCPOWsInScope. Can I assume then that
> GetIncumbentGlobal is just an implementation detail that permitCPOWsInScope will use, and the
> API will stay the same?

permitCPOWsInScope won't be affected.

It's just a question of how we determine what compartment you're in when you use a CPOW. I used to do a stack walk, but it looks like GetIncumbentGlobal is a slightly nicer approach that basically does the same thing.
Flags: needinfo?(wmccloskey)
Depends on: 1237025
Mike, does this patch work as well for you as the last one?
Attachment #8704367 - Flags: feedback?(mconley)
Comment on attachment 8704367 [details] [diff] [review]
GetIncumbentGlobal patch

This seems to work, yes!
Attachment #8704367 - Flags: feedback?(mconley) → feedback+
A ,pre recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=990869e5f112

Getting closer. Just need to get the devtools tests to pass.
Attachment #8703865 - Flags: review?(dteller)
Comment on attachment 8703865 [details]
MozReview Request: Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r?Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29489/diff/1-2/
Comment on attachment 8703866 [details]
MozReview Request: Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29491/diff/1-2/
Attachment #8703866 - Flags: review?(felipc)
Attachment #8703867 - Flags: review?(mdeboer)
Comment on attachment 8703867 [details]
MozReview Request: Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29493/diff/1-2/
Comment on attachment 8703868 [details]
MozReview Request: Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r?Enn

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29495/diff/1-2/
Attachment #8703868 - Flags: review?(enndeakin)
Comment on attachment 8703869 [details]
MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29497/diff/1-2/
Attachment #8703869 - Flags: review?(wmccloskey)
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28285/diff/2-3/
Attachment #8699576 - Attachment description: MozReview Request: Fix unsafe CPOW restriction → MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin
Attachment #8699576 - Flags: review?(enndeakin)
Comment on attachment 8703864 [details]
MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29487/diff/1-2/
Attachment #8703864 - Attachment description: MozReview Request: Bug 1233497 - Remove a CPOW from browser/components/places/tests/browser/head.js r?mak → MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe
Attachment #8703864 - Flags: review?(felipc)
Comment on attachment 8704367 [details] [diff] [review]
GetIncumbentGlobal patch

Review of attachment 8704367 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/idl/xpccomponents.idl
@@ +450,5 @@
> +     * add-ons can use CPOWs. This function allows a non-addon scope
> +     * to opt into CPOWs. It's necessary for the implementation of
> +     * RemoteAddonsParent.jsm.
> +     */
> +    void permitCPOWsInScope(in jsval obj);

Still need to bump this IID.
Attachment #8704367 - Flags: review?(mrbkap) → review+
Attachment #8703864 - Flags: review?(felipc) → review+
Comment on attachment 8703864 [details]
MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe

https://reviewboard.mozilla.org/r/29487/#review26643
Comment on attachment 8703866 [details]
MozReview Request: Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r?felipe

https://reviewboard.mozilla.org/r/29491/#review26645
Attachment #8703866 - Flags: review?(felipc) → review+
Attachment #8703865 - Flags: review?(dteller) → review+
Comment on attachment 8703865 [details]
MozReview Request: Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r?Yoric

https://reviewboard.mozilla.org/r/29489/#review26663

Looks good to me, thanks.
Comment on attachment 8703867 [details]
MozReview Request: Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r?mikedeboer

https://reviewboard.mozilla.org/r/29493/#review26671

Must be fun to through all these different areas of Fx code!

::: browser/components/translation/test/browser_translation_bing.js:92
(Diff revision 2)
> -  Assert.ok(client._serviceUnavailable, "Service should be detected unavailable.");
> +    ok(client._serviceUnavailable, "Service should be detected unavailable.");

I'm not particularly fond of s/Assert.ok/ok/ but not blocking the r+ on that, of course.
Attachment #8703867 - Flags: review?(mdeboer) → review+
Attachment #8704827 - Flags: review?(florian) → review+
Comment on attachment 8704827 [details]
MozReview Request: Bug 1233497 - Permit CPOWs in pageInfo.js until bug 1237025 is fixed. r?florian

https://reviewboard.mozilla.org/r/29763/#review26675
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Add the same comment with the reference to bug 1236991 for browser_input_file_tooltips.js as well.
Attachment #8699576 - Flags: review?(enndeakin) → review+
Attachment #8703868 - Flags: review?(enndeakin) → review+
Comment on attachment 8703865 [details]
MozReview Request: Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r?Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29489/diff/2-3/
Comment on attachment 8703866 [details]
MozReview Request: Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29491/diff/2-3/
Comment on attachment 8703867 [details]
MozReview Request: Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29493/diff/2-3/
Comment on attachment 8703868 [details]
MozReview Request: Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r?Enn

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29495/diff/2-3/
Attachment #8703868 - Flags: review+
Comment on attachment 8703869 [details]
MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29497/diff/2-3/
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28285/diff/3-4/
Attachment #8699576 - Flags: review+
Comment on attachment 8703864 [details]
MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29487/diff/2-3/
Comment on attachment 8704827 [details]
MozReview Request: Bug 1233497 - Permit CPOWs in pageInfo.js until bug 1237025 is fixed. r?florian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29763/diff/1-2/
Comment on attachment 8704828 [details]
MozReview Request: Bug 1233497 - Stop using TabState.flushAsync in session store tests. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29765/diff/1-2/
Comment on attachment 8704829 [details]
MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29767/diff/1-2/
Comment on attachment 8703869 [details]
MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r?billm

https://reviewboard.mozilla.org/r/29497/#review26895

::: testing/mochitest/tests/browser/browser_waitForFocus.js:53
(Diff revision 3)
> -  is(browser.contentDocumentAsCPOW.activeElement.localName, "iframe", "Child iframe is focused");
> +  is(browser.contentWindowAsCPOW.document.activeElement.localName, "iframe", "Child iframe is focused");

Why is this CPOW "busted"? We're whitelisting RemoteAddonsParent, so it seems like contentDocumentAsCPOW should work.
Attachment #8703869 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #64)
> Comment on attachment 8703869 [details]
> MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in
> browser_waitForFocus. r?billm
> 
> https://reviewboard.mozilla.org/r/29497/#review26895
> 
> ::: testing/mochitest/tests/browser/browser_waitForFocus.js:53
> (Diff revision 3)
> > -  is(browser.contentDocumentAsCPOW.activeElement.localName, "iframe", "Child iframe is focused");
> > +  is(browser.contentWindowAsCPOW.document.activeElement.localName, "iframe", "Child iframe is focused");
> 
> Why is this CPOW "busted"? We're whitelisting RemoteAddonsParent, so it
> seems like contentDocumentAsCPOW should work.

Hey - I was actually going to ask you the same thing, because I didn't actually know the answer - but I saw you do something similar in attachment 8704367 [details] [diff] [review] in RemoteAddonsParent.jsm... do you know why that was necessary? I suspect it's for the same reason.
Flags: needinfo?(wmccloskey)
Comment on attachment 8704828 [details]
MozReview Request: Bug 1233497 - Stop using TabState.flushAsync in session store tests. r?billm

https://reviewboard.mozilla.org/r/29765/#review26897

Can you file a bug to remove all the synchronous flushing code? It looks like this is the last use of it.
Attachment #8704828 - Flags: review?(wmccloskey) → review+
Comment on attachment 8704829 [details]
MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm

https://reviewboard.mozilla.org/r/29767/#review26899

::: testing/specialpowers/content/specialpowersAPI.js:35
(Diff revision 2)
> +// For when this script is loaded as a frame script, we need to
> +// make sure that the frame script can access CPOWs from the parent.

Can you explain this more? Why would a frame script have a parent-to-child CPOW?
Attachment #8704829 - Flags: review?(wmccloskey)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #65)
> Hey - I was actually going to ask you the same thing, because I didn't
> actually know the answer - but I saw you do something similar in attachment
> 8704367 [details] [diff] [review] in RemoteAddonsParent.jsm... do you know
> why that was necessary? I suspect it's for the same reason.

Oh, right, I forgot. It's because of this:
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#164
We should either get rid of contentDocumentAsCPOW or else send two separate CPOWs from the frame script: one for the window and one for the document.

It looks like contentDocumentAsCPOW is still used in a few places. Do you know what we're planning to do with them? Maybe they're not used, or not tested, or they're safe CPOWs?
Flags: needinfo?(wmccloskey)
Depends on: 1238095
Comment on attachment 8705331 [details]
MozReview Request: Bug 1233497 - Avoid unsafe CPOWs in devtools tests. r?jryans

https://reviewboard.mozilla.org/r/29937/#review26915
Attachment #8705331 - Flags: review?(jryans) → review+
Having worked on fixing up these tests, I’m slightly worried about how this is going to affect add-ons going forward.

The CPOW restriction that billm’s patch adds does a great job of making sure that CPOWs can only be used within add-on compartments. It is trivial (and likely common), however, for an add-on to pass arguments like CPOWs into other compartments, and this is going to cause explosions.

Imagine some built-in JSM that we ship with the browser, like E10SUtils or BrowserUtils. Each JSM has their own compartment. As soon as an add-on attempts to pass a CPOW to one of these, it’ll explode.

I’m seeing this as I’m fixing tests - the two common cases being the Promise-backend.jsm and Task.jsm compartments (essentially meaning that add-ons can no longer resolve Promises or yield in Tasks with CPOWs). These two cases are actually particularly interesting because something like a Promise resolution with a CPOW will occur in a later tick of the event loop, meaning that the add-on code will no longer be on the stack.

To break down my concern:  It’s possible that this change can break add-ons and it’s not clear how many add-ons this will break, nor how bad the breakages will be.

A few possible approaches:

- Don’t care. Just flip the bit, damn the torpedoes, full speed ahead.
- Only flip the bit when running tests. This has the advantage of letting us know when code that we’ve written tests for uses CPOWs, but it will mean that we could miss untested browser code that uses CPOWs.
- Throw exceptions when accessing CPOWs in pre-release channels, but warn in release channels in order to get the word out to our add-on authors that passing CPOWs around to other modules is no-bueno. Perhaps measure how many warnings we’re seeing via Telemetry, and when the warnings drops beneath some threshold, flip the bit.
- Find some way of marking a CPOW as having been passed by add-on code (and the mark persists beyond returns to the event loop), and then allow browser code to handle it. I have no idea if this is possible or even desirable.

There might be other options, but these are the ones that came to my head right off the bat. How do we want to proceed?
Flags: needinfo?(blassey.bugs)
Comment on attachment 8705332 [details]
MozReview Request: Bug 1233497 - Don't yield or resolve CPOWs from Tasks or Promises in devtools tests. r?jryans

https://reviewboard.mozilla.org/r/29939/#review26919
Attachment #8705332 - Flags: review?(jryans) → review+
Comment on attachment 8705330 [details]
MozReview Request: Bug 1233497 - Remove a CPOW from browser/components/places/tests/browser/head.js r?mak

https://reviewboard.mozilla.org/r/29935/#review27045
Attachment #8705330 - Flags: review?(mak77) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #70)

> - Don’t care. Just flip the bit, damn the torpedoes, full speed ahead.
> - Only flip the bit when running tests. This has the advantage of letting us
> know when code that we’ve written tests for uses CPOWs, but it will mean
> that we could miss untested browser code that uses CPOWs.
> - Throw exceptions when accessing CPOWs in pre-release channels, but warn in
> release channels in order to get the word out to our add-on authors that
> passing CPOWs around to other modules is no-bueno. Perhaps measure how many
> warnings we’re seeing via Telemetry, and when the warnings drops beneath
> some threshold, flip the bit.
this could be part of the solution
> - Find some way of marking a CPOW as having been passed by add-on code (and
> the mark persists beyond returns to the event loop), and then allow browser
> code to handle it. I have no idea if this is possible or even desirable.
This is the first thing I thought of. Bill is it possilbe?
> 
> There might be other options, but these are the ones that came to my head
> right off the bat. How do we want to proceed?
Another thing I thought of was white listing JSM's where it would be "allowed" (warn instead of throw). This is assuming that we have a set of "utility" JSMs that account for the majority of the problem.
Flags: needinfo?(blassey.bugs) → needinfo?(wmccloskey)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #75)
> > - Find some way of marking a CPOW as having been passed by add-on code (and
> > the mark persists beyond returns to the event loop), and then allow browser
> > code to handle it. I have no idea if this is possible or even desirable.
> This is the first thing I thought of. Bill is it possilbe?

Maybe, but it would be a lot of work. I don't think it's worth the trouble.

> Another thing I thought of was white listing JSM's where it would be
> "allowed" (warn instead of throw). This is assuming that we have a set of
> "utility" JSMs that account for the majority of the problem.

Yeah, we do have this. We could whitelist the promise and task JSMs. However, it means that we won't catch places in Firefox that do this.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #76)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #75)
> 
> > Another thing I thought of was white listing JSM's where it would be
> > "allowed" (warn instead of throw). This is assuming that we have a set of
> > "utility" JSMs that account for the majority of the problem.
> 
> Yeah, we do have this. We could whitelist the promise and task JSMs.
> However, it means that we won't catch places in Firefox that do this.

Could we instrument the call sites where CPOWs are passed in to check that there is an add-on compartment on the call stack if a CPOW is passed in and throw otherwise?

If not, we'd still be warning, which Mozilla developers should be paying attention to
Comment on attachment 8705330 [details]
MozReview Request: Bug 1233497 - Remove a CPOW from browser/components/places/tests/browser/head.js r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29935/diff/1-2/
Comment on attachment 8703865 [details]
MozReview Request: Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r?Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29489/diff/3-4/
Comment on attachment 8703866 [details]
MozReview Request: Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29491/diff/3-4/
Comment on attachment 8703867 [details]
MozReview Request: Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29493/diff/3-4/
Comment on attachment 8703868 [details]
MozReview Request: Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r?Enn

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29495/diff/3-4/
Comment on attachment 8703869 [details]
MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29497/diff/3-4/
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28285/diff/4-5/
Comment on attachment 8703864 [details]
MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29487/diff/3-4/
Comment on attachment 8704827 [details]
MozReview Request: Bug 1233497 - Permit CPOWs in pageInfo.js until bug 1237025 is fixed. r?florian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29763/diff/2-3/
Comment on attachment 8704828 [details]
MozReview Request: Bug 1233497 - Stop using TabState.flushAsync in session store tests. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29765/diff/2-3/
Comment on attachment 8704829 [details]
MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29767/diff/2-3/
Attachment #8704829 - Flags: review?(wmccloskey)
Comment on attachment 8705331 [details]
MozReview Request: Bug 1233497 - Avoid unsafe CPOWs in devtools tests. r?jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29937/diff/1-2/
Comment on attachment 8705332 [details]
MozReview Request: Bug 1233497 - Don't yield or resolve CPOWs from Tasks or Promises in devtools tests. r?jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29939/diff/1-2/
Attachment #8706962 - Flags: review?(ato) → review+
Comment on attachment 8706962 [details]
MozReview Request: Bug 1233497 - Allow Marionette tests to use CPOWs. r?ato

https://reviewboard.mozilla.org/r/30331/#review27303

::: testing/marionette/driver/marionette_driver/geckoinstance.py:40
(Diff revision 1)
> +        # Until Bug 1238095 is fixed, we have to enable CPOWs in order
> +        # for Marionette tests to work properly.
> +        "dom.ipc.cpows.forbid-unsafe-from-browser": False,

This is fine, but we must also release a new marionette-driver Python package and update the GeckoDriver/wires HTTPD before CPOWs are disallowed because Marionette is being used to run out-of-tree Firefox update-, UI- and media tests.

I can follow up on this.
https://reviewboard.mozilla.org/r/30331/#review27303

> This is fine, but we must also release a new marionette-driver Python package and update the GeckoDriver/wires HTTPD before CPOWs are disallowed because Marionette is being used to run out-of-tree Firefox update-, UI- and media tests.
> 
> I can follow up on this.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1238996 and https://github.com/jgraham/wires/issues/35.
Comment on attachment 8706961 [details]
MozReview Request: Bug 1233497 - Remove some more CPOW yields from Tasks in devtools tests. r?jryans

https://reviewboard.mozilla.org/r/30329/#review27351
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #77)
> (In reply to Bill McCloskey (:billm) from comment #76)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #75)
> > 
> > > Another thing I thought of was white listing JSM's where it would be
> > > "allowed" (warn instead of throw). This is assuming that we have a set of
> > > "utility" JSMs that account for the majority of the problem.
> > 
> > Yeah, we do have this. We could whitelist the promise and task JSMs.
> > However, it means that we won't catch places in Firefox that do this.
> 
> Could we instrument the call sites where CPOWs are passed in to check that
> there is an add-on compartment on the call stack if a CPOW is passed in and
> throw otherwise?
> 
> If not, we'd still be warning, which Mozilla developers should be paying
> attention to

Over to billm to answer that one.
Flags: needinfo?(wmccloskey)
So we seemed to make a call about this bug during the e10s meeting today, and I wanted to make sure that (a) we got it down in here, and (b) I have it correct.

It sounds like we're not particularly worried about this case outside of Promise.jsm and Task.jsm.

Putting a Cu.permitCPOWsInScope(this); into Task.jsm and Promise-backend.jsm is, theoretically, possible, but would also allow CPOWs from browser code to enter them.

One of the problems here is that both Task.jsm and Promise-backend.jsm, as far as I can tell, do a lot of things asynchronously, so I don't know if it's as simple as looking at the stack and checking for add-on code.

billm, how do you recommend we proceed here? I seem to recall you saying that you didn't think this was a big deal... am I making a mountain out of a molehill here, and should we just land this thing?
Yes, let's just use permitCPOWsInScope for tasks and promises and then land this.
Flags: needinfo?(wmccloskey)
Attachment #8704829 - Flags: review?(wmccloskey) → review+
Comment on attachment 8704829 [details]
MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm

https://reviewboard.mozilla.org/r/29767/#review27929

I'd still appreciate an answer to comment 67. Sorry for the delay here though.
https://reviewboard.mozilla.org/r/29497/#review26895

> Why is this CPOW "busted"? We're whitelisting RemoteAddonsParent, so it seems like contentDocumentAsCPOW should work.

(Answered this in https://bugzilla.mozilla.org/show_bug.cgi?id=1233497#c68, but forgot to mention it in MozReview)
https://reviewboard.mozilla.org/r/29767/#review26899

> Can you explain this more? Why would a frame script have a parent-to-child CPOW?

Having a hard time remembering what test I needed this for - it's not in my notes. I've pushed to try with it disabled to try to find it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e2690aa955
https://reviewboard.mozilla.org/r/29767/#review26899

> Having a hard time remembering what test I needed this for - it's not in my notes. I've pushed to try with it disabled to try to find it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e2690aa955

Looks like it was for just one test - test_bug1086684.html, and I guess I was just being lazy. I was able to pretty easily tweak the test to not hit the CPOW - that's what the last patch in the queue does.
Comment on attachment 8705330 [details]
MozReview Request: Bug 1233497 - Remove a CPOW from browser/components/places/tests/browser/head.js r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29935/diff/2-3/
Comment on attachment 8703865 [details]
MozReview Request: Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r?Yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29489/diff/4-5/
Comment on attachment 8703866 [details]
MozReview Request: Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29491/diff/4-5/
Comment on attachment 8703867 [details]
MozReview Request: Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29493/diff/4-5/
Comment on attachment 8703868 [details]
MozReview Request: Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r?Enn

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29495/diff/4-5/
Comment on attachment 8703869 [details]
MozReview Request: Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29497/diff/4-5/
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28285/diff/5-6/
Comment on attachment 8703864 [details]
MozReview Request: Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29487/diff/4-5/
Comment on attachment 8704827 [details]
MozReview Request: Bug 1233497 - Permit CPOWs in pageInfo.js until bug 1237025 is fixed. r?florian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29763/diff/3-4/
Comment on attachment 8704828 [details]
MozReview Request: Bug 1233497 - Stop using TabState.flushAsync in session store tests. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29765/diff/3-4/
Comment on attachment 8705331 [details]
MozReview Request: Bug 1233497 - Avoid unsafe CPOWs in devtools tests. r?jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29937/diff/2-3/
Comment on attachment 8705332 [details]
MozReview Request: Bug 1233497 - Don't yield or resolve CPOWs from Tasks or Promises in devtools tests. r?jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29939/diff/2-3/
Comment on attachment 8706961 [details]
MozReview Request: Bug 1233497 - Remove some more CPOW yields from Tasks in devtools tests. r?jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30329/diff/1-2/
Comment on attachment 8706962 [details]
MozReview Request: Bug 1233497 - Allow Marionette tests to use CPOWs. r?ato

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30331/diff/1-2/
Comment on attachment 8704829 [details]
MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29767/diff/3-4/
Attachment #8704829 - Attachment description: MozReview Request: Bug 1233497 - Allow specialpowersAPI.js to access CPOWs when run as a frame script. r?billm → MozReview Request: Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r?billm
Comment on attachment 8709188 [details]
MozReview Request: Bug 1233497 - Update test_bug1086684.html to not access CPOWs unsafely inside SpecialPowers. r?mrbkap

https://reviewboard.mozilla.org/r/31261/#review28095
Attachment #8709188 - Flags: review?(mrbkap) → review+
Comment on attachment 8699576 [details]
MozReview Request: Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r?enndeakin

https://reviewboard.mozilla.org/r/28285/#review28403

r+'ing on behalf of Neil, who already did this (but in Bugzilla) in comment 48.

::: browser/base/content/test/general/browser.ini:159
(Diff revision 6)
> +skip-if = e10s # Bug 1236991 - Update or remove tests that use fillInPageTooltip

From Neil in https://bugzilla.mozilla.org/show_bug.cgi?id=1233497#c48:

"Add the same comment with the reference to bug 1236991 for browser_input_file_tooltips.js as well."
Attachment #8699576 - Flags: review+
Comment on attachment 8703868 [details]
MozReview Request: Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r?Enn

https://reviewboard.mozilla.org/r/29495/#review28401

r+'ing on behalf of Neil who r+'d the original patch in Bugzilla after https://bugzilla.mozilla.org/show_bug.cgi?id=1233497#c48.
Attachment #8703868 - Flags: review+
Depends on: 1240044
And strangely, the failure on dt6 has been replaced with other devtools failures on my latest try push[1]. *sigh*

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=096839135cb9&selectedJob=15909144
No longer depends on: 1240044
https://hg.mozilla.org/integration/fx-team/rev/e7d56c67d75c304eb4485bad210eeddb92b8a8e4
Bug 1233497 - Remove a CPOW from browser/components/places/tests/browser/head.js r=mak

https://hg.mozilla.org/integration/fx-team/rev/a1d64d759c6e649e2fb38de38ec970c4f6944c0c
Bug 1233497 - Do not resolve a Promise with a CPOW in browser_addonPerformanceAlerts.js r=Yoric

https://hg.mozilla.org/integration/fx-team/rev/5bb1702e477b061cb211f32d1a1bcaa4165c3f14
Bug 1233497 - Allow CPOW usage in BrowserTestUtils testing module. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/08b694b1f841959f398347b5b56f813352212f47
Bug 1233497 - Run translation tests within ContentTasks to avoid CPOW usage. r=mikedeboer

https://hg.mozilla.org/integration/fx-team/rev/c0b6fdacc909ae2feea9feafaea391b041df8fac
Bug 1233497 - SimpleTest.promiseFocus should not resolve to a window, as it might be a CPOW. r=Enn

https://hg.mozilla.org/integration/fx-team/rev/d344e3649a906894a468a5a6623ed7471dc72ddf
Bug 1233497 - Swap a busted CPOW with a working one in browser_waitForFocus. r=billm

https://hg.mozilla.org/integration/fx-team/rev/2b44d2728bed8b9aa90ca5ce7f4b3f49d764ec12
Bug 1233497 - Disable tests that use fillInPageTooltip for e10s. r=Enn

https://hg.mozilla.org/integration/fx-team/rev/5a8f053799fd08dd1d590e597e1fecd974219afc
Bug 1233497 - Make browser_bug710878.js avoid CPOW usage in utilityOverlay.js. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/dd7c15f55cb1f09cf180643aad2d441fbda3bb2c
Bug 1233497 - Stop using TabState.flushAsync in session store tests. r=billm

https://hg.mozilla.org/integration/fx-team/rev/b71e68e61a23dc61338d9229ce25db3200542c5b
Bug 1233497 - Avoid unsafe CPOWs in devtools tests. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/21b62de0f6403506f3e0cf1ed19b28ab33d79a40
Bug 1233497 - Don't yield or resolve CPOWs from Tasks or Promises in devtools tests. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/e93645a6f8f7662fdc28b506255ba01e6469391d
Bug 1233497 - Remove some more CPOW yields from Tasks in devtools tests. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/67889b5fa1d5d2c0ab7143177bd774aea1eaf2f9
Bug 1233497 - Allow Marionette tests to use CPOWs. r=ato

https://hg.mozilla.org/integration/fx-team/rev/9aab5f6aaff9c01029813882fbe782573e15f874
Bug 1233497 - Temporarily allow unsafe CPOWs in Promise-backend.js and Task.jsm. r=billm

https://hg.mozilla.org/integration/fx-team/rev/1f0e4252897a2ee6ebbc9a11ce9f70423c2c831a
Bug 1233497 - Update test_bug1086684.html to not access CPOWs unsafely inside SpecialPowers. r=mrbkap

https://hg.mozilla.org/integration/fx-team/rev/0c6f5d1343ed1443b4a0a18b58f6b90c6e455885
Bug 1233497 - Fix infrastructure for disallowing unsafe CPOWs in browser code. r=mrbkap

https://hg.mozilla.org/integration/fx-team/rev/9bf3cfaeeedc5896c590575eeb8aa57ed530a7fe
Bug 1233497 - Disallow unsafe CPOWs in browser code. r=mrbkap.
So I'm reasonably certain my patches don't actually make those devtools tests fail more than they're currently failing on fx-team. Let's roll the dice.
Depends on: 1243625
Depends on: 1243643
I think we're going to need to update our Electrolysis documentation to talk about this.

This also might affect some add-ons in particular situations.

Setting appropriate flags.
Depends on: 1244343
Depends on: 1246077
I've added a bit on safe versus unsafe CPOWs:

https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers#Safe_and_unsafe_CPOWs

I've also added warnings at the top of the CPOW page:
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers

...and in the "Limitations of chrome scripts" page:
https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Limitations_of_chrome_scripts#Limitations_of_CPOWs

Please let me know if it looks OK, and sufficient.

However, I'm a bit bothered by some of the responses in the dev-platform thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/Nstdr9vsSWI. We're saying "this is only for browser code", which is easy to read as "it won't affect add-on developers". But it does affect add-on developers, if they are "calling browser code and making it do something unsafe". As an add-on developer, how am I to know that I'm making browser code do something unsafe? Is there any way we can unpack this statement to be more helpful?
Flags: needinfo?(mconley)
(In reply to Will Bamberg [:wbamberg] from comment #134)
> I've added a bit on safe versus unsafe CPOWs:
> 
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Cross_Process_Object_Wrappers#Safe_and_unsafe_CPOWs
> 
> I've also added warnings at the top of the CPOW page:
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Cross_Process_Object_Wrappers
> 
> ...and in the "Limitations of chrome scripts" page:
> https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/
> Limitations_of_chrome_scripts#Limitations_of_CPOWs
> 
> Please let me know if it looks OK, and sufficient.
> 
> However, I'm a bit bothered by some of the responses in the dev-platform
> thread:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/Nstdr9vsSWI.
> We're saying "this is only for browser code", which is easy to read as "it
> won't affect add-on developers". But it does affect add-on developers, if
> they are "calling browser code and making it do something unsafe". As an
> add-on developer, how am I to know that I'm making browser code do something
> unsafe? Is there any way we can unpack this statement to be more helpful?

Hey Will,

I guess the most precise way of putting this is: if a CPOW is being used in add-on code, it's okay. If the code causes Firefox to attempt to manipulate a CPOW, it will throw.

So in general, add-on authors should avoid passing CPOWs into non-addon code, as that's where the trouble lies.

Does that clear it up?
Flags: needinfo?(mconley) → needinfo?(wbamberg)
Yes, that's very helpful.

I've updated the page, specifically:
* the warning at the top
* this bit: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Cross_Process_Object_Wrappers#CPOWs_and_platform_APIs

Let me know if it's OK and I'll close the doc request.
Flags: needinfo?(wbamberg) → needinfo?(mconley)
I'd suggest perhaps getting rid of "particular" in " However, add-ons might still see this error if they call particular browser functions that make the brower try an unsafe CPOWs operation.", since it makes me wonder "which ones?".

Otherwise, LGTM!
Flags: needinfo?(mconley)
Mike, should we uplift this CPOW change to Beta 46 for our e10s experiment?
Flags: needinfo?(mconley)
(In reply to Chris Peterson [:cpeterson] from comment #138)
> Mike, should we uplift this CPOW change to Beta 46 for our e10s experiment?

I would be very hesitant to do that. While this bug is _supposed_ to target browser code only, it does have the potential to impact add-ons that pass CPOWs to non-addon code. I'd rather not spring that on our add-on community if at all possible.
Flags: needinfo?(mconley)
That makes sense. Uplifting this change to Beta 46 wouldn't do anything to improve our e10s experiment results. :)
Depends on: 1279168
You need to log in before you can comment on or make changes to this bug.