Last Comment Bug 693811 - "Assertion failure: shouldCacheProtoShape(cx, proto, &shouldCache) && shouldCache" in ListBase::nativeGet
: "Assertion failure: shouldCacheProtoShape(cx, proto, &shouldCache) && shouldC...
Status: RESOLVED FIXED
: assertion, regression, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla13
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 326633 648801
  Show dependency treegraph
 
Reported: 2011-10-11 14:16 PDT by Jesse Ruderman
Modified: 2012-02-02 08:09 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (asserts fatally when loaded) (171 bytes, text/html)
2011-10-11 14:16 PDT, Jesse Ruderman
no flags Details
stack trace (8.50 KB, text/plain)
2011-10-11 14:17 PDT, Jesse Ruderman
no flags Details
Hacky fix (5.54 KB, patch)
2011-10-14 14:33 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1 (5.08 KB, patch)
2012-01-10 12:47 PST, Peter Van der Beken [:peterv]
mrbkap: review-
Details | Diff | Review
v2 (11.27 KB, patch)
2012-01-17 13:06 PST, Peter Van der Beken [:peterv]
mrbkap: review+
bzbarsky: feedback+
Details | Diff | Review
v2.1 (11.99 KB, patch)
2012-01-19 13:17 PST, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Review
v2.1 additional patch (2.61 KB, patch)
2012-01-31 07:47 PST, Peter Van der Beken [:peterv]
mrbkap: review+
Details | Diff | Review

Description Jesse Ruderman 2011-10-11 14:16:36 PDT
Created attachment 566339 [details]
testcase (asserts fatally when loaded)

Assertion failure: shouldCacheProtoShape(cx, proto, &shouldCache) && shouldCache, at js/src/xpconnect/src/dombindings.cpp:888
Comment 1 Jesse Ruderman 2011-10-11 14:17:10 PDT
Created attachment 566340 [details]
stack trace
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2011-10-11 18:09:59 PDT
Over to peterv for investigation.
Comment 3 Peter Van der Beken [:peterv] 2011-10-12 07:41:14 PDT
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.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-12 10:25:37 PDT
Can we somehow guard on TI information instead of guarding on shape here?  Does TI even have the information we need?
Comment 5 Brian Hackett (:bhackett) 2011-10-12 11:49:38 PDT
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.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-12 12:06:39 PDT
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...
Comment 7 Jesse Ruderman 2011-10-12 16:42:04 PDT
I thought shapes did include the values of properties of type function. Did that change?
Comment 8 Jesse Ruderman 2011-10-12 16:44:11 PDT
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?
Comment 9 Brian Hackett (:bhackett) 2011-10-12 16:50:24 PDT
(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.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-12 18:36:40 PDT
> 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?
Comment 11 Peter Van der Beken [:peterv] 2011-10-14 14:33:38 PDT
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 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-14 17:57:04 PDT
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?
Comment 13 Alex Keybl [:akeybl] 2011-11-28 13:08:50 PST
[Triage Comment]
Does this bug occur in the wild?
Comment 14 Alex Keybl [:akeybl] 2011-12-26 12:27:48 PST
(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.
Comment 15 Peter Van der Beken [:peterv] 2012-01-10 12:47:54 PST
Created attachment 587435 [details] [diff] [review]
v1
Comment 16 Alex Keybl [:akeybl] 2012-01-10 13:28:22 PST
(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?
Comment 17 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-01-11 04:54:17 PST
(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 18 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-01-11 05:44:48 PST
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.
Comment 19 Peter Van der Beken [:peterv] 2012-01-11 12:11:17 PST
(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).
Comment 20 Alex Keybl [:akeybl] 2012-01-12 12:37:06 PST
(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?
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-12 12:40:52 PST
> 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).
Comment 22 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-01-13 08:17:28 PST
(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?
Comment 23 Alex Keybl [:akeybl] 2012-01-17 12:26:38 PST
(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.
Comment 24 Peter Van der Beken [:peterv] 2012-01-17 13:06:30 PST
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.
Comment 25 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-17 15:02:54 PST
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
Comment 26 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-01-18 06:27:09 PST
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.
Comment 27 Peter Van der Beken [:peterv] 2012-01-19 13:17:56 PST
Created attachment 589968 [details] [diff] [review]
v2.1

With the addProperty hook.
Comment 28 Peter Van der Beken [:peterv] 2012-01-31 07:47:57 PST
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.
Comment 29 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-01-31 08:03:07 PST
Comment on attachment 593096 [details] [diff] [review]
v2.1 additional patch

Rookie reviewer mistake on my part. I should have seen this coming.
Comment 30 Ed Morley [:emorley] 2012-02-02 08:09:08 PST
https://hg.mozilla.org/mozilla-central/rev/06ab89fe65ab

Note You need to log in before you can comment on or make changes to this bug.