Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Description • 14 years ago

Object.defineProperty(<x/>,0,[])

asserts js debug shell on TM tip without -j at Assertion failure: obj->map->ops->lookupProperty == js_LookupProperty, at ../jsobj.cpp:2191

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 1 • 14 years ago

autoBisect shows this is probably related to bug 430133:

The first bad revision is:
changeset:   36651:766a6b2e74e7
user:        Jeff Walden
date:        Fri Jun 05 12:56:45 2009 -0700
summary:     Bug 430133 - Implement ES3.1's Object.defineProperty and Object.defineProperties.  r=jorendorff

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 14 years ago

I can tell what's wrong: obj->isNative() is true for XML objects, since they have scopes, but they don't have anything resembling native ops.

I didn't know this before. To me it looks like jsapi.cpp:DefinePropertyById has the same bug.

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 14 years ago

So does js_LookupPropertyWithFlags (bug 500274), and so does the property cache. I think I can fix them all in this bug.

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 14 years ago

I've given up on fixing bug 500274 here. It's complicated and I don't have time.

This bug warrants a direct fix anyway. Running the tests now...

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 14 years ago

The bulk of this patch is just renaming isNative() to hasScope(), with no change in semantics.

The parts to look out for are where I've changed isNative() to hasStandardPropertyOps(), which excludes XML objects.

	Brendan Eich [:brendan]
	Comment 6 • 14 years ago

I don't think we should rename isNative to hasScope. We're getting rid of scopes. Then what? We need a name, and the name's not the thing. Also, E4X sucks and we shouldn't let it make us do gratuitous work.

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 7 • 14 years ago

I'll post a more straightforward fix, but 'isNative' is the worst identifier in the JS engine. We really ought to rename it.

I believe we need a term to refer to objects that have normal property behavior, that support the shape guarantees and thus can be supported by the property cache and the tracing JIT. For better or worse I think we already have this term, and it is "native object".

XML objects are not native in this sense. But xmlobj->isNative() returns true. That's pretty astonishing. I have no idea what bugs I've introduced (or failed to detect on review) due to not knowing about this.

If 'hasScope' is no good, I'd be happy with renaming 'isNative' to 'isNativeOrXML' globally, then introducing an 'isNative' method that means the right thing.

> Also, E4X sucks and we shouldn't let it make us do gratuitous work.

Boy do I wish I could just ignore E4X. But E4X is going to make us do stupid work as long as we continue to support it. It has the uncanny power to cause bugs almost anywhere the engine, as happened here.

	Brendan Eich [:brendan]
	Comment 8 • 14 years ago

(In reply to comment #7)
> I'll post a more straightforward fix, but 'isNative' is the worst identifier in
> the JS engine. We really ought to rename it.

It's in ECMA-262. You don't want "isNotHost" :-P.

> I believe we need a term to refer to objects that have normal property
> behavior, that support the shape guarantees and thus can be supported by the
> property cache and the tracing JIT. For better or worse I think we already have
> this term, and it is "native object".

Right.

> XML objects are not native in this sense. But xmlobj->isNative() returns true.
> That's pretty astonishing. I have no idea what bugs I've introduced (or failed
> to detect on review) due to not knowing about this.

Let's make XML objects non-native, then?

/be

	Brendan Eich [:brendan]
	Comment 9 • 14 years ago

ECMA-357 is a full of botches, Waldemar tried warning the BEA (ex-MSFT, Crossgain) guys to no avail. It seems to want its crazyland objects to be "native" in the ECMA-262 sense. But we can and should ring-fence E4X in SpiderMonkey by making our implementation's XML objects non-native.

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 14 years ago

The one-line fix.

	Brendan Eich [:brendan]
	Comment 11 • 14 years ago

(In reply to comment #0)
> Object.defineProperty(<x/>,0,[])
> 
> asserts js debug shell on TM tip without -j at Assertion failure:
> obj->map->ops->lookupProperty == js_LookupProperty, at ../jsobj.cpp:2191

Any ill effects in optimized builds?

/be

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 14 years ago

(In reply to comment #8)
> Let's make XML objects non-native, then?

Filed bug 561785. I think that's the right way forward. It's bit of a hike to get there from here though.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 13 • 14 years ago

(In reply to comment #11)
> (In reply to comment #0)
> > Object.defineProperty(<x/>,0,[])
> > 
> > asserts js debug shell on TM tip without -j at Assertion failure:
> > obj->map->ops->lookupProperty == js_LookupProperty, at ../jsobj.cpp:2191
> 
> Any ill effects in optimized builds?
> 
> /be

Doesn't seem to have any:

jsfunfuzz-dbg-64-tm-41266-df5f812ab8b7$ ./js-dbg-64-tm-linux 
js> Object.defineProperty(<x/>,0,[])
Assertion failure: obj->map->ops->lookupProperty == js_LookupProperty, at ../jsobj.cpp:2191
Aborted

jsfunfuzz-dbg-64-tm-41266-df5f812ab8b7$ ./js-opt-64-tm-linux 
js> Object.defineProperty(<x/>,0,[])
<x/>
js>

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 14 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/91a48e004fc2

	Robert Sayre
	Comment 15 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/91a48e004fc2

	Christian Holler (:decoder)
	Comment 16 • 12 years ago

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-560101.js.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now) Reporter
	Comment 17 • 12 years ago

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.