Closed Bug 1521906 Opened 1 year ago Closed 1 year ago

Make CheckedUnwrap a dynamic check


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox67 --- fixed


(Reporter: bzbarsky, Assigned: jandem)




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

Depends on: 1521907

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?

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

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.

I'm going to do all the non-js/src bits in bug 1521907. This bug is going to track the js/src bits.

Flags: needinfo?(jdemooij)

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: nobody → jdemooij
Attachment #9042136 - Attachment description: Bug 1521906 part 1 - Use CheckedUnwrapStatic instead of CheckedUnwrap where possible. r?luke! → Bug 1521906 part 1 - Use obj->maybeUnwrapAs<T>() instead of CheckedUnwrap where possible. r?luke!
Attachment #9042136 - Attachment description: Bug 1521906 part 1 - Use obj->maybeUnwrapAs<T>() instead of CheckedUnwrap where possible. r?luke! → Bug 1521906 part 1 - Use obj->maybeUnwrapAs<T>() or obj->maybeUnwrapIf<T>() instead of CheckedUnwrap where possible. r?luke!
Pushed by
part 1 - Use obj->maybeUnwrapAs<T>() or obj->maybeUnwrapIf<T>() instead of CheckedUnwrap where possible. r=luke
Flags: needinfo?(jdemooij)
Keywords: leave-open
Flags: needinfo?(jdemooij)
Depends on: 1527768
No longer depends on: 1527768

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
part 2 - Replace remaining CheckedUnwrap calls in js/src. r=luke

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

Depends on D21353

Pushed by
part 3 - Remove an unnecessary CheckedUnwrap call in JSWindowActorService::ReceiveMessage. r=jdai
Flags: needinfo?(jdemooij)
Keywords: leave-open
Pushed by
part 4 - Remove CheckedUnwrap and rename UnwrapOneChecked to UnwrapOneCheckedStatic. r=bzbarsky
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.