Baseline IC for unshadowed DOM proxy gets never actually gets created

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

This is because TryAttachNativeGetAccessorPropStub does:

    bool cacheableCall = IsCacheableGetPropCall(cx, obj, holder, shape, &isScripted,
                                                isTemporarilyUnoptimizable)

which passes isDOMProxy == false implicitly.  Then IsCacheableGetPropCall does IsCacheableProtoChain which returns false because isDOMProxy is false but we're looking at a DOM proxy.

It looks like we do still manage to set up the _ion_ IC in this situation, so this mostly affects cases that don't get ion-compiled.

I looked at the blame for this code, but it's been broken for a good long while.  Possibly on initial checkin.  :(  I really wish we had some regression tests for this stuff somehow.
I've verified that passing isDOMProxy correctly here fixes the testcase linked from bug 1237451 in two ways: the baseline IC spew actually shows us setting up the ICs now and a profile and breakpoints in a debugger both show us making direct calls to GenericBindingGetter from jitcode instead of via proxy machinery.
Created attachment 8704961 [details] [diff] [review]
Actually create a baseline IC for a JSNative accessor property get that's unshadowed on a DOM proxy.  We apparently failed to ever do this
Attachment #8704961 - Flags: review?(jdemooij)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #0)
> I looked at the blame for this code, but it's been broken for a good long
> while.  Possibly on initial checkin.  :(

http://hg.mozilla.org/mozilla-central/diff/97ccac9529a6/js/src/jit/BaselineIC.cpp#l1.66

:-(
I guess the real issue is that http://hg.mozilla.org/mozilla-central/rev/9e86bfdf6bd8#l1.350 removed the isNative() check but didn't add back the isDOMProxy.
Comment on attachment 8704961 [details] [diff] [review]
Actually create a baseline IC for a JSNative accessor property get that's unshadowed on a DOM proxy.  We apparently failed to ever do this

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

Ugh. Great catch, thanks.
Attachment #8704961 - Flags: review?(jdemooij) → review+

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52cf63c41944
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.