608868 - Firefox/4.0b8pre crash [@ GetPropertyHelper<ScopeNameCompiler>::testForGet() ][@ GetPropertyHelper<GetPropCompiler>::testForGet() ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marcia Knous [:marcia]]

Seen while reviewing crash stats.  http://tinyurl.com/3agjpp3 to the crashes so far, which started appearing using the 2010110100 build. So far the crashes are Windows only.

Frame 	Module 	Signature [Expand] 	Source
0 	mozjs.dll 	GetPropertyHelper<ScopeNameCompiler>::testForGet 	js/src/methodjit/PolyIC.cpp:715
1 	mozjs.dll 	ScopeNameCompiler::update 	js/src/methodjit/PolyIC.cpp:1422
2 	mozjs.dll 	js::mjit::ic::Name 	js/src/methodjit/PolyIC.cpp:1864

Comment 1

[Damon Sicore (:damons)]

Blocking Beta 8, this crash is quickly moving up.

Comment 3

[David Mandelin [:dmandelin]]

This hasn't occurred in builds after 11/1, so it may have been fixed by one of the patches mrbkap landed that day. I'll keep watching for a few days, but hopefully we can just close this out.

Comment 4

[Scoobidiver (away)]

> This hasn't occurred in builds after 11/1
Check the crash signature of the dupe and you will see that it is a moving signature. I added the signature of the dupe to the title of this bug.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> Check the crash signature of the dupe and you will see that it is a moving
> signature. I added the signature of the dupe to the title of this bug.

Thanks.

We crash here, with a shape of 0x1:

    LookupStatus testForGet() {
        if (!shape->hasDefaultGetter()) {

That shape value comes a scope chain lookup, here:

    LookupStatus bind() {
        JSProperty *prop;
        if (!js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &holder, &prop))
            return ic.error(cx);
        if (!prop)
            return ic.disable(cx, "lookup failed");
        shape = (const Shape *)prop;
        return Lookup_Cacheable;
    }

This means we are finding the property on a dense array, typed array, or proxy. I would think proxy is the only real possibility here.

I think we can stop it crashing by checking that there are no non-native objects on the proto chain returned by js_FindProperty, but I'd like to know what's going on here.

Andreas, what are the new rules for what can be on the prototype chain? What do we have to check for?

Comment 6

[Andreas Gal :gal]

proto chain: native and non-native objects can be on the proto chain. If you hit a proxy walking the chain, delegate to the proxy (get). The proxy does its own proto walking (proxies have a proto, but they can decide to look up along a different proto chain or not at all).

parent chain and scope chains: only native objects.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Andreas, what are the new rules for what can be on the prototype chain?

No new rules were added for proxies, btw. A JSProperty *prop out parameter can be interpreted as (Shape *) if pobj->isNative() for the related pobj (AKA holder) out param. This was always the case, since the dawn of JSObjectOps in 1998 or whenever it was. It's not a typesafe API, obviously (we should fix it, ideally by devirtualizing so natives and proxies are "it"), but it is not new.

/be

Comment 8

[David Mandelin [:dmandelin]]

(In reply to comment #7)
> (In reply to comment #5)
> > Andreas, what are the new rules for what can be on the prototype chain?
> 
> No new rules were added for proxies, btw. A JSProperty *prop out parameter can
> be interpreted as (Shape *) if pobj->isNative() for the related pobj (AKA
> holder) out param. This was always the case, since the dawn of JSObjectOps in
> 1998 or whenever it was. It's not a typesafe API, obviously (we should fix it,
> ideally by devirtualizing so natives and proxies are "it"), but it is not new.

Thanks. That's exactly what I need to know. I'll add a little documentation to js_FindPropertyHelper as I fix this.

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 488389 [details] [diff] [review]
Patch

Because of this in ScopeNameCompiler::retrieve()

        if (shape && (!obj->isNative() || !holder->isNative())) {
            if (!obj->getProperty(cx, ATOM_TO_JSID(atom), vp))
                return false;

We'll take the wrong branch, shape will be |NULL|.

Maybe instead, set shape before disabling, then test (shape && !IsCacheableBlah()) in retrieve?

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch v2

Currently running on try.

Comment 12

[David Mandelin [:dmandelin]]

Comment on attachment 488535 [details] [diff] [review]
Patch v2

Looks good on try.

Comment 13

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/c5b07cc4b4ca
http://hg.mozilla.org/mozilla-central/rev/728eedc664b2

I will mark fixed after verifying that we are no longer getting this kind of crash report (or drastically reduced frequency).

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Followon patch

Frequency seems to be reduced, but it's still not gone. I checked the code again and saw that for scope objs, we never actually check that the starting object is native. This patch covers that.

Comment 15

[David Mandelin [:dmandelin]]

Followon patch landed:

http://hg.mozilla.org/tracemonkey/rev/049145d64214
http://hg.mozilla.org/mozilla-central/rev/2f1ce583b94a

Comment 16

[David Mandelin [:dmandelin]]

No reports today--I think we got it. Reopen if you see one with a build date of Nov 10 or later.

Comment 17

[chris hofmann]

looks like these patches didn't make beta7 so its turning out to be the top crash there in early data.  if for any reason we need to inject a quick beta8 with limited changes this should be picked up for that release.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c5b07cc4b4ca