Closed Bug 1187234 Opened 5 years ago Closed 5 years ago

IsArray should throw when called on revoked proxy

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: anba, Assigned: Waldo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Test case:
---
var {proxy, revoke} = Proxy.revocable([], {});
revoke();
Array.isArray(proxy);
---

Expected: Throws TypeError
Actual: Returns false

Spec: ES2015, 7.2.2 IsArray, step 3.a
Related: bug 1111785
I have most of a patch for this, but it involves adding a getUnsafeTarget proxy hook.  That's perfectly doable, but our current system of hooking through the vtable means that I'm unsure my patch overrides the hook in *every* class that should override it.  In bug 1198056 I'm going to make the proxy vtable into an explicit struct of functions so that I'm 100% confident I've correctly defined the new hook in all proxy handlers.  Then I'll circle back to this.
Assignee: nobody → jwalden+bmo
Depends on: 1198056
Attached patch Patch (obsolete) — Splinter Review
Because the IsArray method does its own looping, without delegating to anything else, any error that's thrown *must* be from Array.isArray's compartment.  So we need to do an "unsafe" loop, accessing cross-compartment pointers while being careful not to do anything dangerous with them.  The existing objectClassIs trap (previously used to implement this) does this, so this is not quite the worst thing in the world.  :-\

There's some cargo-culting of definitions for some of these handlers (particularly the IPC ones), based on copying existing traps with similar-ish semantics.  Feel free to punt review on any particular part to someone more knowledgeable.

Haven't tryservered this yet, will certainly do so before landing.  I don't expect it would reveal any fundamental issues, so it doesn't seem necessary to have before reviewing might start -- any issues would be small.

How are you supposed to know I've touched all the right places?  First, know that this touched every place where an objectClassIs operation was specified/implemented.  Since IsArray used objectClassIs before, this can be no worse (along that axis) than the prior state.  Second, I have a followup patch directly atop this that renames objectClassIs to getBuiltinClass and modifies its signature, that I'll be posting momentarily to bug 1179003.  That pointed me at all the relevant places to change.  Third, the combination of the two -- which I'll post after posting that patch -- clearly changes *both* sibling traps each and every time it makes a change.  That should be sufficient to verify that all necessary additions were made here (and all changes made in bug 1179003).
Attachment #8654459 - Flags: review?(efaustbmo)
Blocks: 1179003
Depends on: 1199887
Attached patch Patch, v2Splinter Review
The tactic in the previous patch won't work.  The JS::IsArray method in it *only* can indicate an object's an array, if an actual, honest-to-goodness array (or unboxed facsimile) is found as a target.  But not all arrays will occur this way -- particularly not CPOWs.  That issue's fixable, but only if you add another proxy trap.  And at that point, it might as well be a full isArray trap.

So a full trap it is.  I take a somewhat interesting approach here: rather than indicating array-or-not via outparam, I indicate array/not-an-array/revoked-proxy (with the further possibility of the trap failing and throwing an exception).  This preserves the spec requirement of throwing a TypeError on a revoked proxy from another compartment.  And for CPOWs specifically, in mind because of the issue above, we don't have the CPOW throwing an exception that has to cross a process boundary and *definitely* not be from Array.isArray's compartment.  There's a little fun to marshall JS::IsArrayAnswer (an enum class) as a uint32_t across the CPOW boundary, but nothing complicated.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c41cc768f2b5 is colorful, but everything there does seem to be unrelated, so this should be good to review now.
Attachment #8654459 - Attachment is obsolete: true
Attachment #8654459 - Flags: review?(efaustbmo)
Attachment #8657967 - Flags: review?(efaustbmo)
Comment on attachment 8657967 [details] [diff] [review]
Patch, v2

Review of attachment 8657967 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

I guess we have CPOW tests already. I still don't fully understand why we can't have CPOWs forward errors fromthe target process to the host process, and just do this directly. as CCWs do. If they just pass-by-wrapper, then they do, and we have to do this.

::: js/src/json.cpp
@@ +532,4 @@
>      RootedObject obj(cx, &v.toObject());
>  
>      scx->depth++;
> +    auto dec = mozilla::MakeScopeExit([&] { scx->depth--; });

That's a lot of machinery to avoid putting a scx->depth-- in the !IsArray case, but I guess it's more robust in terms of further changes.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +1105,4 @@
>  ScriptedDirectProxyHandler::objectClassIs(HandleObject proxy, ESClassValue classValue,
>                                            JSContext* cx) const
>  {
> +    return false;

MOZ_ASSERT(classValue != ESClass_Array) or something?
Attachment #8657967 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/mozilla-central/rev/6c90d3eab1f7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.