Closed Bug 1521907 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(7 files)

Bug 1521907 part 1. Add a version of CheckedUnwrap that can do a dynamic security check. r=jandem,sfink
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 2. Add dynamic CheckedUnwrap support to CrossOriginObjectWrapper. r=peterv,sfink
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 3. Start using CheckedUnwrapStatic/Dynamic in bindings. r=peterv
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 4. Start using CheckedUnwrapStatic/Dynamic in non-binding DOM code. r=peterv
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 5. Start using CheckedUnwrapStatic/Dynamic in XPConnect. r=peterv
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 6. Start using CheckedUnwrapStatic/Dynamic in toolkit. r=peterv
47 bytes, text/x-phabricator-request
Details | Review
Bug 1521907 part 7. Start using CheckedUnwrapStatic/Dynamic in JS debugger. r=jandem
47 bytes, text/x-phabricator-request
Details | Review

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)
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
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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: