Closed Bug 789897 Opened 12 years ago Closed 11 years ago

Implement the preventExtensions and isExtensible trap for proxies

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

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

Attachments

(8 files, 11 obsolete files)

8.77 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
3.57 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
6.57 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.53 KB, patch
bholley
: review+
ejpbruel
: review+
Details | Diff | Splinter Review
7.26 KB, patch
bholley
: review+
ejpbruel
: review+
Details | Diff | Splinter Review
8.76 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.92 KB, patch
bholley
: review+
Details | Diff | Splinter Review
30.67 KB, patch
Details | Diff | Splinter Review
Direct proxies have landed, but there are still a couple of traps missing. Chief among them is the preventExtensions trap. This will allow users to make the target non-extensible by calling Object.preventExtensions on the proxy.
Attached patch Patch to be reviewed (obsolete) — Splinter Review
Notable stuff in this patch:
- isExtensible moved from ObjectImpl to JSObject. This is necessary because
isExtensible needs access to isProxy, which is defined on JSObject. Moving isProxy to ObjectImpl won't work, because it's defined in terms of js::IsProxy, which is defined in jsproxy.h, which includes jsobj.h (a circular dependency)

- Even though preventExtensions is supposed to be a fundamental trap, I didn't want to break DOM bindings by forcing them to implement this trap everywhere. Most, if not all of them do not care about preventExtensions anyway, so it makes sense to provide a default implementation that does whatever Proxies did before this patch (see jsscope.cpp for details)
Attachment #659748 - Flags: review?(jorendorff)
No longer blocks: 674195
Blocks: 674195
Whiteboard: [js:t]
The proposal page hasn't been updated, but TC39 agreed on making isExtensible, isSealed and isFrozen traps too at the July meeting http://wiki.ecmascript.org/doku.php?id=harmony:direct_proxies#discussed_during_tc39_july_2012_meeting_microsoft_redmond
Also, seal and freeze need traps too in the current proposal.

Should I file different bugs or should this one be the one for seal/freeze/isExtensible/isSealed/isFrozen ?

(In reply to Eddy Bruel [:ejpbruel] from comment #1)
> - Even though preventExtensions is supposed to be a fundamental trap, I
> didn't want to break DOM bindings by forcing them to implement this trap
> everywhere. Most, if not all of them do not care about preventExtensions
> anyway, so it makes sense to provide a default implementation that does
> whatever Proxies did before this patch (see jsscope.cpp for details)
I don't understand this part. The notion of fundamental trap is gone in direct proxies and the default action for any missing trap is to forward the operation to the target. (maybe you have fundamental traps in C++ side?)
(In reply to David Bruant from comment #2)
> The proposal page hasn't been updated, but TC39 agreed on making
> isExtensible, isSealed and isFrozen traps too at the July meeting
> http://wiki.ecmascript.org/doku.php?id=harmony:
> direct_proxies#discussed_during_tc39_july_2012_meeting_microsoft_redmond
> Also, seal and freeze need traps too in the current proposal.
> 
> Should I file different bugs or should this one be the one for
> seal/freeze/isExtensible/isSealed/isFrozen ?
> 

You can either file multiple separate bugs for each, or one separate bug for all of them.

I implemented this trap separately because it will fix a bug we were seeing in the add-on SDK when freezing an object cross-compartment (making a proxy non-extensible would not make the target non-extensible. It will now)

> (In reply to Eddy Bruel [:ejpbruel] from comment #1)
> > - Even though preventExtensions is supposed to be a fundamental trap, I
> > didn't want to break DOM bindings by forcing them to implement this trap
> > everywhere. Most, if not all of them do not care about preventExtensions
> > anyway, so it makes sense to provide a default implementation that does
> > whatever Proxies did before this patch (see jsscope.cpp for details)
> I don't understand this part. The notion of fundamental trap is gone in
> direct proxies and the default action for any missing trap is to forward the
> operation to the target. (maybe you have fundamental traps in C++ side?)

That's exactly it.
Comment on attachment 659748 [details] [diff] [review]
Patch to be reviewed

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

More things to test:
- Object.preventExtensions(proxy) returns the proxy (as opposed to the target or anything else).
- The default behavior works (does nothing) if the target was already inextensible.
- The default behavior works if the object isn't a plain Object but something crazy, like the global object or a dense Array.
- If there's no seal trap, but there is a preventExtensions trap, Object.seal(proxy) triggers the preventExtensions hook, among other things.
  (I think this is what's supposed to happen, but it might be good to ask samth.)
- Ditto s/seal/freeze/.
- In the above cases regarding seal and freeze, if the preventExtensions trap returns false, a TypeError is thrown.
- If the handler throws, the exception is propagated (i.e. we don't catch that exception and throw a TypeError or something instead).
- The preventExtensions trap receives the handler object as the this-value.
- The preventExtensions trap receives exactly one argument, which is the target.
- The result of preventExtensions trap is interpreted using ToBoolean.

I also think this should make the following work:

    // Cross-compartment wrappers are transparent to Object.preventExtensions.

    var gw = newGlobal('new-compartment');
    var ow = gw.eval("var o = {}; o");
    Object.preventExtensions(ow);
    assertEq(Object.isExtensible(ow), false);
    assertEq(gw.eval("Object.isExtensible(o)"), false);  // currently FAILS

When the isExtensible trap is implemented, add another test:
- If the target itself is a proxy, the default behavior fires the target-proxy's preventExtensions trap followed by the isExtensible trap.
  I'll implement this one if it isn't obvious what I mean...

::: js/src/jit-test/tests/proxy/testDirectProxyPreventExtensions2.js
@@ +1,3 @@
> +/*
> + * Call the trap with the handler as the this value, and the target as the first
> + * argument

Style nits: Suggest using // comments and moving the comment to the very top, so that 'head -n 1 *.js' tells what all the tests are about.  Tolerate moderately long first lines for this reason.  (Don't bother if you think that is dumb. But I actually use this command in jit-test/tests/debug; try it.)

Same comment on all the tests.  These are nice tests btw.

::: js/src/jit-test/tests/proxy/testDirectProxyPreventExtensions3.js
@@ +9,5 @@
> +        preventExtensions: function (target) {
> +            return true;
> +        }
> +    }));
> +}, TypeError);

Suggest creating the proxy first, then

    assertThrowsInstanceOf(function () { Object.preventExtensions(p); }, TypeError);

When we implement ArrowFuntions, it'll be sweeter:

    assertThrowsInstanceOf(() => Object.preventExtensions(p), TypeError);

Kind of can't wait to do that...

::: js/src/jsapi.cpp
@@ +3533,5 @@
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, obj);
>  
>      /* Assume that non-extensible objects are already deep-frozen, to avoid divergence. */
> +    if (obj->isExtensible())

This change looks a bug; !isExtensible was correct, right?

(Actually this whole if-statement is a bug, but beyond the scope of this patch...)

If our jsapi-test for this doesn't detects this bug, add a check or two, because this basically makes JS_DeepFreezeObject a no-op.  Alternatively, post on the engine mailing list to see if anyone is using this, and if not, remove it in a followup bug. (We're not using it in the browser.)

::: js/src/jsobj.h
@@ +526,5 @@
>  
>    public:
> +    friend class js::BaseProxyHandler;
> +
> +    bool isExtensible() const;

Since it's been decided that there will be an isExtensible trap, this operation is going to be fallible.  So ultimately this method's signature will need to look like JSObject::isSealed():

    static bool isExtensible(JSContext *cs, js::HandleObject *obj, bool *resultp);

Feel free to make that change either in this bug or the followup. Either way, it's a nice standalone chunk of work.

::: js/src/jsproxy.cpp
@@ +415,5 @@
>  
>  bool
> +IndirectProxyHandler::preventExtensions(JSContext *cx, JSObject *proxy)
> +{
> +    return GetProxyTargetObject(proxy)->preventExtensions(cx);

Better to root the target, like the other methods here.

@@ +452,5 @@
>  bool
> +IndirectProxyHandler::isExtensible(JSObject *proxy)
> +{
> +    return GetProxyTargetObject(proxy)->isExtensible();
> +}

Same thing here, if you give isExtensible a fallible-looking type signature.

@@ +1683,5 @@
> +        return false;
> +
> +    // step d
> +    if (trap.isUndefined())
> +        return DirectProxyHandler::preventExtensions(cx, proxy_);

There isn't a DirectProxyHandler::preventExtensions, so this actually calls IndirectProxyHandler::preventExtensions. Right?

It seems kind of weird. Can we instead name the class we're actually calling? On the theory that "weird-because-honest" is a little better than "weird-because-lying".

@@ +1700,5 @@
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_REPORT_NON_EXT);
> +            return false;
> +        }
> +        return true;
> +    } else {

Style micro-nit: else after return.

::: js/src/jsscope.cpp
@@ +1063,5 @@
> + * another fundamental trap, however, so we provide a default implementation
> + * here. We cannot simply forward the operation to the target object, since
> + * BaseProxyHandler is not guaranteed to have one. Instead, we use the proxy's
> + * shape tree (as we would for normal objects) to store a flag indicating that
> + * the proxy is not extensible.

I'm not convinced this is the best behavior.  Does the flag actually have any effect?  Why not just throw, with an error message like "this object doesn't support preventExtensions"?

::: js/src/jsscope.h
@@ +15,5 @@
>  #include <stdio.h>
>  #endif
>  
>  #include "jsobj.h"
> +#include "jsproxy.h"

This is weird. Why is this needed here?
Attachment #659748 - Flags: review?(jorendorff)
Comment on attachment 659748 [details] [diff] [review]
Patch to be reviewed

r=me with the comments addressed; in particular, more tests.

Regarding the test I wrote regarding cross-compartment Object.preventExtensions: I don't think it'll pass with this patch as it stands. Assuming it won't, please file a follow-up bug for that and include the test code.
Attachment #659748 - Flags: review+
Blocks: 703537
(In reply to Eddy Bruel [:ejpbruel] from comment #6)
>  https://hg.mozilla.org/integration/mozilla-inbound/rev/12465a1211e0

Ugh. I just realized I pushed this patch without addressing jorendorff's comments. Silly me.

Can someone please back this out for me?
Looks like this had build failures anyway.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15682739&tree=Mozilla-Inbound

../../../js/src/jsscope.cpp: In member function 'virtual bool js::BaseProxyHandler::preventExtensions(JSContext*, JSObject*)':
../../../js/src/jsscope.cpp:1079:29: error: no matching function for call to 'js::BaseProxyHandler::isExtensible()'
../../../js/src/jsscope.cpp:1071:1: note: candidate is: virtual bool js::BaseProxyHandler::isExtensible(JSObject*)
make[5]: *** [jsscope.o] Error 1
(In reply to Ryan VanderMeulen from comment #8)
> Backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/08cb400c9a49

Thanks Ryan, I was in the process of landing some patches that in my backlog that had already been r+'d. Didn't pay attention with this one. Sorry.
What is the status of this? It still prevents Caja from running untranslated on Firefox.
Yeah, I'm guessing this just fell through the cracks.

Eddy, can you finish up this patch and get this landed? It might need some minor rebasing given the most recent proxy hierarchy refactoring (and disappearance of IndirectProxyHandler), but it shouldn't affect the patch much if at all, I'd think.
Flags: needinfo?(ejpbruel)
(In reply to Bobby Holley (:bholley) from comment #12)
> Yeah, I'm guessing this just fell through the cracks.
> 
> Eddy, can you finish up this patch and get this landed? It might need some
> minor rebasing given the most recent proxy hierarchy refactoring (and
> disappearance of IndirectProxyHandler), but it shouldn't affect the patch
> much if at all, I'd think.

I'm currently at the devtools workweek, but I should be able to make time for this somewhere this week.
Flags: needinfo?(ejpbruel)
I did not have time to work on this during the work week, but it's on my to do stack for this week.
Attached patch Implement isExtensible trap (obsolete) — Splinter Review
For direct proxies, the proxy is extensible iff the target is extensible. At the same time, we do not want to break any existing code for classes inheriting from BaseProxyHandler.

The obvious solution is to add an isExtensible trap to BaseProxyHandler, which defaults to the same behavior as Object::isExtensible, but is overridden by DirectProxyHandler to forward to the target.
Attachment #659748 - Attachment is obsolete: true
Attachment #726760 - Flags: review?
Attached patch Implement preventExtensions trap (obsolete) — Splinter Review
For direct proxies, we forward the preventExtensions trap to the target. For non-direct proxies, we emulate the behavior of Object::preventExtensions for the sake of backwards compatibility (that is, prevent extensions on the proxy itself).
Attachment #726767 - Flags: review?
Attachment #726760 - Flags: review? → review?(bobbyholley+bmo)
Attachment #726767 - Flags: review? → review?(bobbyholley+bmo)
Attached patch Add scripting support + tests (obsolete) — Splinter Review
Attachment #726768 - Flags: review?(bobbyholley+bmo)
Just for the heads up, apparently, the 'seal' and 'frozen' traps aren't in scope for this bug and that's a good thing given that the most recent consensus is that Object.seal and Object.freeze should delegate to the preventExtensions and defineProperty traps.
https://mail.mozilla.org/pipermail/es-discuss/2013-March/029277.html
Comment on attachment 726760 [details] [diff] [review]
Implement isExtensible trap

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

This is probably going to need review from Waldo as well.

::: js/src/jsobjinlines.h
@@ +55,5 @@
> +inline bool
> +JSObject::isExtensible() const
> +{
> +    if (this->isProxy())
> +        return js::Proxy::isExtensible(const_cast<JSObject *>(this));

Do we want to make the proxy trap itself take a const?

::: js/src/jsproxy.cpp
@@ +329,5 @@
>  
>  bool
> +BaseProxyHandler::isExtensible(JSObject *proxy)
> +{
> +    return !proxy->lastProperty()->hasObjectFlag(BaseShape::NOT_EXTENSIBLE);

Does lastProperty() even make sense for non-native objects like proxies? I have no idea if this is really sane or not. Can we ask Waldo?

@@ +690,5 @@
> +bool
> +DirectProxyHandler::isExtensible(JSObject *proxy)
> +{
> +    return GetProxyTargetObject(proxy)->isExtensible();
> +}

This should probably be overridden in SecurityWrapper as well to default-false in those cases. That _might_ break something, depending on the situations in which we use isExtensible. But we should shoot for default-false unless we have a reason to do otherwise.
Attachment #726760 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 726760 [details] [diff] [review]
Implement isExtensible trap

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

::: js/src/jsproxy.h
@@ +128,5 @@
>      virtual bool iterate(JSContext *cx, JSObject *proxy, unsigned flags,
>                           Value *vp);
>  
>      /* Spidermonkey extensions. */
> +    virtual bool isExtensible(JSObject *proxy);

This should be a HandlObject.
Comment on attachment 726767 [details] [diff] [review]
Implement preventExtensions trap

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

::: js/src/jsproxy.cpp
@@ +126,5 @@
> +    AutoIdVector props(cx);
> +    if (!js::GetPropertyNames(cx, proxy, JSITER_HIDDEN | JSITER_OWNONLY, &props))
> +        return false;
> +
> +    return proxy->setFlag(cx, BaseShape::NOT_EXTENSIBLE, JSObject::GENERATE_SHAPE);

Like the other patch, I'm skeptical that this makes any sense, especially since proxies don't have shapes. Waldo or Luke will know.

@@ +467,5 @@
> +DirectProxyHandler::preventExtensions(JSContext *cx, JSObject *proxy)
> +{
> +    RootedObject target(cx, GetProxyTargetObject(proxy));
> +    return target->preventExtensions(cx);
> +}

This definitely needs to be overriden to deny in SecurityWrapper.
Attachment #726767 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 726768 [details] [diff] [review]
Add scripting support + tests

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

I don't really know anything about the scripted proxies. Bouncing to jorendorff.
Attachment #726768 - Flags: review?(bobbyholley+bmo) → review?(jorendorff)
> ::: js/src/jsproxy.cpp
> @@ +329,5 @@
> >  
> >  bool
> > +BaseProxyHandler::isExtensible(JSObject *proxy)
> > +{
> > +    return !proxy->lastProperty()->hasObjectFlag(BaseShape::NOT_EXTENSIBLE);
> 
> Does lastProperty() even make sense for non-native objects like proxies? I
> have no idea if this is really sane or not. Can we ask Waldo?
> 

I see your point. I implemented it like this because this is essentially how preventExtensions works on proxies *right now* (that is, it has no notion of proxies, so it will happily set the NOT_EXTENSIBLE flag on its shape).

If this doesn't make sense, the obvious alternative is to add a mIsExtensible flag to BaseProxyHandler and use that instead.
(In reply to Tom Schuster [:evilpie] from comment #20)
> Comment on attachment 726760 [details] [diff] [review]
> Implement isExtensible trap
> 
> Review of attachment 726760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsproxy.h
> @@ +128,5 @@
> >      virtual bool iterate(JSContext *cx, JSObject *proxy, unsigned flags,
> >                           Value *vp);
> >  
> >      /* Spidermonkey extensions. */
> > +    virtual bool isExtensible(JSObject *proxy);
> 
> This should be a HandlObject.

I just explained to you on irc why its not. isExtensible is only there so we can properly implement the preventExtensions trap. It its not scriptable, nor does it trigger a gc. Moreover, to make it take a HandleObject, we would have to wrap the JSObject * passed to Proxy::isExtensible in a RootedObject, which requires a JSContext *, which Object::isExtensible doesn't take at the moment. 

We can refactor that of course, but it looks like it would be easier do that as part of rebasing when you land your rooting stuff.
(In reply to Bobby Holley (:bholley) from comment #19)
> Comment on attachment 726760 [details] [diff] [review]
> Implement isExtensible trap
> 
> Review of attachment 726760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is probably going to need review from Waldo as well.
> 
> ::: js/src/jsobjinlines.h
> @@ +55,5 @@
> > +inline bool
> > +JSObject::isExtensible() const
> > +{
> > +    if (this->isProxy())
> > +        return js::Proxy::isExtensible(const_cast<JSObject *>(this));
> 
> Do we want to make the proxy trap itself take a const?
> 
I would say no, mainly because:

1. GetProxyTargetObject doesnt take a const
2. The other Proxy traps dont take a const either

> ::: js/src/jsproxy.cpp
> @@ +329,5 @@
> >  
> >  bool
> > +BaseProxyHandler::isExtensible(JSObject *proxy)
> > +{
> > +    return !proxy->lastProperty()->hasObjectFlag(BaseShape::NOT_EXTENSIBLE);
> 
> Does lastProperty() even make sense for non-native objects like proxies? I
> have no idea if this is really sane or not. Can we ask Waldo?
> 
According to Waldo, this is valid. However, it's an implementation detail, and should thus be hidden inside ObjectImpl.

> @@ +690,5 @@
> > +bool
> > +DirectProxyHandler::isExtensible(JSObject *proxy)
> > +{
> > +    return GetProxyTargetObject(proxy)->isExtensible();
> > +}
> 
> This should probably be overridden in SecurityWrapper as well to
> default-false in those cases. That _might_ break something, depending on the
> situations in which we use isExtensible. But we should shoot for
> default-false unless we have a reason to do otherwise.

Let's try to avoid breaking any existing code, and not default false this in SecurityWrapper. Reporting extensible objects as non-extensible seems like a bad idea anyway (there's a reason this is not allowed for scripted direct proxies). If we default false preventExtensions instead, the observable result will be the same.
Attached patch Implement isExtensible trap (obsolete) — Splinter Review
New patch with comments by Bobby addressed.
Attachment #726760 - Attachment is obsolete: true
Attachment #727190 - Flags: review?(bobbyholley+bmo)
Attached patch Implement preventExtensions trap (obsolete) — Splinter Review
Same.
Attachment #726767 - Attachment is obsolete: true
Attachment #727190 - Attachment is obsolete: true
Attachment #727190 - Flags: review?(bobbyholley+bmo)
Attachment #727201 - Flags: review?(bobbyholley+bmo)
Attachment #727190 - Flags: review?(bobbyholley+bmo)
Attachment #727190 - Attachment is obsolete: false
Attached patch Add scripting support + tests (obsolete) — Splinter Review
New patch for Jason in which I actually added the tests to mercurial so they show up in the patch (I keep making this mistake!).
Attachment #726768 - Attachment is obsolete: true
Attachment #726768 - Flags: review?(jorendorff)
Attachment #727205 - Flags: review?(jorendorff)
Comment on attachment 727190 [details] [diff] [review]
Implement isExtensible trap

This appears to be the wrong patch.
Attachment #727190 - Flags: review?(bobbyholley+bmo) → review-
Comment on attachment 727201 [details] [diff] [review]
Implement preventExtensions trap

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

::: js/src/jswrapper.cpp
@@ +662,5 @@
>  template <class Base>
>  bool
> +SecurityWrapper<Base>::preventExtensions(JSContext *cx, JSObject *wrapper)
> +{
> +    return false;

Returning false without setting an exception means OOM - you need to throw here. See what the other SecurityWrapper implementations do.
Attachment #727201 - Flags: review?(bobbyholley+bmo) → review-
Attached patch Implement isExtensible trap (obsolete) — Splinter Review
Attachment #727190 - Attachment is obsolete: true
Attachment #727277 - Flags: review?(bobbyholley+bmo)
Attached patch Implement preventExtensions trap (obsolete) — Splinter Review
Attachment #727277 - Attachment is obsolete: true
Attachment #727277 - Flags: review?(bobbyholley+bmo)
Attachment #727279 - Flags: review?(bobbyholley+bmo)
Comment on attachment 727277 [details] [diff] [review]
Implement isExtensible trap

I think you obsoleted the wrong patch here.
Attachment #727277 - Attachment is obsolete: false
Attachment #727277 - Flags: review?(bobbyholley+bmo)
Attachment #727201 - Attachment is obsolete: true
Comment on attachment 727277 [details] [diff] [review]
Implement isExtensible trap

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

I'd still kind of like to default-false for SecurityWrapper, but I won't insist on it.

This looks fine from a proxy perspective, but Waldo needs to look at it too.
Attachment #727277 - Flags: review?(jwalden+bmo)
Attachment #727277 - Flags: review?(bobbyholley+bmo)
Attachment #727277 - Flags: review+
Attachment #727279 - Flags: review?(jwalden+bmo)
Attachment #727279 - Flags: review?(bobbyholley+bmo)
Attachment #727279 - Flags: review+
(In reply to Bobby Holley (:bholley) from comment #33)
> Comment on attachment 727277 [details] [diff] [review]
> Implement isExtensible trap
> 
> I think you obsoleted the wrong patch here.

Filing patches while sleep deprived is fun!
(In reply to Bobby Holley (:bholley) from comment #34)
> Comment on attachment 727277 [details] [diff] [review]
> Implement isExtensible trap
> 
> Review of attachment 727277 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd still kind of like to default-false for SecurityWrapper, but I won't
> insist on it.
> 
> This looks fine from a proxy perspective, but Waldo needs to look at it too.

Fair enough, I still need a r+ from jorendorff anyway.

As always, thanks for the quick review Bobby.
Comment on attachment 727205 [details] [diff] [review]
Add scripting support + tests

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

::: js/src/jit-test/tests/proxy/testDirectProxyDefineProperty2.js
@@ +25,5 @@
>      configurable: true
>  };
>  Object.defineProperty(new Proxy(target, handler), 'foo', desc);
>  assertEq(called, true);
> +assertEq(Object.isExtensible(target), false);

Why would the target object become inextensible? It looks like it's created extensible, and then nothing ever touches it.

::: js/src/jsproxy.cpp
@@ +1689,5 @@
> +    RootedValue trapResult(cx);
> +    if (!Invoke(cx, ObjectValue(*handler), trap, 1, argv, trapResult.address()))
> +        return false;
> +
> +    // step f 

Micro-nit: trailing whitespace on this line

@@ +1690,5 @@
> +    if (!Invoke(cx, ObjectValue(*handler), trap, 1, argv, trapResult.address()))
> +        return false;
> +
> +    // step f 
> +    bool success = ToBoolean(trapResult);;

Double semicolon here.
Attachment #727205 - Flags: review?(jorendorff) → review+
Looking at the patches, I find myself convinced again that isExtensible and preventExtensions should be in ObjectImpl.  Sorry for vacillating on this!

The idea of the split in the long run is that JSObject exposes only specification methods, and it only knows about ObjectImpl's public/protected interfaces.  (Ideally we'll get to a point where JSObject *privately* inherits from ObjectImpl.  But we're a long ways from there yet.)  In the long run objects will either be regular old things or proxies, and that distinction will be implemented in ObjectImpl, so this is conceptually right and clean.

But implemented as here, JSObject knows how extensibility is determined -- but that's implementation concerns that are ObjectImpl's domain.  So, really this method should be implemented in ObjectImpl.  We were just wrong before to not have its implementation be non-proxy-aware.  I see in super-long-ago comments here that putting it in ObjectImpl was deemed wrong because existing methods were in JSObject, but really that's just because we haven't moved them.  :-)

Rather than have you spend your time figuring out and implementing what I want, and moving stuff until it works, I decided to fix up your patches the way I wanted.  (Lucky you -- I got to rebase through proxy/wrapper handle-ification for you!)  Patch series based on what you did, with the changes I want, coming up.  (I did my best to keep my ObjectImpl-motivated changes to separate patches, so the patches I copied over from you are unchanged in spirit.  Except see infra.)  It looks pretty green so far:

https://tbpl.mozilla.org/?tree=Try&rev=a777f318ce26

One notable change from what you have.  I don't think preserving compatibility with isExtensible and preventExtensions for old-style proxies is required.  And that it affects every proxy is definitely not good.  That it worked before wasn't good, and I think we don't know all the unintended consequences of frobbing the flag on some subset of proxy objects.  And like bholley, I'm not okay with the default being something that half-works, especially if the "security" case must override to throw.  So these patches change that behavior.
Attached patch Implement an isExtensible trap (obsolete) — Splinter Review
Attachment #727277 - Attachment is obsolete: true
Attachment #727277 - Flags: review?(jwalden+bmo)
Attachment #728585 - Flags: review?(ejpbruel)
This is slightly move-until-it-works, unfortunately.  The |using foo| bits are purely to clarify things; they're not necessary, and I could remove them if desired.  I tried to avoid using |asObjectPtr()| when I could, but I didn't want to go too far down the path here, to avoid delaying the bug's actual work.

Not all of these moved-to-impl methods really should be public, but that's a separate concern for a future time.
Attachment #728587 - Flags: review?(jorendorff)
We seem to finally be at a point where most stuff's rooted, so this change was easy enough.
Attachment #728588 - Flags: review?(ejpbruel)
Most of your original patch should be pretty clear here, modulo the forbid-preventExtensions-on-otherwise-unhandled-proxies change.
Attachment #727279 - Attachment is obsolete: true
Attachment #727279 - Flags: review?(jwalden+bmo)
Attachment #728589 - Flags: review?(ejpbruel)
Posting just because I used the existing tests and had to tweak them slightly.  jorendorff's nits (and the substantive issue) have been fixed.  Looked good to me as well with those issues fixed, so I'll stamp it too.  No further review needed, feel free to comment if you have anything, of course.
Attachment #727205 - Attachment is obsolete: true
Attachment #728590 - Flags: review+
Attachment #728587 - Flags: review?(jorendorff) → review+
Pushed the JSObject-to-ObjectImpl method moves patch, as it could be pulled out of sequence and pushed with ease.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15e82d1c776a

The remaining patches, I think, should be pushed as a single revision, once they have review.  Only the final effect of all of them makes sense, or probably passes tests (although for reviewing purposes a splitup was nice).
Whiteboard: [js:t] → [js:t][leave open]
Waldo and I discussed this stuff in the hallway. These were some things we concluded:

* DirectProxyHandler::isExtensible should forward to its target
* SecurityWrapper::isExtensible should _always_ return true. Returning false when the object might be extensible is dangerous (potentially for future JIT correctness), and the extensibility of security wrappers isn't necessarily related to the extensibility of the underlying object. For example, an Xray wrapper to a frozen DOM object should still be extensible from the perspective of the caller.
...and the patches here implement all of comment 45, I believe.  Note that in the last case (Xray that looks extensible when the underlying thing isn't), actions that actually try to add a property to the underlying object will fail -- just a little bit later, perhaps.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #46)
> ...and the patches here implement all of comment 45, I believe.

I don't believe they do. The isExtensible patch doesn't seem to have an override below DirectProxyHandler, so by the looks of it SecurityWrappers would inherit the behavior of forwarding to the target.

> Note that
> in the last case (Xray that looks extensible when the underlying thing
> isn't), actions that actually try to add a property to the underlying object
> will fail -- just a little bit later, perhaps.

Right, but the semantics of Xrays dictate that the JS state of the object is never examined.
Actually, doing it on SecurityWrapper alone isn't enough. We should also put it on XrayWrapper, because we sometimes have XrayWrappers that aren't SecurityWrappers (such as chrome->content Xrays).
There's a fair part of me that thinks "ES5 Harmony fundamental" isn't a meaningful concept for these methods.  What's meaningful is fundamentality, period -- is it inherent in the concept the wrapper/proxy/handler implements?

On that basis, every whole-cloth implementation of a handler subtype should have to expose isExtensible, and provide a preventExtensions implementation.  That, I think, would argue for having BaseProxyHandler::{isExtensible,preventExtensions} be pure virtual.

But probably most implementations would be can-always-extend anyway, so perhaps it's not worth it.  That, or I'm splitting hairs.
Attachment #728585 - Attachment is obsolete: true
Attachment #728585 - Flags: review?(ejpbruel)
Attachment #729365 - Flags: review?(ejpbruel)
Attachment #729365 - Flags: review?(bobbyholley+bmo)
The order of application for the remaining unlanded patches is:

1. Implement isExtensiblePatch
2. Move JSObject::preventExtensions (non-static) to ObjectImpl::preventExtensions (static).
3. Implement a preventExtensions trap
4. Scripting support, tests

https://tbpl.mozilla.org/?tree=Try&rev=1a350a808a7f
Attachment #728589 - Attachment is obsolete: true
Attachment #728589 - Flags: review?(ejpbruel)
Attachment #729367 - Flags: review?(ejpbruel)
Attachment #729367 - Flags: review?(bobbyholley+bmo)
Second try, with more compiling-success: https://tbpl.mozilla.org/?tree=Try&rev=1bdd7102d1f7
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #49)
> Created attachment 729365 [details] [diff] [review]
> isExtensible, including SecurityWrapper and XrayWrapper traps
> 
> There's a fair part of me that thinks "ES5 Harmony fundamental" isn't a
> meaningful concept for these methods.  What's meaningful is fundamentality,
> period -- is it inherent in the concept the wrapper/proxy/handler implements?

Not really. The fundamental-derived distinction is kind of wacky, and I'd like to eventually get rid of it, replacing it with an |own| / |non-own| distinction. But at the moment, that's infeasible because XPCWN Xrays depend on being able to implement getPropertyDescriptor in a way that  totally ignores the prototype chain and flattens all of the inherited methods. We're working on fixing this for the Paris binding setup, so once XPCWN Xrays go away we might be able to re-evaluate it.

> On that basis, every whole-cloth implementation of a handler subtype should
> have to expose isExtensible, and provide a preventExtensions implementation.
> That, I think, would argue for having
> BaseProxyHandler::{isExtensible,preventExtensions} be pure virtual.

That seems reasonable to me. I'm in favor of doing that.
Attachment #728588 - Flags: review?(ejpbruel) → review+
Comment on attachment 729365 [details] [diff] [review]
isExtensible, including SecurityWrapper and XrayWrapper traps

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

::: js/src/jsobj.h
@@ -960,4 @@
>      inline bool isObject() const;
>      inline bool isPrimitive() const;
>      inline bool isPropertyIterator() const;
> -    inline bool isProxy() const;

Do we really need "using js::ObjectImpl::isProxy" here?
Attachment #729365 - Flags: review?(ejpbruel) → review+
Attachment #729367 - Flags: review?(ejpbruel) → review+
"using isProxy" isn't necessary -- just seems to me if all the methods like this are here, we should keep some note of that method here as well.  I'll remove it if you insist, but at this point my queue's ready for push when I get bholley's r+es, so I'm unlikely to frob without specific request.
(In reply to Bobby Holley (:bholley) from comment #53)
> > On that basis, every whole-cloth implementation of a handler subtype should
> > have to expose isExtensible, and provide a preventExtensions implementation.
> > That, I think, would argue for having
> > BaseProxyHandler::{isExtensible,preventExtensions} be pure virtual.
> 
> That seems reasonable to me. I'm in favor of doing that.

Do you want to do that in this bug?
Comment on attachment 729365 [details] [diff] [review]
isExtensible, including SecurityWrapper and XrayWrapper traps

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

r=bholley on the proxy stuff

::: js/src/jswrapper.cpp
@@ +629,5 @@
>  template <class Base>
>  bool
> +SecurityWrapper<Base>::isExtensible(JSObject *wrapper)
> +{
> +    // Just like BaseProxyHandler, SecurityWrappers claim by default to always

Make sure to change this comment if you change the trap to be always virtual in BaseProxyHandler.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1420,5 @@
>  bool
> +XrayWrapper<Base, Traits>::isExtensible(JSObject *wrapper)
> +{
> +    // Xray wrappers don't touch the wrapped object, and they conservatively
> +    // claim to always be extensible.

How about:

// Xray wrappers are supposed to provide a clean view of the target reflector,
// hiding any modifications by script in the target scope. So even if that script
// freezes the reflector, we don't want to make that visible to the caller. DOM
// reflectors are always extensible by default, so we can just return true here.
Attachment #729365 - Flags: review?(bobbyholley+bmo) → review+
Attachment #729367 - Flags: review?(bobbyholley+bmo) → review+
Eh, might as well go all the way in this bug.  Straightforward changes, seem to me, built in a debug build locally.
Attachment #730733 - Flags: review?(bobbyholley+bmo)
Comment on attachment 730733 [details] [diff] [review]
Make BaseProxyHandler::{isExtensible,preventExtensions}} pure virtual

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

r=bholley

::: js/src/vm/ScopeObject.cpp
@@ +1474,5 @@
>      }
> +
> +    bool isExtensible(JSObject *proxy) MOZ_OVERRIDE
> +    {
> +        // always [[Extensible]] like most proxies with no way to change

"no way to change"?
Attachment #730733 - Flags: review?(bobbyholley+bmo) → review+
I guess I can wordsmith that a little bit.  :-)  Maybe

    // always [[Extensible]], can't be made non-[[Extensible]], like most proxies

And, um, I guess I forgot to merge these into a single patch when pushing.  :-(  Oh well, hopefully not the worst thing in the world.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #61)
> And, um, I guess I forgot to merge these into a single patch when pushing. 
> :-(  Oh well, hopefully not the worst thing in the world.

Hm, do they need to be merged? I would have thought that each state in the stack was independently correct...
It's possible.  But given how closely isExtensible and preventExtensions track each other, I don't have much confidence that making a trap for isExtensible, but not having a trap for preventExtensions, results in consistent behavior.  Eh, too late now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81cef708ab47
Whiteboard: [js:t][leave open] → [js:t]
Target Milestone: --- → mozilla22
Changing the summary to reflect what happened in this bug (tell me if I misunderstood)
Keywords: dev-doc-needed
Summary: Implement the preventExtensions trap for proxies → Implement the preventExtensions and isExtensible trap for proxies
Depends on: 856013
Should this be backed out temporarily?

/be
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Brendan Eich [:brendan] from comment #66)
> Should this be backed out temporarily?
> 
> /be

I fully support this - was discussing this on #developers late last night (but most people who worked on this were not around).

I filed bug 855960 which demonstrated how the fuzzers almost went completely crazy, and this morning bug 856013 was reported, which showed a manifestation of the regression impact on Nightly in the wild.
Eddy, you around? Please back out -- no foul, but some harm. Reopen until the blocker is fixed, then re-land. Thanks,

/be
RyanVM is backing this out, as we just mentioned on #developers.
Flags: needinfo?(ryanvm)
(In reply to Brendan Eich [:brendan] from comment #68)
> Eddy, you around? Please back out -- no foul, but some harm. Reopen until
> the blocker is fixed, then re-land. Thanks,
> 
> /be

Hi Brendan. I'm on PTO today, but it looks like RyanVM is taking care of the backing out. Sorry for the trouble caused by this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pretty sure I know what the problem is -- CrossCompartmentWrapper obviously needs to override these hooks to enter compartments correctly -- rebuilding now to test.  It's a pretty depressing state of affairs for our tests if that's the entire extent of the problem.  I'll bet the lack-of-tests blame falls on me, too, seeing as bug 492849 was my fault.  :-\
I hit some conflicts along the way in jsproxy.h due to bug 855136, but I *think* got the right final product. We'll see if it burns anyway...
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0b6744aef6
Flags: needinfo?(ryanvm)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #71)
> Pretty sure I know what the problem is -- CrossCompartmentWrapper obviously
> needs to override these hooks to enter compartments correctly -- rebuilding
> now to test.  It's a pretty depressing state of affairs for our tests if
> that's the entire extent of the problem.  I'll bet the lack-of-tests blame
> falls on me, too, seeing as bug 492849 was my fault.  :-\

I tried entering the target's compartment in DirectProxyHandler::preventExtensions. That didn't change anything, sadly.

I quickly verified this using the one-liner tests in bug 855960.
Yes, this needs an override in CrossCompartmentWrapper. Sorry for not catching that.
This seems to fix everything complained about.  My tree's still pre-backout, so this is atop everything here -- seems better for reviewing purposes, too.  Next step: fold it all together for a single push when ready (and for gkw to fuzz, since he seems to be chomping at the bit there ;-) ).
Attachment #731316 - Flags: review?(bobbyholley+bmo)
Comment on attachment 731316 [details] [diff] [review]
Add hooks to CrossCompartmentWrapper

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

r=bholley

::: js/src/jswrapper.cpp
@@ +228,5 @@
>  
>  bool
> +CrossCompartmentWrapper::isExtensible(JSObject *wrapper)
> +{
> +    return wrappedObject(wrapper)->isExtensible();

The lack of a cx to enter a compartment here is troublesome. We don't know anything about the wrapped object (it might even be a proxy!), and and it could theoretically pull the cx off the context stack and continue using it (in the wrong compartment). But I guess that's not very likely.

Please add a comment here about this.
Attachment #731316 - Flags: review?(bobbyholley+bmo) → review+
Gary, you see any issues with this?  I'll push in a couple hours if you don't see anything more.
Comment on attachment 731335 [details] [diff] [review]
Final rollup patch of everything

Fuzz-testing this patch now on Linux, as I type. (with Object.preventExtensions and others turned back on in jsfunfuzz.js)
Attachment #731335 - Flags: feedback?(gary)
Comment on attachment 731335 [details] [diff] [review]
Final rollup patch of everything

Marking as feedback+ based on the first few minutes of fuzzing, in that it doesn't immediately blow everything up.

Will still be testing this patch.
Attachment #731335 - Flags: feedback?(gary) → feedback+
https://hg.mozilla.org/mozilla-central/rev/f5b92b22981c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 856384
Whiteboard: [js:t] → [js:t][DocArea=JS]
Thanks, :arai! Reviewed.

I noticed that bug 978235 also claims to have implemented the isExtensible trap. (which would be Firefox 31 instead of 22 in that case). Maybe someone who knows can update or clarify.
Hm, only ScriptedDirectProxyHandler::preventExtensions is implemented in this bug,
and ScriptedDirectProxyHandler::isExtensible is implemented in bug 978235.
So I guess we should move the release note about isExtensible for JavaScript to Firefox 31.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: