Closed Bug 888969 Opened 11 years ago Closed 8 years ago

Implement ES6 Proxy traps for getPrototypeOf and setPrototypeOf

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bbenvie, Assigned: Waldo)

References

(Depends on 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(7 files, 6 obsolete files)

94.52 KB, patch
Details | Diff | Splinter Review
4.33 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
79.57 KB, patch
bholley
: review+
Details | Diff | Splinter Review
14.05 KB, patch
bzbarsky
: review+
efaust
: review+
Details | Diff | Splinter Review
259.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Somewhat recently these two new traps were added to the ES6 spec for Proxies. From ES6 spec (May 2013 revision):

* proxy.[[GetInheritance]] (section 8.5.1)
This is triggered from user code when either the `Object.prototype.__proto__` getter is called with the proxy as the receiver or Object.getPrototypeOf is called on the proxy. It is reflected to the `getPrototypeOf` trap on the handler. The trap, if not undefined, is called to get the trap result. It is then verified that the trap result matches the actual [[Prototype]] of the target, and if they don't match a TypeError is thrown.

* proxy.[[SetInheritance]] (section 8.5.2)
This is triggered from user code when either the `Object.prototype.__proto__` setter is called with the proxy as the receiver or Object.setPrototypeOf is called on the proxy. It is reflected to the `setPrototypeOf` trap on the handler. The trap, if not undefined, is called and its result coerced to boolean indicating whether it changed the target's [[Prototype]] or not. It must be verified that the target is extensible if the trap returns true.


Some notable interactions that should be observed here:

* Doing `proxy.__proto__` will first result in `handler.get(target, '__proto__', proxy)` which, if the normal behavior is followed, will only then result in `handler.getPrototypeOf(target)` once it reaches the Object.prototype ProtoGetter. Same thing for `proxy.__proto__ = proto`.

* The getPrototypeOf trap requires that the result actually matches the [[Prototype]] of the target. This does allow for changing the [[Prototype]] of the target each time the trap is invoked arbitrarily (assuming the target is extensible), but at the end of the trap the proxy must match the target.
Blocks: 889198
Depends on: 914830
Depends on: 926012
No longer blocks: 889198
Blocks: 978228
Assignee: general → nobody
Depends on: 1110083
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
No longer blocks: es6
Attached patch Most of a patch (obsolete) — Splinter Review
This is complete *except* for enforcing OrdinarySetPrototypeOf's "If the [[GetPrototypeOf]] internal method of p is not the ordinary object internal method defined in 9.1.1, let done be true" behavior.  (Note that method may be new in the spec -- previously it was just the ordinary object [[SetPrototypeOf]] definition.)  That's going to require a new trap that indicates whether a sibling setPrototypeOf hook is "non-ordinary", and it's more than I have time to do tonight.  Tomorrow!
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
This may or may not depend on a bunch of (still-local) patchwork rewriting js::Invoke to js::Call.  I think it does, too lazy to check now.  That work will be filed, and deps set as needed, tomorrow, as well.

Or wait.  Today.  :-)  (And in comment 2.)
Historically there was FUD about whether making direct proxies trap these things would break any in-engine code that just walked chains willy-nilly without being careful.  I audited everything (and possibly cleaned up some naming of things), and if everything I renamed here, I audited correctly, there's no worry about just making these traps overridable.

Whether we want to land this patch, I dunno.  The names are marginally better, maybe.  But honestly, a lot of the getProto uses were in JITs in places that weren't immediately obviously safe -- you had to look at callers, figure out interactions (like the number of hops to verify as safe, before guarding so many hops), and generally do fairly non-local things to check this.  I'd probably prefer if we tried to clean that up by attacking the problem at the source to make safety-implications more obvious at each point.  Right now, it just isn't that clear in many cases why one thing or another is safe.
I've run into a snag revealed by existing test canaries.  \o/  /o\

Implementing ordinary objects' [[SetPrototypeOf]] trap correctly, and therefore implementing {Reflect,Object}.setPrototypeOf correctly, requires doing a prototype-chain walk.  Along that walk, each non-proxy object is tested for equality with the proposed new [[Prototype]].  If the walk hits an object whose "[[GetPrototypeOf]] internal method of p is not the ordinary object internal method defined in 9.1.1", looping terminates.

This check requires a trap.  If we have a plain object, we know to exit.  But if we have a proxy, it could be a CrossCompartmentWrapper or an IPC wrapper to a plain object and we'd have to loop.

So, we can add one.  But then we run into the age-old bug 1198056, that adding traps is Really Hard to get Correct.  All traps exist in vtables, so defining a trap on BaseProxyHandler and SecurityWrapper alone, will cause some things to work, and other things to fail to work by omission -- no sign whatsoever in the patch that a forgotten handler must change.

I don't want to stall this patchwork for that.  But this is the third time I've hit the problem of trap-inheritance making it ~impossible to know where traps need to be added/defined to cover all bases.  I really have no way to do that now.  (Other than individually renaming BaseProxyHandler, DirectProxyHandler, and all handler subclasses, then fixing every bustage to ensure I find every user.)  So for the umpteenth time, I'm seriously considering rewriting the system so proxy traps live in explicit function pointer maps.  Any trap-list that doesn't mention my new trap would immediately error at compile time.

I have reviews and other things to do now, so I'm punting what to do for a couple days.  After that, I dunno.
Attached patch Possible patch (obsolete) — Splinter Review
This might be there, now -- need to run full tests on it, not just the jstests/jit-tests hit by a "roxy" glob-pattern.  Almost!
Attachment #8734262 - Attachment is obsolete: true
Patching here is almost complete.  The most recent try-run of partial progress:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8d4972c2107&selectedJob=19234967

I think I have two remaining problems to address, beyond the fairly-simple patching of the original problem.

First is that XPCJSID.cpp's FindObjectForHasInstance, invoked for |foo instanceof Ci.nsIFooBar|, will walk |foo|'s prototype chain (as |instanceof| usually does) while assuming walking doesn't invoke arbitrary code or throw exceptions.  This assumption fails for dom/base/test/chrome/test_cpows.xul, which does some things with a scripted proxy whose handler is *another* scripted proxy that screams bloody murder if any property's ever accessed.  Currently prototype-walking on this scripted proxy does...something that doesn't throw, not sure what, exactly.

This is moderately simply fixed by rewriting it to return the desired object *with* a success/failure code.  Then if prototype-walking fails/throws, no biggie.

Second, the tree's one implementation of nsIRemoteTagService.getRemoteObjectTag -- used by CPOW code to "tag" CPOWs with a string value sent along with them across processes -- does this on (apparently) *every* single CPOW'd object:

  getRemoteObjectTag: function(target) {
    if (target instanceof Ci.nsIDocShellTreeItem) {
      return "ContentDocShellTreeItem";
    }

    if (target instanceof Ci.nsIDOMDocument) {
      return "ContentDocument";
    }

    return "generic";
  }

But test_cpows.xul sends across one of these no-touchy proxies, and XPCJSID.cpp change makes these throw.  So this approach is out.  My first idea was this:

  getRemoteObjectTag: function(target) {
    try {
      if (target.QueryInterface) {
        if (target.QueryInterface(Ci.nsIDocShellTreeItem)) {
          return "ContentDocShellTreeItem";
        }

        if (target.QueryInterface(Ci.nsIDOMDocument)) {
          return "ContentDocument";
        }
      }
    } catch (e) {}

    return "generic";
  }

But toolkit/components/addoncompat/tests/browser/browser_addonShims.js demonstrates the flaw in this: a content document won't have a QueryInterface method on it any more (I think) yet should pass the instanceof test (I think).

Still pondering what to do for this last problem.  Possibly switch nsIRemoteTagService to C++.  A task for tomorrow.
Latest effort, including frumious hackery to address comment 7 part 2:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b1d672a6ed
The C++ tactic might be slightly aesthetically cleaner, but I'm inclined to the smallest possible fix for now.  I could circle back once *this* bug is fixed to make this C++, if desired.
Attachment #8740192 - Flags: review?(bzbarsky)
Moderately straightforward, but the trap-addition is going to be messy to look at, for sure.
Attachment #8740197 - Flags: review?(efaustbmo)
Attachment #8738813 - Attachment is obsolete: true
Blocks: 1263778
Tryservering of these patches, FWIW (without one or two comment-changes added just before posting):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81b1d672a6ed
Comment on attachment 8740190 [details] [diff] [review]
Make XPCJSID instanceof comparisons work correctly when [[GetPrototypeOf]] on the [[Prototype]] chain of the instance being tested throws an exception

>+            JSAutoCompartment ac(cx, obj);

<sigh>.  No one passed a CCW for a proxy in here, eh?  Add a test?

>+    nsresult rv = FindObjectForHasInstance(cx, obj, &target);
>+    if (!NS_WARN_IF(NS_FAILED(rv)))

That "!" does not seem right to me.  Please make sure this fails some existing test, and if not please add a test that it fails.

>+    if (XPCWrappedNative* other_wrapper = XPCWrappedNative::Get(obj)) {

s/obj/target/.  Again, if this is not failing existing tests, please add one it does fail.

r=me with the above fixed, but please do make sure to fix them.
Attachment #8740190 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740192 [details] [diff] [review]
Make our tree's sole implementation of nsIRemoteTagService.getRemoteObjectTag not depend upon the infallibility of [[GetPrototypeOf]] on the object provided to it

r=me
Attachment #8740192 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #13)
> <sigh>.  No one passed a CCW for a proxy in here, eh?  Add a test?

It's not that they never did, it's that nobody ever cared what compartment they were in for this.  Previously all [[GetPrototypeOf]] behavior was constrained to super-simple behavior, or to minimally-complicated behavior.  Only once you allow the arbitrary behavior of this method, does CCW-ness have any relevance.  (And of course note that the returned object may be from another compartment, per the function's interface.  So callers had to deal with other-compartment objects if returned.)

> That "!" does not seem right to me.  Please make sure this fails some
> existing test, and if not please add a test that it fails.
> 
> s/obj/target/.  Again, if this is not failing existing tests, please add one
> it does fail.

Consed up tests for these.  The best, most luxuriant Components.classesByID[...] instanceof tests ever.
Keywords: leave-open
Comment on attachment 8740197 [details] [diff] [review]
Make the getPrototypeOf/setPrototypeOf traps scriptable

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

Looks great to me. We should keep an eye out for FilteringWrapper or XrayWrapper spec confusions.

::: dom/base/nsGlobalWindow.cpp
@@ +920,5 @@
> +                                           bool* isOrdinary,
> +                                           JS::MutableHandle<JSObject*> protop) const
> +{
> +  // Window objects may not have a "lazy" [[Prototype]], but they definitely
> +  // have a non-ordinary (i.e. custom) [[GetPrototypeOf]] trap.

In light of our confused IRL discussion, can we change this, or another sentence, even though it's not necessary that says something like "even though we can implement it as ordinary, the spec-wise non-ordinaryness is visible."

::: js/src/jsapi.h
@@ +2881,5 @@
> + * in |result|.  Otherwise set |*isOrdinary = false|.  In case of error, both
> + * outparams have unspecified value.
> + */
> +extern JS_PUBLIC_API(bool)
> +JS_GetPrototypeIfOrdinary(JSContext* cx, JS::HandleObject obj, bool* isOrdinary,

stock JS_ grumble.

::: js/src/proxy/BaseProxyHandler.cpp
@@ +395,5 @@
>  
>  bool
>  BaseProxyHandler::getPrototype(JSContext* cx, HandleObject proxy, MutableHandleObject protop) const
>  {
> +    MOZ_CRASH("must override getPrototype with lazy prototype");

is the lowercase better for the resulting crash message in some way?

::: js/src/proxy/CrossCompartmentWrapper.cpp
@@ +112,5 @@
> +CrossCompartmentWrapper::getPrototypeIfOrdinary(JSContext* cx, HandleObject wrapper,
> +                                                bool* isOrdinary, MutableHandleObject protop) const
> +{
> +    {
> +        RootedObject wrapped(cx, wrappedObject(wrapper));

s/wrapped/target/ please. It matches better with the existing wrapper terminology.

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +237,3 @@
>  }
>  
> +// ES8 rev 0c1bd3004329336774cbc90de727cd0cf5f11e93 9.5.2 Proxy.[[SetPrototypeOf]].

Is ES8 more likely to be used than ES2017?
Attachment #8740197 - Flags: review?(efaustbmo) → review+
The FilteringWrapper/XrayWrapper bits of this, and their short-circuiting to say non-ordinary, is possibly the tricky/goofy part of this.  Assuming you're still in MV today, I'll try to swing by to explain over IRL if that helps, and to maybe cover the rationale/need for this trap at all.
Attachment #8745105 - Flags: review?(bobbyholley)
Attachment #8740197 - Attachment is obsolete: true
Comment on attachment 8745105 [details] [diff] [review]
Make the getPrototypeOf/setPrototypeOf traps scriptable

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

::: js/src/jsapi.h
@@ +2874,5 @@
>  extern JS_PUBLIC_API(bool)
>  JS_GetPrototype(JSContext* cx, JS::HandleObject obj, JS::MutableHandleObject result);
>  
>  /**
> + * If |obj| (underneath any functionally-transparent wrapper proxies) has as

I don't understand the parenthetical.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +172,5 @@
> +{
> +    // FilteringWrapper<CrossOriginXrayWrapper, CrossOriginAccessiblePropertiesOnly>
> +    // used to implement cross-origin |window| objects corresponds to
> +    // <https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-getprototypeof>,
> +    // so it must say |*isOrdinary = false|.  It's not entirely clear what

Well, if we were to forward to the underling object, and that object (as a window proxy) was non-ordinary, that would work too, right?

But I think it's moot, see below.

@@ +174,5 @@
> +    // used to implement cross-origin |window| objects corresponds to
> +    // <https://html.spec.whatwg.org/multipage/browsers.html#windowproxy-getprototypeof>,
> +    // so it must say |*isOrdinary = false|.  It's not entirely clear what
> +    // other cases must (or perhaps may) say.  For now, conservatively treat
> +    // filtering wrappers as being special.

It seems to me that we should just hoist this case up to SecurityWrapper, and then we don't need to override it here.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2355,5 @@
> +    // |Base|. But doing that statically with templates requires partial method
> +    // specializations (and therefore a helper class), which is all more trouble
> +    // than it's worth. Do a dynamic check.
> +    if (Base::hasSecurityPolicy())
> +        return Base::getPrototypeIfOrdinary(cx, wrapper, isOrdinary, protop);

Assuming that non-ordinary is safe, it seems like we don't need the conditional here. Especially because, as this patch exists right now, security-wrapper _doesn't_ override getPrototypeIfOrdinary with a safe default, so SecurityWrappers will forward the check to the wrappee whereas non-SecurityWrappers will bail, which is generally the opposite of how things normally work.
Attachment #8745105 - Flags: review?(bobbyholley) → review-
Conclusions from video discussion Monday were:

* drop FilteringWrapper's getPrototypeIfOrdinary override
* have the getPrototypeIfOrdinary override in XrayWrapper (removing the Base::hasSecurityPolicy bit)
* ask Chrome/WebKit what they care about for window/location objects

This testcase checks for correct behavior for cycles being permitted through Location objects:

http://playground.whereswalden.com/cross-origin-location-window.html

Chrome/v8 peoples agreed that their not supporting this is a bug.  As long as different observers see a Location object having different [[Prototype]], the [[GetPrototypeOf]] trap in play cannot be the ordinary definition, and so we must permit Location objects to participate in cycles.

https://bugs.chromium.org/p/v8/issues/detail?id=4949

Unfortunately !strcmp("Location") in js::GetPrototypeIfOrdinary won't cut it, because js::SetClassAndProto could create cyclic chains of *native* objects.  Every place that walks prototype chains, aborting on the first non-native prototype, would iloop.  I could modify them all, but they're hairy already, in hairy places like JITs.  I don't think we can do that.

What if we kept treating Location objects as ordinary here, temporarily spec-violating?  Prototype mutation involving Location would keep throwing a TypeError -- no change from current bugginess, and an error case code would have to go out of its way to perform.  Or Reflect.setPrototypeOf would return false and do nothing, but Reflect usage is probably still low.  These consequences seem acceptable in the short run, until Location objects are proxified.

I'll cons up that modified patch now for review.
Attachment #8745105 - Attachment is obsolete: true
> the [[GetPrototypeOf]] trap in play cannot be the ordinary definition, and so we must permit Location
> objects to participate in cycles.

Why must we permit this?  Is it because the loop in OrdinarySetPrototypeOf bails out at soon as it sees a thing with a non-default [[GetPrototypeOf]], just to avoid triggering side-effects?  And I guess we can't fix that via giving Location an interesting [[SetPrototypeOf]] because the loop-creating set may be happening on an ordinary object?

This seems like a spec bug to me, honestly.

Anyway, if all you want to do is proxify Location in the cheapest way possible, we can add a named getter to it that doesn't claim any supported property names.  I believe this should be black-box identical to not having such a named getter, except for the fact that internally we'll use a proxy class for it.
> As long as different observers see a Location object having different [[Prototype]]

Wait.  So this is fallout from the Location spec deciding to use a single object to handle both the same-origin and cross-origin cases instead of using a sane object in the same-origin case and an exotic filter in the cross-origin case?  Domenic, Anne, it's _really_ unfortunate that this forces Location to participate in proto chain cycles...  Do we really want that?
Flags: needinfo?(d)
Flags: needinfo?(annevk)
(In reply to Boris Zbarsky [:bz] from comment #23)
> Why must we permit this?  Is it because the loop in OrdinarySetPrototypeOf
> bails out at soon as it sees a thing with a non-default [[GetPrototypeOf]],
> just to avoid triggering side-effects?

Not necessarily so much side effects, as that objects with unrecognizable [[GetPrototypeOf]] behavior can't be relied upon not to return, say, a new object every time they're called.  Or to claim no cycle at one instant, then profess a cycle after the moment of trial has passed: the object could report a non-cyclic [[Prototype]] at one instant, then the next time it's asked return a cyclic [[Prototype]].

> And I guess we can't fix that via giving Location an interesting
> [[SetPrototypeOf]] because the loop-creating set may be happening on an
> ordinary object?

The Location object's [[Prototype]] isn't what's being set.  It's some *other* object's [[Prototype]], that would create a cycle in the process of setting.

  var loc = window.location;
  var locProto = Object.getPrototypeOf(loc); // Location.prototype -> Object.prototype -> null
  Object.setPrototypeOf(locProto, loc); // locProto -> loc -> locProto -> loc -> ...

Location objects are clearly specified with non-ordinary [[GetPrototypeOf]]:

https://html.spec.whatwg.org/multipage/browsers.html#location-getprototypeof

(In reply to Boris Zbarsky [:bz] from comment #24)
> Wait.  So this is fallout from the Location spec deciding to use a single
> object to handle both the same-origin and cross-origin cases instead of
> using a sane object in the same-origin case and an exotic filter in the
> cross-origin case?

Is single-observable-object not already part of the web platform?  And supposing the two were split, what would be the behavior in the presence of |document.domain| causing a different-origin viewer to observe the [[Prototype]] to change?  Two different [[Prototype]]s at different times?  What happens if the joined origins diverge, because one of them sets |document.domain| *again*, to a different value?  (...is that possible?  I dunno if it is.)

As for exotic filter: from a JS point of view, isn't it the "same" object, and if you passed it across a compartment boundary, behavior would suddenly shift back to the same-origin case again, even if identical-by-equality was required?  That's what it is for |window|, and I've heard claims we want |window| and |location| to be similar this way.

That requires a custom [[GetPrototypeOf]], inexorably dragging in this cycle potentiality.  *Something* has to be queried for this, be it the object's [[GetPrototypeOf]] algorithm, a [[HasGoofyGetPrototypeOf]] boolean, or something else.

> Anyway, if all you want to do is proxify Location in the cheapest way
> possible, we can add a named getter to it that doesn't claim any supported
> property names.

It additionally needs the null-to-other-origins, Location.prototype-to-same-origin behavior.  Which means it needs to claim to be non-ordinary to gPIO.  I don't know enough Codegen.py to effortlessly slot that in, although I could figure it out.
> Is single-observable-object not already part of the web platform?

How would you tell. across origins?  I guess in the document.domain cases you can tell, when you suddenly change whether you're same-origin or not....  That's annoying.  :(

> isn't it the "same" object

It wouldn't have to be, if it were not for document.domain.  :(

> It additionally needs the null-to-other-origins, Location.prototype-to-same-origin behavior.

We already have this, no?  The "other origins" case gets a wrapper for the actual Location that does the right thing.  Or are you proposing to remove that?
(In reply to Boris Zbarsky [:bz] from comment #26)
> How would you tell. across origins?  I guess in the document.domain cases
> you can tell, when you suddenly change whether you're same-origin or not....
> That's annoying.  :(

YES.  :-)

> > isn't it the "same" object
> 
> It wouldn't have to be, if it were not for document.domain.  :(

VIOLENT AGREEMENT.
Flags: needinfo?(d)
Flags: needinfo?(annevk)
Comment on attachment 8745803 [details] [diff] [review]
Remove FilteringWrapper::getPrototypeIfOrdinary, remove the hasSecurityPolicy bit of XrayWrapper::gPIO and have the comment note that it handles Window's behavior

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

r=me on everything but the scripted proxy stuff, which I didn't look at.
Attachment #8745803 - Flags: review?(bobbyholley) → review+
Turns out, once I looked into it, adding a way to specify a non-ordinary [[GetPrototypeOf]] by defining the necessary custom trap is pretty easy to codegen up.  Doing that, and making the call-the-trap determination based purely on whether a proxy is being acted upon, makes this not too bad.

I don't know where/what format we want DOM tests in these days, so no test here yet.  Tell me the where/how (note particularly that testing requires ability to test across multiple domains sharing a non-TLD least common ancestor) and I'll point another patch at you posthaste, landing the two simultaneously if desired.
Attachment #8746397 - Flags: review?(bzbarsky)
Attachment #8746397 - Flags: review?(efaustbmo)
Comment on attachment 8746397 [details] [diff] [review]
Permit a cyclic [[Prototype]] chain to be created through a Location object

The DOM bits look fine, generally.

Given the change to js::GetPrototypeIfOrdinary, do we need to define a useful getPrototypeIfOrdinary on BaseProxyHandler?  And if so, why is the change to BaseDOMProxyHandler needed?  If we don't need to do that, can you explain why not?

r=me with that addressed.

As far as tests go, the right place is probably to add web platform tests.  So run:

  mach web-platform-tests-create testing/web-platform/tests/html/browsers/history/the-location-interface/your-test-name.html

and then edit up that test as needed.  You should have access to the the host:port combinations defined in http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/harness/wptrunner/browsers/server-locations.txt for your multiple hostname needs; I think what's there should be good for your purposes.  You probably want to follow the suggestions at http://testthewebforward.org/docs/test-format-guidelines.html#tests-involving-multiple-origins in terms of naming your things with somename.sub.html and then using things like "http://{{domains[www]}}:{{ports[http][0]}}/path-bits-here" (with the path bits possibly coming from {{location[pathname]}}; this part is a bit clunky because you want to remove the filename from the path and replace it with a different filename....).
Attachment #8746397 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #32)
> Given the change to js::GetPrototypeIfOrdinary, do we need to define a
> useful getPrototypeIfOrdinary on BaseProxyHandler?

We only need to define one *if* there are any handler classes that would pick up BaseProxyHandler's version of the method.  (BPH itself is abstract.)  By my reading, the subclasses of BPH are:

* CPOWProxyHandler, which implements its own version;
* BaseDOMProxyHandler, which in this patch has its own version;
* DirectProxyHandler, which implements its own version;
* ModuleNamespaceObject::ProxyHandler, which provides implements its own version;
* DebugScopeProxy, which *does not* implement its own, but scope objects aren't exposed to untrusted code for prototype access, so crashing is fine;
* DeadObjectProxy, which implements its own version;
* ScriptedDirectProxyHandler, which implements its own version.

So everything's covered not having BPH implement a useful gPIO.

> And if so, why is the
> change to BaseDOMProxyHandler needed?  If we don't need to do that, can you
> explain why not?

Given the above, not implementing BDPH::gPIO would be a crash creating a cycle through a non-Location DOM proxy object.
> We only need to define one *if* there are any handler classes that would pick up BaseProxyHandler's version
> of the method.

Ah.  Is there a reason the BPH version is not pure-virtual then, with the assert moved to DebugScopeProxy?

Not a big deal in either case.
Comment on attachment 8746397 [details] [diff] [review]
Permit a cyclic [[Prototype]] chain to be created through a Location object

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

If we want to assume that it's OK for proxies to have dynamic prototype, we can shove BaseDOMProxyHandler::getPrototypeIfOrdrinary upwards into BaseProxyHandler::getPrototypeIfOrdinary. If we want it to "work for DOM proxies", then we should leave it as is. I don't much care either way, but I guess I have a mild preference for leaving it. We really can't provide a "sane" cover-all behavior for this operation. If we make BPH::getPrototypeIfOrdinary pure virtual, great. If we leave it as MOZ_CRASH. That's also fine.
Attachment #8746397 - Flags: review?(efaustbmo) → review+
./mach web-platform-tests-create made these seemingly-unrelated changes -- splitting them out for cleaner reviewing purposes of the actual test-addition.
Attachment #8747390 - Flags: review?(bzbarsky)
I have no idea whether this is an aesthetically-nice way to async-handle otherwise-more-or-less-parallel things.  But it seems to work.
Attachment #8747391 - Flags: review?(bzbarsky)
Comment on attachment 8747390 [details] [diff] [review]
Make web-platform-tests updates unrelated to , separate from that bug's desired w-p-t changes

Yeah, this is bug 1259775.  r=me, I guess, until we fix the silly tools.  :(
Attachment #8747390 - Flags: review?(bzbarsky) → review+
I tested the other patch some, and it seems to make browsers not supporting this a little unhappy.  I *think* this does the trick for them (at least it does for unfixed Firefox), and it works in fixed Firefox.
Attachment #8747398 - Flags: review?(bzbarsky)
Attachment #8747391 - Attachment is obsolete: true
Attachment #8747391 - Flags: review?(bzbarsky)
Comment on attachment 8747398 [details] [diff] [review]
Alternative, slightly-shorter version that doesn't seem to throw non-conforming browsers for quite so much a loop

>+  crossOrigin.onload = t.step_func(function(e) {

Are you purposefuly not calling t.done() in the catch clause here?  Because if not, you should take out the manual t.done() call and replace your t.step_func() call with t.step_func_done().

Also, triggerSameOriginTest will do an assert_unreached on exception; is there a reason this test does not?

>+      var location = win.location;

Probably better to use "loc" or something for the local, so it doesn't cause confusion when reading the following code due to shadowing the global "location".  This comes up in three places.

>+  Promise.all([triggerCrossOriginTest, triggerSameOriginTest])

This could use some comments explaining that there is an ordering dependency between the tests that requires triggerCrossOriginTest and triggerSameOriginTest to run before we mess with document.domain in crossOriginJoinTest.

r=me
Attachment #8747398 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #40)
> >+  crossOrigin.onload = t.step_func(function(e) {
> 
> Are you purposefuly not calling t.done() in the catch clause here?  Because
> if not, you should take out the manual t.done() call and replace your
> t.step_func() call with t.step_func_done().

Is a test considered "done" for done() purposes if any individual step of it fails or throws an exception?  I took it as required (for lack of any authoritative statement the other direction in the API docs, such as they were) that tests in all cases had to call .done().  (Well, not really -- looks like I had calling .done() here in the rejection case wrong.)  Is is wrong to assume that every test must call .done()?

> Also, triggerSameOriginTest will do an assert_unreached on exception; is
> there a reason this test does not?

Er, ugh.  tSOT is what I really meant to do everywhere, to handle the calling .done() thing, then continue on to the final test.  Question above still stands, for how to deal with this.

> Probably better to use "loc" or something for the local
> This could use some comments explaining that there is an ordering dependency

Fair on both points.  (I was thinking of doing the former this way to clue its being a Location object, but I guess the global confusion is more of a counterargument.)
Flags: needinfo?(bzbarsky)
> Is a test considered "done" for done() purposes if any individual step of it fails or throws an exception? 

I believe so, yes...  jgraham would know for sure.
Flags: needinfo?(bzbarsky) → needinfo?(james)
Yes, if a step in your test fails or throws an exception it's considered finished without you having to call t.done(). I'm not sure how else it could work since the test could theoretically fail at any point.
Flags: needinfo?(james)
I changed moar than I'm really comfortable self-reviewing.  :-(
Attachment #8748344 - Flags: review?(bzbarsky)
Attachment #8747398 - Attachment is obsolete: true
Comment on attachment 8748344 [details] [diff] [review]
Add a test for cyclic [[Prototype]] chains -- one last time because bah

>+var triggerCrossOriginTest = (function() {

This should probably "throw e" after rejecting, like triggerSameOriginTest does.

r=me
Attachment #8748344 - Flags: review?(bzbarsky) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/7c1ce3b16cdc
https://hg.mozilla.org/mozilla-central/rev/1c286374a518
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Is there a specific reason for the Promises?
(In reply to Florian Scholz [:fscholz] (MDN) from comment #49)
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Proxy/handler/setPrototypeOf

Updated this some to document the return value of the setPrototypeOf trap -- traps have to return true or false (well, truthy value or falsy value) to indicate whether the operation succeeded or failed.  I also added examples of returning false versus throwing an exception and the behavioral distinction between them.
Depends on: 1282746
You need to log in before you can comment on or make changes to this bug.