Make CheckedUnwrap a dynamic check
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: jandem)
References
Details
Attachments
(4 files)
Right now CheckedUnwrap is a static check: it just examines the wrapper being unwrapped. In a multiple-globals-per-compartment world, that doesn't work, because whether a wrapper can be unwrapped starts to depend on which global in the compartment asking, at least for the wrappers used for Window and Location objects.
Even without multiple-globals-per-compartment CheckedUnwrap stops being a static check once wrapper recomputation on document.domain set is removed.
The proposal is to add a JSContext argument to CheckedUnwrap, with the current Realm of the JSContext representing the global to use for the dynamic check. And then we need to go through all the callsites and fix them.
I'll add the basic infrastructure and do some conversions, but we may want to parallelize the overall work some.
Assignee | ||
Comment 1•6 years ago
|
||
Does this also affect CheckedUnwraps where it's totally fine to return nullptr for Window/Location wrappers? Consider JS_IsTypedArrayObject for instance... I think we have a lot of those in the JS engine. Keeping an API (with a different name maybe) for that use case might be nice?
Reporter | ||
Comment 2•6 years ago
|
||
That's a good question. I was trying to figure that out for some other cases too, actually.
One simple possibility I've considered is that such callers could pass nullptr for the JSContext and we would use that to mean "do the strict check where we fail unwrap if there's any chance that something in our compartment might not have access to the thing". As in, no JSContext means we just skip the dynamic check and never unwrap the WindowProxy/Location wrappers.
I would also be fine with having an explicit differently-named API for that case. Not quite sure how to name it.
In the short term, I'd like to not keep around a CheckedUnwrap API that does not take a JSContext, because that will make it hard to determine which callsites still need auditing. And in the long term, I think the subtle distinction between the two APIs would make it too easy to reach for the wrong one if they have the same name...
Assignee | ||
Comment 3•6 years ago
|
||
Maybe CheckedUnwrapStatic vs CheckedUnwrapDynamic? Then we could document that CheckedUnwrapDynamic is only necessary when you care about unwrapping special wrappers like WindowProxy and Location. In the JS engine most unwraps are "can we safely get a FooObject out of this" and CheckedUnwrapStatic would be what people should use for that.
Reporter | ||
Comment 4•6 years ago
|
||
I'm going to do all the non-js/src bits in bug 1521907. This bug is going to track the js/src bits.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
The main cases where we can definitely use CheckedUnwrapStatic are:
(1) Code behaves the same when unwrapping or a Class check fails (and the Class
is not a WindowProxy/Location Class). Example:
obj = CheckedUnwrapStatic(obj); return obj && obj->is<FooObject>();
There's no observable difference between unwrapping or not unwrapping
WindowProxy/Location here.
(2) Code knows we have a wrapped FooObject. Example:
obj = CheckedUnwrapStatic(obj); if (!obj) { .. } return obj->as<FooObject>().bar();
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7dc5234c656 part 1 - Use obj->maybeUnwrapAs<T>() or obj->maybeUnwrapIf<T>() instead of CheckedUnwrap where possible. r=luke
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 7•6 years ago
|
||
bugherder |
Assignee | ||
Comment 8•6 years ago
|
||
This uses CheckedUnwrapStatic in places where we don't expect WindowProxy/Location
objects to show up or where we will throw an exception in that case anyway. I also
used this in the more complicated cases (the compartment situation in PromiseObject
for example is very complicated).
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17d8de567ea4 part 2 - Replace remaining CheckedUnwrap calls in js/src. r=luke
Comment 10•6 years ago
|
||
bugherder |
Assignee | ||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
The CacheIR code only sees transparent CCWs so it's fine to do a static unwrap.
DebuggerObject::unwrap is more complicated. We're in the debugger's compartment
there; I went with UnwrapOneCheckedStatic as it seems safest and simplest for
now.
Depends on D21353
Comment 13•6 years ago
|
||
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/82df15131f15 part 3 - Remove an unnecessary CheckedUnwrap call in JSWindowActorService::ReceiveMessage. r=jdai
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
bugherder |
Assignee | ||
Updated•6 years ago
|
Comment 15•6 years ago
|
||
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4325cfacfc49 part 4 - Remove CheckedUnwrap and rename UnwrapOneChecked to UnwrapOneCheckedStatic. r=bzbarsky
Comment 16•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Description
•