Add a version of CheckedUnwrap that takes a JSContext and convert dom/bindings to using it
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(7 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
Needed to start switching CheckedUnwrap to being a dynamic check.
|
Assignee | |
Comment 1•5 years ago
|
||
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.
|
|
Assignee | |
Comment 2•5 years ago
|
||
This will allow us to correctly handle CheckedUnwrapDynamic on wrappers around
WindowProxy and Location.
|
|
Assignee | |
Comment 3•5 years ago
|
||
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.
|
|
Assignee | |
Comment 4•5 years ago
|
||
|
|
Assignee | |
Comment 5•5 years ago
|
||
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...
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Updated•5 years 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
|
||
Comment 9•5 years ago
|
||
Backed out 7 changesets (bug 1521907) for JSObject Wrapper.cpp bustages and failures
push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=ce3108090e77f23b3272e1de2c463a68035bf963
backout: https://hg.mozilla.org/integration/autoland/rev/0afc21b5734ab60266676c02c8c91f52dc38107b
|
|
Assignee | |
Comment 10•5 years ago
|
||
Grr... Try not running all relevant tests strikes again. :(
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
||
Comment 11•5 years 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
|
||
Comment 12•5 years ago
|
||
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
|
|
Assignee | |
Comment 13•5 years ago
|
||
That's a bug in the rooting fix. CheckedUnwrapDynamic needs to return null, not the wrapper, when UnwrapOneCheckedDynamic returns null!
|
|
||
Comment 14•5 years 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
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/026c691e29c6
https://hg.mozilla.org/mozilla-central/rev/46854f5097bb
https://hg.mozilla.org/mozilla-central/rev/64af12d24e9d
https://hg.mozilla.org/mozilla-central/rev/f41215bdded6
https://hg.mozilla.org/mozilla-central/rev/a0b9977daa36
https://hg.mozilla.org/mozilla-central/rev/f39008382451
https://hg.mozilla.org/mozilla-central/rev/5473faa3c4b0
|
||
Updated•5 years ago
|
Description
•