Make CheckedUnwrap a dynamic check

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: bzbarsky, Assigned: jandem)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(4 attachments)

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
Assignee

Comment 1

5 months 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?

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

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

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

5 months ago
Flags: needinfo?(jdemooij)
Assignee

Comment 5

4 months 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

4 months ago
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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!

Comment 6

4 months 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

4 months ago
Flags: needinfo?(jdemooij)
Keywords: leave-open
Assignee

Updated

4 months ago
Flags: needinfo?(jdemooij)
Depends on: 1527768
Assignee

Updated

4 months ago
No longer depends on: 1527768
Assignee

Comment 8

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

Comment 9

4 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17d8de567ea4
part 2 - Replace remaining CheckedUnwrap calls in js/src. r=luke
Assignee

Comment 12

4 months 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

4 months 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

4 months ago
Flags: needinfo?(jdemooij)
Assignee

Updated

4 months ago
Keywords: leave-open

Comment 15

4 months 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

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.