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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: marcia, Assigned: dmandelin)

Tracking

({crash, topcrash})

Trunk
x86
Windows 7
crash, topcrash
Points:
---

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: [jmcrash][fixed-in-tracemonkey], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

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
(Assignee)

Updated

8 years ago
Whiteboard: [jmcrash]
Blocking Beta 8, this crash is quickly moving up.
blocking2.0: --- → beta8+
Duplicate of this bug: 609235
(Assignee)

Comment 3

8 years ago
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

8 years ago
> 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.
Summary: Firefox/4.0b8pre crash in [@ GetPropertyHelper<ScopeNameCompiler>::testForGet() ] → Firefox/4.0b8pre crash [@ GetPropertyHelper<ScopeNameCompiler>::testForGet() ][@ GetPropertyHelper<GetPropCompiler>::testForGet() ]
(Assignee)

Comment 5

8 years ago
(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

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

Comment 8

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

Comment 9

8 years ago
Created attachment 488389 [details] [diff] [review]
Patch
Assignee: general → dmandelin
Status: NEW → ASSIGNED
Attachment #488389 - Flags: review?(dvander)
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?
Attachment #488389 - Flags: review?(dvander)
(Assignee)

Comment 11

8 years ago
Created attachment 488535 [details] [diff] [review]
Patch v2

Currently running on try.
Attachment #488389 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Comment on attachment 488535 [details] [diff] [review]
Patch v2

Looks good on try.
Attachment #488535 - Flags: review?(dvander)
Attachment #488535 - Flags: review?(dvander) → review+
(Assignee)

Comment 13

8 years ago
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).

Updated

8 years ago
Keywords: topcrash
(Assignee)

Comment 14

8 years ago
Created attachment 489252 [details] [diff] [review]
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.
Attachment #489252 - Flags: review?(dvander)
Attachment #489252 - Flags: review?(dvander) → review+
(Assignee)

Comment 16

8 years ago
No reports today--I think we got it. Reopen if you see one with a build date of Nov 10 or later.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [jmcrash] → [jmcrash][fixed-in-tracemonkey]

Comment 17

8 years ago
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.

Updated

8 years ago
Duplicate of this bug: 613017
Depends on: 616508
Crash Signature: [@ GetPropertyHelper<ScopeNameCompiler>::testForGet() ] [@ GetPropertyHelper<GetPropCompiler>::testForGet() ]
You need to log in before you can comment on or make changes to this bug.