Closed Bug 1481184 Opened Last year Closed Last year

Remove remaining JSAutoRealmAllowCCW uses in toolkit/recordreplay

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

There are two of them left:

* https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/toolkit/recordreplay/ipc/JSControl.cpp#204

* https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/toolkit/recordreplay/ipc/JSControl.cpp#344

It looks like both of these could be arbitrary objects (and therefore CCWs) passed in from JS.

Our options are:

(1) Change them to JSAutoRealm, this will diagnostic-assert the object is a CCW.
(2) Use CheckedUnwrap or UncheckedUnwrap and wrap the return value in the cx compartment.
(3) Check for CCWs and throw an exception in the JSNatives receiving those objects.
(4) Store a global object in addition to these objects, and use that with JSAutoRealm (a bit more complicated).
Flags: needinfo?(bhackett1024)
(In reply to Jan de Mooij [:jandem] from comment #0)
> (1) Change them to JSAutoRealm, this will diagnostic-assert the object is a
> CCW.

is *not*, of course.
Hmm, I think these objects will either never be CCWs, or will always be CCWs.  These APIs are used exclusively by devtools/server/actors/replay/debugger.js, and the debugger/handlers used there will always be in the same compartment as the ReplayDebugger object itself which is calling into RecordReplayControl.  However, there is some oddness where RecordReplayControl as accessed here came from a sandbox different from the realm where the ReplayDebugger object lives, see devtools/shared/builtin-modules.js.  I don't know if that realm will be in a different compartment from the ReplayDebugger (or, maybe more importantly, whether it is guaranteed to always be in the same compartment as the ReplayDebugger; FWIW this is all chrome code that is running).

Doing 2) when the objects are initially passed in and then 1) seems like the safest way forward here.
Flags: needinfo?(bhackett1024)
Brian, do we build/test this code on OS X in automation? In that case I'll do a Mac Try push.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8998202 - Flags: review?(bhackett1024)
Comment on attachment 8998202 [details] [diff] [review]
Use JSAutoRealm instead of JSAutoRealmAllowCCW in toolkit/recordreplay

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

Thanks!  Right now there is one devtools mochitest which runs this code, browser_dbg_rr_breakpoints-01.js (more tests coming soon!)
Attachment #8998202 - Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28d1ff679d50
Use JSAutoRealm instead of JSAutoRealmAllowCCW in toolkit/recordreplay. r=bhackett
https://hg.mozilla.org/mozilla-central/rev/28d1ff679d50
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.