Closed Bug 1512655 Opened 9 months ago Closed 8 months ago

Audit/fix CompartmentPrivate for same-compartment realms

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Depends on: 1513262
Priority: -- → P2
It's better to have these flags per realm than enabling them for the whole system compartment.
(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.
Flags: needinfo?(jdemooij)
(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.
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.
(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.
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
Flags: needinfo?(jdemooij) → needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
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?
Flags: needinfo?(kmaglione+bmo)
Attachment #9031200 - Attachment is obsolete: true
(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.
Flags: needinfo?(kmaglione+bmo)
Pushed by jdemooij@mozilla.com:
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
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9dc823c27b54
Backed out 3 changesets for bustages in s/xpconnect/src/XPCComponents.cpp:2001 CLOSED TREE
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
Flags: needinfo?(jdemooij)
Bah, silly rooting hazard.
Pushed by jdemooij@mozilla.com:
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
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/d65837fa9710
https://hg.mozilla.org/mozilla-central/rev/96af27c42dc4
https://hg.mozilla.org/mozilla-central/rev/e8b4fb08b5e3
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.