1084150 - Various Baseline IC stubs guard on post-operation shapes even when the operation changed the shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

For example, DoSetPropFallback does the set first, then outputs stubs in the scripted/jsnative setter case that use the then-current shape of the object in the shape guard.  So if the setter mutates the shape, we're guarding on the shape we have _after_ the set, not the one we have before.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, this may not be a security issue.  Just a case of generating IC stubs that we know will never match, if the setter evolved the shape of obj.

We'll also never match if the shape of holder changes during the setter, but that's likely to be rare, and also harder to detect (e.g. have to do a full property lookup before we perform the set).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't generate a getter or setter baseline IC stub if we know up front it won't match the sort of object we just did a get or set on

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't think there's a security issue here.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8506588 [details] [diff] [review]
Don't generate a getter or setter baseline IC stub if we know up front it won't match the sort of object we just did a get or set on

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

Hmmm, we certainly don't want to be doing what we're doing, because it's a correctness hole. So r=me for this to land immediately.

That being said, we could also just generate cache entries with the post-change shape, right? Can we file a followup of at least "investigate the performance implications of..." rigor, please? In general, we hope that there are lots of "similar objects" at the sites, right? For this reason, Ion even has "add property ICs" which cache the new shape at a property add site and reshape the object in jitcode. If that's useful, maybe the pre-shape thing will be as well?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't think there's a correctness hole here.  We generate the IC entry with the post-change shape but also the post-shape getter/setter.  So it'll behave correctly.  All this patch is doing is trying to make the perf a _bit_ better by not generating ICs we're pretty sure will miss.

You're right that generating based on pre-change shape would be even better; that requires grabbing more info before we do the change (things like getters and whatnot).  Or doing the IC before, but baseline also has an add-property IC, and _that_ needs to get generated after the set.  I filed bug 1089050 on this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cb0111a14b

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e4cb0111a14b