The default bug view has changed. See this FAQ.

"Assertion failure: shouldCacheProtoShape(cx, proto, &shouldCache) && shouldCache" in ListBase::nativeGet

RESOLVED FIXED in mozilla13

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: peterv)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla13
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 566339 [details]
testcase (asserts fatally when loaded)

Assertion failure: shouldCacheProtoShape(cx, proto, &shouldCache) && shouldCache, at js/src/xpconnect/src/dombindings.cpp:888
(Reporter)

Comment 1

6 years ago
Created attachment 566340 [details]
stack trace
tracking-firefox10: --- → ?
Over to peterv for investigation.
Assignee: nobody → peterv
(Assignee)

Comment 3

6 years ago
Shape doesn't cover the values of properties, so the cached shape didn't change. The assert looks wrong, but really the underlying assumptions are wrong. We rely on a shape change to detect changes in values.
Can we somehow guard on TI information instead of guarding on shape here?  Does TI even have the information we need?
What is the information being guarded on?  Changes in values can trigger changes in types (e.g. changing function-valued properties), but those changes can't really be guarded on in the same manner as shapes.  When a type changes it can trigger jitcode recompilation, and could potentially trigger e.g. invalidation of certain caches also.  We don't do the latter anywhere; I haven't seen places it is worth the hassle, and additionally TI isn't always on.
The basic setup here is that we have a proxy whose prototype has a property named 'item' which is a JS function that's backed by a fastnative that goes and calls into Gecko code.

Since this is a proxy there are no ICs going on in the jit, so when someone does .item() on it we have to walk up the proto chain looking for the property, then return the value it has.  This is somewhat slow.

So what we'd like to do is to guard on the case when the prototype is "correct" in the sense that its .item is the function we expect.  And in that case, we can just directly return that function without having to do a full prototype chain walk.

The current code actually guards on all the prototype's methods being what we expect (which is the common case).  But it only guards on the shape, which of course does not capture the actual value of the method-valued properties...
(Reporter)

Comment 7

6 years ago
I thought shapes did include the values of properties of type function. Did that change?
(Reporter)

Comment 8

6 years ago
Why is it faster for the optimization to check the prototype than for the normal codepath to walk up one level in the prototype chain?
(In reply to Jesse Ruderman from comment #7)
> I thought shapes did include the values of properties of type function. Did
> that change?

This is sometimes true --- shape determines the particular function for branded objects and method shapes, neither of which I think will apply here.  Also, the object shrinking stuff going on in the JM branch removes both of these constraints.

I think for now the code should check the slot value as well, the shape check ensures it is in a particular slot which should be fast to access.  The fastest solution will be to teach the JIT about the behavior of the proxy, so that it can generate code which will be invalidated should the prototype's 'item' property change.  That is some way off, though.
> Why is it faster for the optimization to check the prototype than for the normal codepath
> to walk up one level in the prototype chain?

A single shape check on the proto followed by the return of a cached value from a slot is faster than JS_GetProperty with all its machinery.

Brian, you're basically suggesting that the assert at the beginning of ListBase<LC>::nativeGet be loosened to just assert the shape and the JS_IsNativeFunction() call change from an assert to a check, right?
(Assignee)

Comment 11

6 years ago
Created attachment 567184 [details] [diff] [review]
Hacky fix

This seems to fix it by recording a property set with a setProperty op on the prototype.
Comment on attachment 567184 [details] [diff] [review]
Hacky fix

Will this work if someone changes out the proto from under you for some object that has the right things to start and then changes one of them?
[Triage Comment]
Does this bug occur in the wild?
Keywords: #relman/triage/needs-info
(In reply to Alex Keybl [:akeybl] from comment #13)
> [Triage Comment]
> Does this bug occur in the wild?

I don't yet have enough information to determine whether this should be tracked for FF10. What's the user effect here?

If we're concerned about this bug, let's try to land a low risk fix on m-c and then consider for beta.
(Assignee)

Comment 15

5 years ago
Created attachment 587435 [details] [diff] [review]
v1
Attachment #567184 - Attachment is obsolete: true
Attachment #587435 - Flags: review?(mrbkap)
(In reply to Peter Van der Beken [:peterv] from comment #15)
> Created attachment 587435 [details] [diff] [review]
> v1

peterv - can you explain what the user effect is of this bug?

Updated

5 years ago
Attachment #587435 - Flags: review?(mrbkap) → review+
(In reply to Alex Keybl [:akeybl] from comment #16)
> peterv - can you explain what the user effect is of this bug?

If a page attempts to override a function on a list by setting it on the prototype, attempts to call that function through pre-existing instances of that list type will call the original function instead of the new one.
Comment on attachment 587435 [details] [diff] [review]
v1

Actually, testing locally seems to show that this disables the proto cache altogether (since sProtoProperties doesn't know about the class getter).

Also, this only fixes this bug for a single instance of a nodelist.
Attachment #587435 - Flags: review+ → review-
(Assignee)

Comment 19

5 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #18)
> Actually, testing locally seems to show that this disables the proto cache
> altogether (since sProtoProperties doesn't know about the class getter).

Yeah, fixed by checking that the setter is |sProtoProperties[n].setter ? sProtoProperties[n].setter : InvalidateProtoShape|.

> Also, this only fixes this bug for a single instance of a nodelist.

I think it does actually, we reset the flag on the proto when we find that all the properties are the right ones. It doesn't matter how many instances we have, either a property isn't the right one and we'll just keep skipping the cache or they all are and we'll use the cache. Right?

There is a problem with multiple prototypes though (like HTMLOptionsCollection->HTMLCollection), the shape check covers both, but we only check the flag on the derived prototype (HTMLOptionsCollection).
(In reply to Blake Kaplan (:mrbkap) from comment #17)
> (In reply to Alex Keybl [:akeybl] from comment #16)
> > peterv - can you explain what the user effect is of this bug?
> 
> If a page attempts to override a function on a list by setting it on the
> prototype, attempts to call that function through pre-existing instances of
> that list type will call the original function instead of the new one.

I appreciate the technical explanation, but this doesn't help me understand whether this should be tracked for 10. What is this a regression from? Was FF9 affected? What would the /user/ impact be?
> Was FF9 affected?

No.  This is a bug in code that was added for Firefox 10.

The obvious user impact would be some sites possibly breaking, if they modify such prototypes.  I don't think there's security impact or crash potential (outside debug builds, of course).
(In reply to Peter Van der Beken [:peterv] from comment #19)
> I think it does actually, we reset the flag on the proto when we find that
> all the properties are the right ones. It doesn't matter how many instances
> we have, either a property isn't the right one and we'll just keep skipping
> the cache or they all are and we'll use the cache. Right?

Oops. You're right. I didn't realize that we wouldn't re-enable the cache until all of the properties had been restored.

> There is a problem with multiple prototypes though (like
> HTMLOptionsCollection->HTMLCollection), the shape check covers both, but we
> only check the flag on the derived prototype (HTMLOptionsCollection).

It seems like shouldCacheProtoShape can check to see if any of the great-prototypes have the flag set?
(In reply to Boris Zbarsky (:bz) from comment #21)
> The obvious user impact would be some sites possibly breaking, if they
> modify such prototypes.  I don't think there's security impact or crash
> potential (outside debug builds, of course).

Given where we are in the cycle, the fact that this hasn't yet been addressed on m-c, and that there's no known website regressions caused by this, we won't track for Firefox 10.
tracking-firefox10: ? → -
(Assignee)

Comment 24

5 years ago
Created attachment 589275 [details] [diff] [review]
v2

Boris, I'd appreciate it if you could also review the big comment to make sure it's clear enough.
Attachment #587435 - Attachment is obsolete: true
Attachment #589275 - Flags: review?(mrbkap)
Attachment #589275 - Flags: feedback?(bzbarsky)
Comment on attachment 589275 [details] [diff] [review]
v2

I think the comment is fine.

I assume that the Shape* can't be deallocated and reallocated out from under us?

Should the classname be "Object" perhaps?

Is the setProperty hook called when someone defines/redefines a property on the object via the metaobject protocol?  Or do we need to hook at least addProperty too?  For that matter, what about delProperty? In either case, would be good to add tests for this. 

r=me modulo those
Attachment #589275 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 589275 [details] [diff] [review]
v2

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

bz was right about the need to observe addProperty since Object.defineProperty won't call the setter but will change the value of a property. delProperty isn't needed since deleting a property changes the shape.

::: js/xpconnect/src/dombindings.cpp
@@ +435,5 @@
> +InvalidateProtoShape(JSContext *cx, JSObject *obj, jsid id, JSBool strict, jsval *vp)
> +{
> +    if (JSID_IS_STRING(id)) {
> +        js::SetReservedSlot(obj, 0, PrivateUint32Value(CHECK_CACHE));
> +    }

Nit: I think we avoid braces for single-line if statements now.
Attachment #589275 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 27

5 years ago
Created attachment 589968 [details] [diff] [review]
v2.1

With the addProperty hook.
Attachment #589275 - Attachment is obsolete: true
Attachment #589968 - Flags: review+
(Assignee)

Comment 28

5 years ago
Created attachment 593096 [details] [diff] [review]
v2.1 additional patch

This fixes the Dromaeo failures, we need to make sure the setter is called for our prototype objects, not for a DOM proxy object.
Attachment #593096 - Flags: review?(mrbkap)
Comment on attachment 593096 [details] [diff] [review]
v2.1 additional patch

Rookie reviewer mistake on my part. I should have seen this coming.
Attachment #593096 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/06ab89fe65ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.