Jesse Ruderman Reporter
	Description • 13 years ago

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

	Johnny Stenback (:jst)
	Comment 2 • 13 years ago

Over to peterv for investigation.

	Peter Van der Beken [:peterv] Assignee
	Comment 3 • 13 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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 13 years ago

Can we somehow guard on TI information instead of guarding on shape here?  Does TI even have the information we need?

	Brian Hackett [Laid off!]
	Comment 5 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 13 years ago

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...

	Jesse Ruderman Reporter
	Comment 7 • 13 years ago

I thought shapes did include the values of properties of type function. Did that change?

	Jesse Ruderman Reporter
	Comment 8 • 13 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?

	Brian Hackett [Laid off!]
	Comment 9 • 13 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 13 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?

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?

	Peter Van der Beken [:peterv] Assignee
	Comment 11 • 13 years ago

This seems to fix it by recording a property set with a setProperty op on the prototype.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 13 years ago

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?

	Alex Keybl [:akeybl]
	Comment 13 • 13 years ago

[Triage Comment]
Does this bug occur in the wild?

	Alex Keybl [:akeybl]
	Comment 14 • 13 years ago

(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.

	Alex Keybl [:akeybl]
	Comment 16 • 13 years ago

(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?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 17 • 13 years ago

(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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 18 • 13 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 19 • 13 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).

	Alex Keybl [:akeybl]
	Comment 20 • 13 years ago

(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?

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 13 years ago

> 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).

	Blake Kaplan (:mrbkap) (inactive)
	Comment 22 • 13 years ago

(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?

	Alex Keybl [:akeybl]
	Comment 23 • 13 years ago

(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.

	Peter Van der Beken [:peterv] Assignee
	Comment 24 • 13 years ago

Boris, I'd appreciate it if you could also review the big comment to make sure it's clear enough.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 13 years ago

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

	Blake Kaplan (:mrbkap) (inactive)
	Comment 26 • 13 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 27 • 13 years ago

With the addProperty hook.

	Peter Van der Beken [:peterv] Assignee
	Comment 28 • 13 years ago

This fixes the Dromaeo failures, we need to make sure the setter is called for our prototype objects, not for a DOM proxy object.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 29 • 13 years ago

Comment on attachment 593096 [details] [diff] [review]
v2.1 additional patch

Rookie reviewer mistake on my part. I should have seen this coming.

	Ed Morley [:emorley]
	Comment 30 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/06ab89fe65ab