If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: André Bargull, Assigned: jorendorff)

Tracking

({assertion, testcase})

Trunk
mozilla27
assertion, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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...
(Reporter)

Updated

4 years ago
Keywords: assertion, testcase
(Assignee)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

4 years ago
Created attachment 813744 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #813744 - Flags: review?(jwalden+bmo)
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.
Attachment #813744 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 3

4 years ago
(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.
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.
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4815b4ad64
https://hg.mozilla.org/mozilla-central/rev/9f4815b4ad64
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.