Add a version of CheckedUnwrap that takes a JSContext and convert dom/bindings to using it

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 fixed)

Details

Attachments

(7 attachments)

Needed to start switching CheckedUnwrap to being a dynamic check.

Flags: needinfo?(bzbarsky)

We're going to need this because we will have multiple Realms in the same
compartment which want different CheckedUnwrap behavior in some cases. So we
need to be able to check which Realm we're in.

This will allow us to correctly handle CheckedUnwrapDynamic on wrappers around
WindowProxy and Location.

The basic idea for the changes around UnwrapObjectInternal and its callers
(UnwrapObject, UNWRAP_OBJECT, etc) is to add a parameter to the guts of the
object-unwrapping code in bindings which can be either a JSContext* or nullptr
(statically typed). Then we test which type it is and do either a
CheckedUnwrapDynamic or CheckedUnwrapStatic. Since the type is known at
compile time, there is no actual runtime check; the compiler just emits a call
to the right thing directly (verified by examining the assembly output on
Linux).

The rest of the changes are mostly propagating through that template parameter,
adding static asserts to make sure people don't accidentally pass nullptr while
trying to unwrap to a type that might be a WindowProxy or Location, etc.

There are also some changes to places that were calling CheckedUnwrap directly
to use either the static or dynamic version, as needed.

I am not a huge fan of the UnwrapReflectorToISupports setup here. Maybe we
should introduce two differently-named methods that make it somewhat clear what
the limitations of not taking a JSContext are? I couldn't think of sane
naming...

Flags: needinfo?(bzbarsky)

Comment 8

4 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b0a09a46c70
part 1.  Add a version of CheckedUnwrap that can do a dynamic security check.  r=jandem
https://hg.mozilla.org/integration/autoland/rev/b3daf5ca3d11
part 2.  Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/ca65b46b0d37
part 3.  Start using CheckedUnwrapStatic/Dynamic in bindings.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/192152fe986a
part 4.  Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/2d0895148907
part 5.  Start using CheckedUnwrapStatic/Dynamic in XPConnect.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/efd05f4979f1
part 6.  Start using CheckedUnwrapStatic/Dynamic in toolkit.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/ce3108090e77
part 7.  Start using CheckedUnwrapStatic/Dynamic in JS debugger.  r=jandem

Grr... Try not running all relevant tests strikes again. :(

Attachment #9039669 - Attachment description: Bug 1521907 part 1. Add a version of CheckedUnwrap that can do a dynamic security check. r=jandem → Bug 1521907 part 1. Add a version of CheckedUnwrap that can do a dynamic security check. r=jandem,sfink
Attachment #9039670 - Attachment description: Bug 1521907 part 2. Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper. r=peterv → Bug 1521907 part 2. Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper. r=peterv,sfink

Comment 11

4 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/270b1db9ea81
part 1.  Add a version of CheckedUnwrap that can do a dynamic security check.  r=jandem,sfink
https://hg.mozilla.org/integration/autoland/rev/ac2e180a35b6
part 2.  Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper.  r=peterv,sfink
https://hg.mozilla.org/integration/autoland/rev/e593c29aaff4
part 3.  Start using CheckedUnwrapStatic/Dynamic in bindings.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/585fa0024d46
part 4.  Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/df09b7be63c5
part 5.  Start using CheckedUnwrapStatic/Dynamic in XPConnect.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/ac1c61bf61e9
part 6.  Start using CheckedUnwrapStatic/Dynamic in toolkit.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/ef04359ccf0d
part 7.  Start using CheckedUnwrapStatic/Dynamic in JS debugger.  r=jandem

Backed out 7 changesets (bug 1521907) for failing at unit/test_bug1151385.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/bcb403c04f1c869e7a64636077deb8f6a9ef2aff

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=ef04359ccf0dfb82a65e82fbffff5143a60e941b&selectedJob=225548801

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225548801&repo=autoland&lineNumber=2124

Log snippet:

[task 2019-02-01T22:54:56.355Z] 22:54:56 INFO - TEST-START | js/xpconnect/tests/unit/test_bug1151385.js
[task 2019-02-01T22:54:56.464Z] 22:54:56 WARNING - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_bug1151385.js | xpcshell return code: 0
[task 2019-02-01T22:54:56.466Z] 22:54:56 INFO - TEST-INFO took 115ms
[task 2019-02-01T22:54:56.467Z] 22:54:56 INFO - >>>>>>>
[task 2019-02-01T22:54:56.468Z] 22:54:56 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-02-01T22:54:56.470Z] 22:54:56 WARNING - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_bug1151385.js | run_test - [run_test : 5] false == true
[task 2019-02-01T22:54:56.472Z] 22:54:56 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_bug1151385.js:run_test:5
[task 2019-02-01T22:54:56.473Z] 22:54:56 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:521
[task 2019-02-01T22:54:56.474Z] 22:54:56 INFO - -e:null:1
[task 2019-02-01T22:54:56.475Z] 22:54:56 INFO - exiting test
[task 2019-02-01T22:54:56.476Z] 22:54:56 WARNING - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_bug1151385.js | run_test - [run_test : 7] false == true
[task 2019-02-01T22:54:56.478Z] 22:54:56 INFO - /builds/worker/workspace/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_bug1151385.js:run_test:7
[task 2019-02-01T22:54:56.479Z] 22:54:56 INFO - /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:521
[task 2019-02-01T22:54:56.480Z] 22:54:56 INFO - -e:null:1
[task 2019-02-01T22:54:56.481Z] 22:54:56 INFO - exiting test
[task 2019-02-01T22:54:56.482Z] 22:54:56 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2019-02-01T22:54:56.483Z] 22:54:56 INFO - <<<<<<<
[task 2019-02-01T22:54:56.488Z] 22:54:56 INFO - TEST-START | js/xpconnect/tests/unit/test_exportFunction.js
[task 2019-02-01T22:54:56.684Z] 22:54:56 WARNING - TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_exportFunction.js | xpcshell return code: -11
[task 2019-02-01T22:54:56.685Z] 22:54:56 INFO - TEST-INFO took 192ms

That's a bug in the rooting fix. CheckedUnwrapDynamic needs to return null, not the wrapper, when UnwrapOneCheckedDynamic returns null!

Flags: needinfo?(bzbarsky)

Comment 14

4 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/026c691e29c6
part 1.  Add a version of CheckedUnwrap that can do a dynamic security check.  r=jandem,sfink
https://hg.mozilla.org/integration/autoland/rev/46854f5097bb
part 2.  Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper.  r=peterv,sfink
https://hg.mozilla.org/integration/autoland/rev/64af12d24e9d
part 3.  Start using CheckedUnwrapStatic/Dynamic in bindings.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/f41215bdded6
part 4.  Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/a0b9977daa36
part 5.  Start using CheckedUnwrapStatic/Dynamic in XPConnect.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/f39008382451
part 6.  Start using CheckedUnwrapStatic/Dynamic in toolkit.  r=peterv
https://hg.mozilla.org/integration/autoland/rev/5473faa3c4b0
part 7.  Start using CheckedUnwrapStatic/Dynamic in JS debugger.  r=jandem
You need to log in before you can comment on or make changes to this bug.