1512655 - Audit/fix CompartmentPrivate for same-compartment realms

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

There are some fields in CompartmentPrivate that I want to move to RealmPrivate. Ideally CompartmentPrivate would only contain wrapper-related things, to make it easier to group realms.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1512655 - Move allowCPOWs and forcePermissiveCOWs from CompartmentPrivate to RealmPrivate, assert harder about correct use. r?kmag

It's better to have these flags per realm than enabling them for the whole system compartment.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> There are some fields in CompartmentPrivate that I want to move to
> RealmPrivate. Ideally CompartmentPrivate would only contain wrapper-related
> things, to make it easier to group realms.

I'm on the fence about this. We definitely don't want to allow enabling these flags for the system compartment. But I also don't really want us to be able to create permissive CPOWs in a whitelisted realm and then pass them to another realm in the same compartment and have them still work.

Do you have a sense of what realms we're currently setting this for? My recollection is that it's only for certain test sandboxes and a couple of whitelisted JSMs. I think we may be better off just forcing those into separate compartments and checking that we never set these flags on the system compartment private.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Kris Maglione [:kmag] from comment #2)
> But I also don't really want us to
> be able to create permissive CPOWs in a whitelisted realm and then pass them
> to another realm in the same compartment and have them still work.

Good point; I hadn't thought of that. Tomorrow I'll try to tighten the asserts instead.

Comment 4

[Jan de Mooij [:jandem]]

We will probably want different approaches here, for instance forcePermissiveCOWs only applies to the *target* of new wrappers we create so I think that could/should still be on RealmPrivate. allowCPOWs OTOH is more of a compartment property.

I'll split this up tomorrow so we can discuss each one separately.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> We will probably want different approaches here, for instance
> forcePermissiveCOWs only applies to the *target* of new wrappers we create
> so I think that could/should still be on RealmPrivate.

Yeah, agreed, that should be fine living on the RealmPrivate.

Comment 6

[Jan de Mooij [:jandem]]

Considering pre-existing issues with the allowCPOWs flag (bug 1514153) and the fact allowCPOWs has no effect anyway if we're not running in automation [0] thanks to evilpie, I wonder if moving that flag to RealmPrivate is actually the right thing to do for now. We could make Cu.permitCPOWsInScope a no-op if !xpc::IsInAutomation() to be even more explicit about this.

[0] https://searchfox.org/mozilla-central/source/js/ipc/JavaScriptParent.cpp#62-65,80

Comment 7

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Considering pre-existing issues with the allowCPOWs flag (bug 1514153) and
> the fact allowCPOWs has no effect anyway if we're not running in automation
> [0] thanks to evilpie, I wonder if moving that flag to RealmPrivate is
> actually the right thing to do for now. We could make Cu.permitCPOWsInScope
> a no-op if !xpc::IsInAutomation() to be even more explicit about this.

This IsInAutomation checks are kind of a double-edged sword. When I moved message manager globals into the shared JSM scope, and segregated the code that had been allowing permissive COWs for all frame scripts, I found a bunch of code was accidentally relying on that behavior, and only worked in tests.

I'm kind of worried about running into the same problem for CPOWs, so I'd really rather segregate that code into isolated compartments as much as possible, and keep the checks at the compartment level.

Comment 8

[Jan de Mooij [:jandem]]

Does that also work for the browser-test.js use though? We load that in a chrome window here IIUC:

https://searchfox.org/mozilla-central/rev/6eae3b2be58c3ecb05e1b0159a1f5a785d2cc727/testing/mochitest/api.js#15

Is that not going to cause problems for bug 1514210?

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1512655 part 1 - Move forcePermissiveCOWs from CompartmentPrivate to RealmPrivate. r?kmag

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1512655 part 2 - Assert Cu.setWantXrays is never called on system-principal scopes. r?kmag

Depends on D14694

Comment 11

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Does that also work for the browser-test.js use though? We load that in a
> chrome window here IIUC:

browser-test.js creates special sandboxes for tests that need to use CPOWs, and only enables CPOWs for those sandboxes. I guess we'll need a way to put those in separate compartments, but it shouldn't affect bug 1514210 one way or the other, since the CPOW code never runs in a window global.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1512655 part 3 - Fix assertion in Cu.permitCPOWsInScope and ensure test globals that use it are in a separate compartment. r?kmag

Depends on D14695

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccb15a086148
part 1 - Move forcePermissiveCOWs from CompartmentPrivate to RealmPrivate. r=kmag
https://hg.mozilla.org/integration/autoland/rev/da21d7e91e19
part 2 - Assert Cu.setWantXrays is never called on system-principal scopes. r=kmag
https://hg.mozilla.org/integration/autoland/rev/2593c7d67f51
part 3 - Fix assertion in Cu.permitCPOWsInScope and ensure test globals that use it are in a separate compartment. r=kmag

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dc823c27b54
Backed out 3 changesets for bustages in s/xpconnect/src/XPCComponents.cpp:2001 CLOSED TREE

Comment 15

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1512655) for bustages in s/xpconnect/src/XPCComponents.cpp:2001 CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=adcf05cc20255510741620dabdbf442a1d3ed0b1&selectedJob=217649544

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217649544&repo=autoland&lineNumber=53870

[task 2018-12-18T12:58:54.400Z] -rw-r--r--  1 worker worker 356K Dec 18 12:58 unnecessary.txt
[task 2018-12-18T12:58:54.400Z] drwxr-xr-x  2 worker worker 4.0K Dec 18 11:42 work
[task 2018-12-18T12:58:54.400Z] -rw-r--r--  1 worker worker 352M Dec 18 12:36 worklist.sort
[task 2018-12-18T12:58:54.400Z] + shopt -s nullglob
[task 2018-12-18T12:58:54.400Z] + set +e
[task 2018-12-18T12:58:54.400Z] + local important
[task 2018-12-18T12:58:54.400Z] + important=(refs.txt unnecessary.txt hazards.txt gcFunctions.txt allFunctions.txt heapWriteHazards.txt)
[task 2018-12-18T12:58:54.400Z] + tar -acvf /builds/worker/artifacts/hazardIntermediates.tar.xz --exclude-from /dev/fd/63 allFunctions.txt callgraph.txt gcEdges.txt gcFunctions.txt gcTypes.txt hazards.txt heapWriteHazards.txt refs.txt rootingHazards.txt typeInfo.txt unnecessary.txt gcFunctions.lst limitedFunctions.lst build_xgill.log
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo refs.txt
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo unnecessary.txt
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo hazards.txt
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo gcFunctions.txt
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo allFunctions.txt
[task 2018-12-18T12:58:54.400Z] ++ for f in '"${important[@]}"'
[task 2018-12-18T12:58:54.400Z] ++ echo heapWriteHazards.txt
[task 2018-12-18T12:58:54.402Z] callgraph.txt
[task 2018-12-18T12:59:05.957Z] gcEdges.txt
[task 2018-12-18T12:59:05.957Z] gcTypes.txt
[task 2018-12-18T12:59:05.960Z] rootingHazards.txt
[task 2018-12-18T12:59:05.961Z] typeInfo.txt
[task 2018-12-18T12:59:05.964Z] gcFunctions.lst
[task 2018-12-18T12:59:06.350Z] limitedFunctions.lst
[task 2018-12-18T12:59:06.352Z] build_xgill.log
[task 2018-12-18T12:59:14.885Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:14.885Z] + gzip -9 -c refs.txt
[task 2018-12-18T12:59:14.886Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:14.887Z] + gzip -9 -c unnecessary.txt
[task 2018-12-18T12:59:14.898Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:14.898Z] + gzip -9 -c hazards.txt
[task 2018-12-18T12:59:14.899Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:14.899Z] + gzip -9 -c gcFunctions.txt
[task 2018-12-18T12:59:21.915Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:21.915Z] + gzip -9 -c allFunctions.txt
[task 2018-12-18T12:59:29.718Z] + for f in '"${important[@]}"'
[task 2018-12-18T12:59:29.718Z] + gzip -9 -c heapWriteHazards.txt
[task 2018-12-18T12:59:29.719Z] + check_commit_msg --upload-xdbs
[task 2018-12-18T12:59:29.720Z] + set +e
[task 2018-12-18T12:59:29.720Z] + [[ -n 1 ]]
[task 2018-12-18T12:59:29.720Z] + hg --cwd /builds/worker/checkouts/gecko log -r. --template '{desc}\n'
[task 2018-12-18T12:59:29.720Z] + grep -F -q -- --upload-xdbs
[task 2018-12-18T12:59:29.839Z] + '[' -n '' ']'
[taskcluster 2018-12-18 12:59:30.172Z] === Task Finished ===
[taskcluster 2018-12-18 12:59:37.282Z] Unsuccessful task run with exit code: 1 completed in 5390.763 seconds

Comment 16

[Jan de Mooij [:jandem]]

Bah, silly rooting hazard.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d65837fa9710
part 1 - Move forcePermissiveCOWs from CompartmentPrivate to RealmPrivate. r=kmag
https://hg.mozilla.org/integration/autoland/rev/96af27c42dc4
part 2 - Assert Cu.setWantXrays is never called on system-principal scopes. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e8b4fb08b5e3
part 3 - Fix assertion in Cu.permitCPOWsInScope and ensure test globals that use it are in a separate compartment. r=kmag

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d65837fa9710
https://hg.mozilla.org/mozilla-central/rev/96af27c42dc4
https://hg.mozilla.org/mozilla-central/rev/e8b4fb08b5e3