901979 - Assertion failure: !global->nativeLookup(cx, id), at ../jsobjinlines.h:1125

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

The following test cases both assert with: !global->nativeLookup(cx, id), at ../jsobjinlines.h:1125

-----
var global = this;
var handler = {
  get: function get(t, pk, r) {},
  has: function has(t, pk) {
    print("has:" + pk);
    if (pk == "size") {
      Object.defineProperty(global, "Map", {value: 0});
    }
    return false
  }
};
Object.prototype.__proto__ = new Proxy(Object.create(null), handler);
Map;
-----

---
var global = this;
var handler = {
  get: function get(t, pk, r) {},
  has: function has(t, pk) {
    print("has:" + pk);
    if (pk == "length") {
      "" + global.Int8Array
    }
    return false
  }
};
Object.prototype.__proto__ = new Proxy(Object.create(null), handler);
ArrayBuffer;
---


This also reveals the initialisation process of certain built-ins...

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 813744 [details] [diff] [review]
v1

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

Of course this is kind of broken for the case of defining a property on a proxy, but that was broken already, so whatever.  The global's never a proxy, so at least the global/lazy problem specifically shown here is fixt.

::: js/src/jit-test/tests/proxy/bug901979-1.js
@@ +3,5 @@
> +
> +var global = this;
> +var status = "pass";
> +var handler = {
> +  get: function get(t, pk, r) {},

Maybe make this |status = "FAIL get";|, and the other "FAIL set"?

::: js/src/jit-test/tests/proxy/bug901979-2.js
@@ +8,5 @@
> +// 1. handler is a proxy that fails the test if you try to call a method on it.
> +var failHandler = {
> +  get: _ => { status = "FAIL"; },
> +  has: _ => { status = "FAIL"; },
> +  invoke: _ => { status = "FAIL"; }

"SMASH" seems more appropriate, perhaps.

::: js/src/jsobj.cpp
@@ -3571,5 @@
>  
>          /*
>           * If we are defining a getter whose setter was already defined, or
> -         * vice versa, finish the job via obj->changeProperty, and refresh the
> -         * property cache line for (obj, id) to map shape.

Die, property cache, die!  ("The, property cache, the" in English, of course.)

@@ +3579,2 @@
>           */
> +        if (!NativeLookupOwnProperty(cx, obj, id, &shape))

Pass along |flags| and use it appopriately.  No real reason not to, when it's this easy, and it might minimize the brokenness in those places that depend on |flags| to get half-correct behavior.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Of course this is kind of broken for the case of defining a property on a
> proxy, but that was broken already, so whatever.  [...]

What exactly is broken when defining on a proxy?

> Maybe make this |status = "FAIL get";|, and the other "FAIL set"?

"FAIL has", yes. I can't believe I left that empty - bizarre.

> "SMASH" seems more appropriate, perhaps.

Smashed.

> > +        if (!NativeLookupOwnProperty(cx, obj, id, &shape))
> 
> Pass along |flags| and use it appopriately.  No real reason not to, when
> it's this easy, and it might minimize the brokenness in those places that
> depend on |flags| to get half-correct behavior.

Huh. When I wrote this I deliberately did not set flags, I guess because the existing code didn't?

I don't think this difference is observable (by the JSRESOLVE_ASSIGNING users we've got) so it seems low-risk to make this change.

Comment 4

[Jeff Walden [:Waldo]]

What's broken is that you can only lookup -- full chain -- on a proxy, there being no getOwn hook in our MOP.  Same old, same old.  Or so it looked to me, seeing this comment a week later.

Comment 5

[Jason Orendorff [:jorendorff]]

But "for the case of defining a property on a proxy", the case you were concerned about, none of this code is ever reached.

I think defining properties on proxies is correct, because proxies have a defineProperty() hook — you don't attempt a lookup first.

Comment 6

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4815b4ad64

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f4815b4ad64