1394146 - Crash in js::jit::BaselineScript::maybeICEntryFromPCOffset

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[baffclan]

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

[Naveed Ihsanullah [:naveed]]

Sean can you please take a look and evaluate the risk of this crash

Comment 2

[Sean Stangl [:sstangl]]

Attachment: 0001-Avoid-overflow-on-backwards-iteration-of-IC-entries.patch

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.

Comment 3

[Jan de Mooij [:jandem]]

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?

Comment 4

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-1394146-Avoid-overflow-on-backwards-iteration-of.patch

Carrying r+ from Comment 3.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f05ef54c84d
Avoid overflow on backwards iteration of IC entries. r=jandem

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9f05ef54c84d

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Low volume & too late for 56.
I don't think we want this in 57 either, right?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 9

[Sean Stangl [: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.

Comment 10

[Sean Stangl [:sstangl]]

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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c89417bb5ce9