Closed Bug 869040 Opened 7 years ago Closed 7 years ago

Ion get ICs seem to be broken with non-overridebuiltins named gets

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 + fixed
firefox23 + fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(3 files)

In the attached testcase, the document should say:

  present1
  present2

but instead says:

  undefined
  present2

This is weirdly working in Firefox 19 and 20 but failing in the nighties they were built from.  I haven't tried in beta or aurora builds yet.
Attached file Testcase
Looks like the testcase works correctly in Beta 21 but not Aurora 22 as of today.
Keywords: regression
So looking at today's code (not sure how relevant it is to Aurora)....

We land in TryAttachNativeGetPropStub and check for shadowing.  This is not an overridebuiltins, so shadows comes out as DoesntShadow.

Then we set checkObj to the proto of the object and keep going.

The lookup on checkObj of course returns a null shape.

Now we call DetermineGetPropKind with the checkObj.  We have no shape, and our proto chain is all plain-vanilla, so IsCacheableNoProperty returns true.  So we set readSlot to true and return true from DetermineGetPropKind and then go on to do an attachReadSlot on the obj.  Which will never get us anything useful, since it's a proxy, of course.

Basically, we seem to generate a missing-property ic based on the lack of a property on our proto chain, but for that case we have to consider own properties too: not shadowing just says that _if_ we find something on the proto we'll keep finding it there, not that if we don't find anything on the proto then we have nothing.
Blocks: 852092
I just tried this on a nightly from the end of last week, and it returns correct results.  Is this affecting aurora only then?
It's affecting my nightly build for sure (and in fact every nightly I've tried).
Affects my nightly/aurora as well, tracking and tagging to find the regression window here.
Blocks: 869796
Blocks: 868834
Peter, are you working on this or should I take it?
Flags: needinfo?(peterv)
Assignee: nobody → bzbarsky
Flags: needinfo?(peterv)
Whiteboard: [need review]
Attachment #747033 - Flags: review?(kvijayan) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/b61e721b78af
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla23
Comment on attachment 747090 [details] [diff] [review]
Aurora version

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 852092 
User impact if declined: Broken behavior with some DOM objects, but only once
   the script has run for a bit.
Testing completed (on m-c, etc.): Passes the test, fixes the sites that are
   broken
Risk to taking this patch (and alternatives if risky): Risk is about as low as patches to the JIT can get, I think.  The other option is to just not fix this
   and let some things be broken, but that seems worse to me.  Note that more
   things are broken on Aurora than in 20 or 21 since we've converted more
   objects to WebIDL.
String or IDL/UUID changes made by this patch: None
Attachment #747090 - Attachment description: Fix ion IC for non-overridebuiltins named gets on ListBase proxies to not cache lack of a property when it's just missing on the prototype. → Aurora version
Attachment #747090 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b61e721b78af
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #747090 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 747090 [details] [diff] [review]
Aurora version

Approving for Beta 22
Attachment #747090 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta+
Depends on: 885660
No longer depends on: 885660
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.