Closed Bug 1394146 Opened 6 years ago Closed 6 years ago

Crash in js::jit::BaselineScript::maybeICEntryFromPCOffset


(Core :: JavaScript Engine, defect, P1)

Windows 10



Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed


(Reporter: baffclan, Assigned: sstangl)


(Keywords: crash)

Crash Data


(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-4ed09ff4-251b-4837-b3da-054190170826,

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
Sean can you please take a look and evaluate the risk of this crash
Flags: needinfo?(sstangl)
Priority: -- → P1
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 on attachment 8910506 [details] [diff] [review]

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+
Keywords: checkin-needed
Assignee: nobody → sstangl
Attachment #8910506 - Attachment is obsolete: true
Pushed by
Avoid overflow on backwards iteration of IC entries. r=jandem
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Low volume & too late for 56.
I don't think we want this in 57 either, right?
(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)
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)
Comment on attachment 8910964 [details] [diff] [review]

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 on attachment 8910964 [details] [diff] [review]

Fix a crash, taking it.
Should be in 57b4 (gtb tomorrow Thursday)
Attachment #8910964 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.