Closed
Bug 1237501
Opened 7 years ago
Closed 7 years ago
Baseline IC for unshadowed DOM proxy gets never actually gets created
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 2•7 years ago
|
||
Attachment #8704961 -
Flags: review?(jdemooij)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 3•7 years ago
|
||
(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 :-(
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52cf63c41944
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•