Closed
Bug 1394146
Opened 6 years ago
Closed 6 years ago
Crash in js::jit::BaselineScript::maybeICEntryFromPCOffset
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: baffclan, Assigned: sstangl)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file, 1 obsolete file)
4.63 KB,
patch
|
sstangl
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-4ed09ff4-251b-4837-b3da-054190170826, bp-53587ba7-51fc-4ba6-8d16-138de0170826. ============================================================= Crashing Thread (0) Frame Module Signature Source 0 xul.dll js::jit::BaselineScript::maybeICEntryFromPCOffset(unsigned int) js/src/jit/BaselineJIT.cpp:669 1 xul.dll js::jit::DoGetPropFallback js/src/jit/SharedIC.cpp:2095 2 @0x2dbd2e6c2f4 3 xul.dll js::WrapperMap::lookup(js::CrossCompartmentKey const&) js/src/jscompartment.h:369 4 @0xb9b8ffce87 Application Basics: Name: Firefox Version: 57.0a1 Build ID: 20170826100418 Update Channel: nightly User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 OS: Windows_NT 10.0
Comment 1•6 years ago
|
||
Sean can you please take a look and evaluate the risk of this crash
Flags: needinfo?(sstangl)
Priority: -- → P1
Assignee | ||
Comment 2•6 years ago
|
||
If the pcOffset is at the very start of the BaselineScript, then the backwards iteration case of `BaselineScript::maybeICEntryFromPCOffset()` depends on overflow to terminate the loop. Because the crash seems to only manifest on MSVC, which has historically had problems with unsigned overflow in loops on optimized builds, I'd like to land this patch that terminates the backwards loop explicitly, to see if the crashes continue to occur. The rarity of the crashes and the crash address suggests that it's hitting this case. It's at least not a bad guess.
Flags: needinfo?(sstangl)
Attachment #8910506 -
Flags: review?(jdemooij)
Comment 3•6 years ago
|
||
Comment on attachment 8910506 [details] [diff] [review] 0001-Avoid-overflow-on-backwards-iteration-of-IC-entries.patch Review of attachment 8910506 [details] [diff] [review]: ----------------------------------------------------------------- This is nicer than relying on underflow. ::: js/src/jit/BaselineJIT.cpp @@ +674,4 @@ > // Found an IC entry with a matching PC offset. Search backward, and then > // forward from this IC entry, looking for one with the same PC offset which > // has isForOp() set. > + for (size_t i = mid; icEntry(i).pcOffset() == pcOffset; i--) { There's similar code in callVMEntryFromPCOffset. Fix that one too?
Attachment #8910506 -
Flags: review?(jdemooij) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Assignee: nobody → sstangl
Updated•6 years ago
|
Attachment #8910506 -
Attachment is obsolete: true
Updated•6 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f05ef54c84d Avoid overflow on backwards iteration of IC entries. r=jandem
Keywords: checkin-needed
![]() |
||
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9f05ef54c84d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 7•6 years ago
|
||
Low volume & too late for 56. I don't think we want this in 57 either, right?
Comment 8•6 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #7) > I don't think we want this in 57 either, right? Sean, what do you think?
Flags: needinfo?(sstangl)
Assignee | ||
Comment 9•6 years ago
|
||
It's an extremely low-volume crash, but the signature hasn't shown up since in nightlies since the patch landed. The patch is very safe to take. I'd suggest it be included in 57.
Flags: needinfo?(sstangl)
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8910964 [details] [diff] [review] 0001-Bug-1394146-Avoid-overflow-on-backwards-iteration-of.patch Approval Request Comment [Feature/Bug causing the regression]: Long-standing. [User impact if declined]: Extremely rare, low-volume crashes on Windows. [Is this code covered by automated tests?]: The crash isn't reproducible -- the patch is just a guess at the problem. [Has the fix been verified in Nightly?]: No new crashes with this signature have been reported since the patch landed. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It's a very simple mechanical change to the termination condition for for-loops -- no logic changes. It avoids unsigned overflow. [String changes made/needed]: No. No downsides to taking it, and may avoid some crashes.
Attachment #8910964 -
Flags: approval-mozilla-beta?
Comment 11•6 years ago
|
||
Comment on attachment 8910964 [details] [diff] [review] 0001-Bug-1394146-Avoid-overflow-on-backwards-iteration-of.patch Fix a crash, taking it. Should be in 57b4 (gtb tomorrow Thursday)
Attachment #8910964 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c89417bb5ce9
You need to log in
before you can comment on or make changes to this bug.
Description
•