862848 - defineProperty checks the prototype chain for some odd reason

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is happening in js::DefineNativeProperty when we do this:

3275         if (!baseops::LookupProperty<CanGC>(cx, obj, id, &pobj, &shape))

Should that be something more like HasOwnProperty instead?

In any case, a shell testcase:

  function func() {
  }
  func.prototype = Proxy.create({}, {});
  var obj = new func();
  Object.defineProperty(obj, "foo", { value: 5});
  print(obj.foo);

this throws "TypeError: obj is not a function" on the defineProperty line, when afaict from the spec it should just print 5.

Note that this bug makes lazy setup of DOM stuff via resolve hooks page-detectable, since you can nuke some proto up near the root like this and then defining the default props on all things resolved _after_ that would fail.

Comment 1

[Jeff Walden [:Waldo]]

The lookup API intrinsically finds the property wherever it exists, in whichever prototype object.  It has to call lookup all the way up the chain until it finds nothing.  Switching to HasOwnProperty wouldn't make a difference:

[jwalden@find-waldo-now src]$ gcc-dbg/js
js> function func() { }; func.prototype = Proxy.create({}, {}); var obj = new func();
js> Object.prototype.hasOwnProperty.call(obj, "foopy")
typein:7:0 TypeError: Object.prototype.hasOwnProperty is not a function
js> Object.prototype.hasOwnProperty
function hasOwnProperty() {
    [native code]
}

What's needed is for looking up a property to use a [[GetOwnProperty]]-style mechanism, and to not expose the presence of a property only through a Shape* that can't be meaningful when a proxy object exists along the prototype chain.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> The lookup API intrinsically finds the property wherever it exists

Sure.  My question is why we're using that API at all.  As far as I can tell, per spec we should only be looking for the property on the object itself, right?

HasOwnProperty ends up doing a full lookup too internally???

Comment 3

[Jeff Walden [:Waldo]]

lookupProperty is the fundamental primitive for all property lookups, own or otherwise, in SpiderMonkey right now.  There's no way to avoid it.  So that's what HasOwnProperty uses.  I think there might be a small exception if the object where you start the lookup is a proxy.  Yes, this is obviously inconsistent, considered with respect to this bug.  It's an artifact of not doing property gets/sets the way the proto-climbing refactoring[0] does them.

Historically this was required because any property with JSPROP_SHARED | JSPROP_PERMANENT in its attributes, anywhere on the prototype chain, was to be implicitly treated as an "own" property in the original object, if all objects from original to the object where the property was found had the same class.  This was the dreaded shared-permanent hack.  Over time that's been gradually hacked away, such that maybe -- maybe -- the full chain walk is no longer needed.  I'm looking at the existing code now to figure out whether this is or is not the case any more.

0. http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring

Comment 4

[Jeff Walden [:Waldo]]

I think the walk might no longer be necessary in the purely-own checking case.  HasOwnProperty only returns true if pobj == obj, or pobj == outerized(pobj) as an apparent outer-window hack.  This doesn't require a walk.  Luke, does that sound right?

If so, this might be a good time to add getOwnProperty and hasOwnProperty ObjectOps (both, because ES6 seems to want a hasOwnProperty trap for reasons I don't understand, rather than implying one derived from getOwnProperty behavior).  Then we could have the various has-own checks use those hooks and avoid the walk.  This might be a good incremental step toward getting rid of walking entirely, even.

Comment 5

[Jeff Walden [:Waldo]]

See comment 4.

Comment 6

[Luke Wagner [:luke]]

Sounds good to me

Comment 7

[Jeff Walden [:Waldo]]

Attachment: WIP

This partially works, by adding hasOwn* object ops, but it doesn't handle has-own-property checks on non-native objects.  It looks like doing that properly requires adding getOwn* hooks as well.  That's certainly doable, but I think this is about the limit of how much I can divert immediately right now, from other patches that need investigation/landing that are further along.  I'll get back to this in a few days when those loose ends are cleaned up.

Comment 8

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=897310a46b25 was a try-push of that, exposing the non-native has-own issue in comment 7, plus some other minor failures, plus stuff from all the other patches that were underneath it, that should be irrelevant to this patch particularly.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is actually a bit of a performance drag: it forces the JIT to check for exceptions after basic getters like .firstChild.  If this bug were fixed, we could make those getters infallible....

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - just move some code, v1

I have a narrower fix (3 patches). It specifically fixes HasOwnProperty's behavior when the target object is a native object.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Factor out LookupOwnPropertyWithFlagsInline from LookupPropertyWithFlagsInline, v1

...still no change in behavior, just like part 1...

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Make HasOwnProperty call LookupOwnProp instead of LookupProp, v1

The punchline.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  What about when the target object is a proxy (with a C++ proxy handler)?  In the DOM, we need that case to also work without poking the proto chain...

Comment 14

[Jason Orendorff [:jorendorff]]

Looks green. https://tbpl.mozilla.org/?tree=Try&rev=aadc53da5010

Boris, I hadn't planned to fix that here but it looks easy enough. I'll give it a shot.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 773992 [details] [diff] [review]
Part 1 - just move some code, v1

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

Assuming copypasta, sure.  :-)

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 773995 [details] [diff] [review]
Part 3 - Make HasOwnProperty call LookupOwnProp instead of LookupProp, v1

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

Yeah, the non-native case was the hard part of all this.  The WIP here worked just fine, except for that aspect, as I recall.

Comment 17

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
>> Hmm.  What about when the target object is a proxy (with a C++ proxy handler)?
>> In the DOM, we need that case to also work without poking the proto chain...
>
> Boris, I hadn't planned to fix that here but it looks easy enough. I'll give
> it a shot.

I did give this a shot, a few weeks ago, and found out that the right thing for proxies is to fix up Proxy::defineProperty to the latest spec language, which is different from what was in the wiki page and handles these cases in fairly obvious ways.

It's big enough to file a separate bug: bug 905168. (I have not taken that bug. Maybe Waldo or ejpbruel can take it, or I can come back to it in a few weeks.)

Comment 18

[Jeff Walden [:Waldo]]

I did a bunch of work in bug 826587 to make make those fixes and had a partial, still-not-compiling patch.  Then other things came up that prevented me from finishing it.  I've been rebasing the patch for months, tho, so it would be fairly easy to get back to it.

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 773995 [details] [diff] [review]
Part 3 - Make HasOwnProperty call LookupOwnProp instead of LookupProp, v1

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

::: js/src/jsobj.cpp
@@ +3645,3 @@
>              objp.set(NULL);
>              propp.set(NULL);
> +            *donep = true;

Please move this into the previous patch?

Comment 20

[Jason Orendorff [:jorendorff]]

(In reply to :Ms2ger from comment #19)
> Please move this into the previous patch?

Sure, done.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/019c7f4a167e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e7d1aad638
   https://hg.mozilla.org/integration/mozilla-inbound/rev/4247e81ace63

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/019c7f4a167e
https://hg.mozilla.org/mozilla-central/rev/c5e7d1aad638
https://hg.mozilla.org/mozilla-central/rev/4247e81ace63