1013996 - crash in js::irregexp::ExecuteCode(JSContext*, js::jit::JitCode*, char16_t const*, unsigned int, unsigned int, js::MatchPairs*)

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-06e789b3-9de8-4a89-8fa4-10e132140520.
=============================================================

Comment 1

[Robert Kaiser]

Aaron, looks like this is not happening in builds newer than 5/21.

Brian, did an irregexp fix for Android or ARM land at that time so we can dupe this to that bug?

Comment 3

[Robert Kaiser]

I'm reopening this as https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext%2A%2C%20js%3A%3Ajit%3A%3AJitCode%2A%2C%20char16_t%20const%2A%2C%20unsigned%20int%2C%20unsigned%20int%2C%20js%3A%3AMatchPairs%2A%29 shows that we still see crashes with this signature in 32 Betas.

Doug, you did the bug 1014402 fix, any chance you can take a look here?
Otherwise, Brian, can we get someone to dig into this?

Comment 4

[Aaron Train [:aaronmt]]

Are there any common URL's? (no access)

Comment 5

[Robert Kaiser]

(In reply to Aaron Train [:aaronmt] from comment #4)
> Are there any common URL's? (no access)

Not really. http://www.nascar.com/en_us/sprint-cup-series/leaderboard/leaderboard-live.html and facebook have 2 hits, and all the others have not even the domain in common and have 1 hit only.

This has been the #9 crash yesterday, but it took only 17 crashes per day to be in that place with the volume we have on Android beta. That said, it's been constantly over 10 crashes per day, so I'll give it a topcrash keyword for now.

Comment 6

[Douglas Crosher [:dougc]]

Is it possible to get more information from the crash dumps?

Most of the crashes are SIGBUS, which might be caused by a badly aligned load or store, and it if we knew the failing instruction it might help narrow it down.

It's probably much more complex, a bad jump or corruption of the state, and there are a small number of SIGSEGV crashes.

Is the address map stored in a crash dump, to check if the stack overflowed, and to confirm that the crash occurred in JIT code?

Comment 7

[Robert Kaiser]

We have crash dumps, David Major might be able to help you getting the info you need out of them.
David, can you take a look?

Comment 8

[(Away)]

There isn't much to go on. I'm not seeing any memory mapping info using minidump-memorylist. It looks like there are some bad addresses and the SIGBUS/SIGSEGV distinction is merely whether the bogus value happened to be properly aligned for the type of load.

Here are three SIGBUS:

 r0=53eccc98  r1=00690076  r2=ffffffda  r3=46f75804  r4=00000076  r5=00000000
 r6=00000000  r7=00000000  r8=4d4b9340  r9=53eccc70 r10=00000014 r11=4d3e72e0
r12=46f75b80  sp=4789e410  lr=00000000  pc=46d0c188 psr=80000010 N---- ARM
46d0c188 021090e7 ldr         r1,[r0,r2]

 r0=6a16eeda  r1=00000073  r2=ffffffe0  r3=5ca75804  r4=00000001  r5=00000073
 r6=00000000  r7=00000000  r8=6b1160d0  r9=6a16eeb8 r10=00000011 r11=686a2a60
r12=ffffffee  sp=5d5b9cb0  lr=00000000  pc=656b0800 psr=80000010 N---- ARM
656b0800 021090e7 ldr         r1,[r0,r2]

 r0=5485d78a  r1=0069002f  r2=fffffc94  r3=4727582c  r4=0049002f  r5=0000003a
 r6=00000000  r7=00000000  r8=52fa2c10  r9=5485d400 r10=000001c5 r11=47229520
r12=fffffc86  sp=47df9660  lr=00000000  pc=47ba7ca0 psr=80000010 N---- ARM
47ba7ca0 021090e7 ldr         r1,[r0,r2]

And two SIGSEGV:

 r0=00010008  r1=0065006d  r2=ffffff8a  r3=75875804  r4=00000001  r5=00000072
 r6=00000000  r7=00000000  r8=8f229280  r9=91a88b60 r10=0000004c r11=8a0f3f60
r12=ffffff98  sp=774bb5a8  lr=00000000  pc=7994ba88 psr=800d0010 N---- ARM
7994ba88 bc1090e1 ldrh        r1,[r0,r12]

 r0=76ba4516  r1=00740065  r2=ffffffe4  r3=69e75804  r4=00600066  r5=00000000
 r6=00000000  r7=00000000  r8=72849a00  r9=76ba44f0 r10=00000013 r11=69e29520
r12=007a0075  sp=6ab465a0  lr=00000000  pc=67b14770 psr=20000010 --C-- ARM
67b14770 0c0051e5 ldrb        r0,[r1,#-0xC]

Comment 9

[Douglas Crosher [:dougc]]

The SIGBUSs on the misaligned 'ldr' could indicate that alignment checks are actually enabled on these devices.

The irregexp code is assuming that the processor has alignment checks disabled, see:
https://hg.mozilla.org/mozilla-central/rev/713b0491788e
https://hg.mozilla.org/mozilla-central/rev/14bc5bddd097

I do not have access to any of the failing devices to test.

Can we test this on Beta at this point by trying to disable the generation of the mis-aligned loads and then checking if it reduces the crash reports?

The SIGSEGV's might be a separate issue.

(In reply to David Major [:dmajor] from comment #8)
> There isn't much to go on. I'm not seeing any memory mapping info using
> minidump-memorylist. It looks like there are some bad addresses and the
> SIGBUS/SIGSEGV distinction is merely whether the bogus value happened to be
> properly aligned for the type of load.
> 
> Here are three SIGBUS:
> 
>  r0=53eccc98  r1=00690076  r2=ffffffda  r3=46f75804  r4=00000076  r5=00000000
>  r6=00000000  r7=00000000  r8=4d4b9340  r9=53eccc70 r10=00000014 r11=4d3e72e0
> r12=46f75b80  sp=4789e410  lr=00000000  pc=46d0c188 psr=80000010 N---- ARM
> 46d0c188 021090e7 ldr         r1,[r0,r2]
> 
>  r0=6a16eeda  r1=00000073  r2=ffffffe0  r3=5ca75804  r4=00000001  r5=00000073
>  r6=00000000  r7=00000000  r8=6b1160d0  r9=6a16eeb8 r10=00000011 r11=686a2a60
> r12=ffffffee  sp=5d5b9cb0  lr=00000000  pc=656b0800 psr=80000010 N---- ARM
> 656b0800 021090e7 ldr         r1,[r0,r2]
> 
>  r0=5485d78a  r1=0069002f  r2=fffffc94  r3=4727582c  r4=0049002f  r5=0000003a
>  r6=00000000  r7=00000000  r8=52fa2c10  r9=5485d400 r10=000001c5 r11=47229520
> r12=fffffc86  sp=47df9660  lr=00000000  pc=47ba7ca0 psr=80000010 N---- ARM
> 47ba7ca0 021090e7 ldr         r1,[r0,r2]
> 
> And two SIGSEGV:
> 
>  r0=00010008  r1=0065006d  r2=ffffff8a  r3=75875804  r4=00000001  r5=00000072
>  r6=00000000  r7=00000000  r8=8f229280  r9=91a88b60 r10=0000004c r11=8a0f3f60
> r12=ffffff98  sp=774bb5a8  lr=00000000  pc=7994ba88 psr=800d0010 N---- ARM
> 7994ba88 bc1090e1 ldrh        r1,[r0,r12]
> 
>  r0=76ba4516  r1=00740065  r2=ffffffe4  r3=69e75804  r4=00600066  r5=00000000
>  r6=00000000  r7=00000000  r8=72849a00  r9=76ba44f0 r10=00000013 r11=69e29520
> r12=007a0075  sp=6ab465a0  lr=00000000  pc=67b14770 psr=20000010 --C-- ARM
> 67b14770 0c0051e5 ldrb        r0,[r1,#-0xC]

Comment 10

[Douglas Crosher [:dougc]]

Attachment: 1. Avoid unaligned accesses in ARM code.

The assumption that all ARM devices have alignment checking disabled might be wrong. This patch attempts to disable the generation of unaligned loads by irregexp to see if this solves some of the reported failures. This patch is just for beta.

If this helps then some more work will be necessary for the trunk as bug 1026438 probably introduced more unaligned loads. I'd also like to add a backend flag to indicate if unaligned loads are supported and make it a run time decision.

Comment 11

[Robert Kaiser]

(In reply to Douglas Crosher [:dougc] from comment #9)
> Can we test this on Beta at this point by trying to disable the generation
> of the mis-aligned loads and then checking if it reduces the crash reports?

Sorry, no idea what you are asking of me here. I only look at crash stats and see that issues are being taken care of...

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 8474190 [details] [diff] [review]
1. Avoid unaligned accesses in ARM code.

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

If this does end up being the problem, it would be really good to be able to detect/know whether unaligned loads are supported, as this feature is pretty important for perf on some benchmarks, particularly sunspider regexp-dna.

Comment 13

[Douglas Crosher [:dougc]]

Comment on attachment 8474190 [details] [diff] [review]
1. Avoid unaligned accesses in ARM code.

Approval Request Comment
[Feature/regressing bug #]: Bug 1011366.
[User impact if declined]: SIGBUS crashes on ARM devices with alignment checks enabled when users visit sites that make use of irregexp which appears to include some top sites such as Facebook.
[Describe test coverage new/current, TBPL]: None. We can't even reproduce. This is a probing patch to see if it resolves a class of the crashes. But here's a try run to check for regressions: https://tbpl.mozilla.org/?tree=Try&rev=02c486d95d04
[Risks and why]: Some risk as the code path is not well tested because it has been disabled from some time. The patch disables an optimization and is expected to degrade performance even on ARM devices on which the optimization would work - a more comprehensive patch can be developed if this probing patch helps.
[String/UUID change made/needed]: n/a

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm reluctant to take a speculative change like this so late in beta. Do you think it's worse to ship as is or to take the risk associated with the patch? 

As well, does this crash only impact 32? If not, can we get the patch landed somewhere else first?

Comment 15

[Douglas Crosher [:dougc]]

(In reply to Lawrence Mandel [:lmandel] from comment #14)
> I'm reluctant to take a speculative change like this so late in beta. Do you
> think it's worse to ship as is or to take the risk associated with the
> patch? 

If the problem is really that some devices have alignments checks enabled then I suggest it would be a blocker. People using those devices are not going to put up with sure crashes for six weeks, they'll be lost.
 
> As well, does this crash only impact 32? If not, can we get the patch landed
> somewhere else first?

Yes, it would impact all later versions. Just checking patches for them all.

Comment 16

[Robert Kaiser]

(In reply to Lawrence Mandel [:lmandel] from comment #14)
> As well, does this crash only impact 32? If not, can we get the patch landed
> somewhere else first?

We only see it in beta stats, probably because of the low amount of overall users on nightly and aurora for Android.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8474190 [details] [diff] [review]
1. Avoid unaligned accesses in ARM code.

beta+

Still need patches for m-c and aurora.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/093bfa0f1dee

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(In reply to Lawrence Mandel [:lmandel] from comment #17)
> Still need patches for m-c and aurora.

Doug, are you working on patches for 33+ still? We're on a new train now, so presumably this affects 35 as well.

Comment 20

[Douglas Crosher [:dougc]]

Yes, I did some work on the ARM simulator to check that the change did not break anything, but was waiting to see if the patch really addresses the SIGBUS issue.

Looking at the crash reports, the last report of a SIGBUS crash with this signature appears to be on the 20140818190554 build, there are none for the 20140826124214 build. Although there are still a few SIGSEGV crashes. Would you concur that the patch landed in comment 18 has addressed the SIGBUS issue?

If so then we need to do some more work to dynamically detect these systems so that the optimization can be disabled at runtime for only these systems.

Since the patch has not been applied to 33 we should see them return - a double check.

Comment 21

[Douglas Crosher [:dougc]]

Attachment: 2. Simulator alignment checks and irregexp aligned access support.

WIP patch.

* Adds back the alignment checks to the ARM simulator, dynamically conditional on HasAlignmentFault() which can be set with the HWCAP flag 'align'.

* Hooks up the irregexp CanReadUnaligned() method to the ARM HasAlignmentFault() method.

We still need to add reliable dynamic detection of alignment faults being enabled.

Comment 22

[Marty Rosenberg [:mjrosenb]]

Attachment: 3. AddAlignmentChecking-r0.patch

This should detect when alignment checking is enabled, and then should allow codegen to avoid generating misaligned loads.

Comment 23

[Robert Kaiser]

Looks like the signatures changed between 32 and 33 (but maybe it's different bugs):

32 only:
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext%2A%2C%20js%3A%3Ajit%3A%3AJitCode%2A%2C%20char16_t%20const%2A%2C%20unsigned%20int%2C%20unsigned%20int%2C%20js%3A%3AMatchPairs%2A%29
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+wchar_t+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+char16_t+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+char16_t+const%2A%2C+unsigned+long%2C+unsigned+long%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode&

33+:
https://crash-stats.mozilla.com/report/list?signature=js%3A%3ARegExpRunStatus+js%3A%3Airregexp%3A%3AExecuteCode%3Cchar16_t%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+char16_t+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3ARegExpRunStatus+js%3A%3Airregexp%3A%3AExecuteCode%3Cunsigned+char%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+unsigned+char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3ARegExpRunStatus+js%3A%3Airregexp%3A%3AExecuteCode%3Cunsigned+char%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+unsigned+char+const%2A%2C+unsigned+long%2C+unsigned+long%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%3Cunsigned+char%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+unsigned+char+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%3Cwchar_t%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+wchar_t+const%2A%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs%2A%29&
https://crash-stats.mozilla.com/report/list?signature=js%3A%3ARegExpRunStatus+js%3A%3Airregexp%3A%3AExecuteCode%3Cchar16_t%3E%28JSContext%2A%2C+js%3A%3Ajit%3A%3AJitCode%2A%2C+char16_t+const%2A%2C+unsigned+long%2C+unsigned+long%2C+js%3A%3AMatchPairs%2A%29&

Comment 25

[Douglas Crosher [:dougc]]

Comment on attachment 8486395 [details] [diff] [review]
3. AddAlignmentChecking-r0.patch

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

Looks good, thank you. Some issues below.

::: js/src/jit/arm/Architecture-arm.cpp
@@ +62,5 @@
> +MOZ_NEVER_INLINE  __attribute__((naked))  int
> +performAlignmentCheck(int *base, int offset) {
> +    // Use ldr.w to force the assembler to emit a 4-byte instruction, so we can
> +    // always step over it via pc += 4.
> +    asm("ldr.w r0, [r0, r1]\n\t"

The RPI assembler did not accept ldr.w. Can change this to ldrh and put a nop after it to handle thumb encoding.

@@ +68,5 @@
> +       );
> +}
> +volatile static bool hasAlignmentChecking = false;
> +
> +void handler(int signum, siginfo_t *info, void *ptr)

The Android NDK does not define ucontext. We can inline it here, see AsmJSSignalHanders.cpp.

@@ +85,5 @@
> +    newact.sa_sigaction = handler;
> +    newact.sa_flags = SA_SIGINFO;
> +    if (sigaction(SIGSEGV, &newact, &oldSEGVact) < 0)
> +        return true;
> +    if (sigaction(SIGBUS, &newact, &oldBUSact)) {

To be consisten could this test for '< 0'.

@@ +86,5 @@
> +    newact.sa_flags = SA_SIGINFO;
> +    if (sigaction(SIGSEGV, &newact, &oldSEGVact) < 0)
> +        return true;
> +    if (sigaction(SIGBUS, &newact, &oldBUSact)) {
> +        sigaction(SIGSEGV, &oldSEGVact, nullptr);

Could the return value be checked as below, and crash if it fails.

@@ +283,5 @@
>              if (strstr(buf, "ARMv7"))
>                  flags |= HWCAP_ARMv7;
>          }
>      }
> +    if (alignmentCheck()) {

There are some concerns with installing and uninstalling signal handers in this path. InitARMFlags() is called from InitializeIon() which is called from JS_Init() which is run early but is it early enough.

Comment 26

[Douglas Crosher [:dougc]]

Attachment: 3. Detect if misaligned loads generate an exception at runtime.

Possible changes.

Tried to enable the alignment checks in the Flame kernel, but it causes the kernel to crash and boot cycle.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Doug, it is a bit unclear to me what is the plan with regards to 33. Could you explain me? Thanks

Comment 28

[Douglas Crosher [:dougc]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #27)
> Doug, it is a bit unclear to me what is the plan with regards to 33. Could
> you explain me? Thanks

We are collecting data. Patch 1 should be landed on 33, 34, and 35 soon. We can again collect data and see if the SIGBUS crashes stop. If so then we can work on a test to dynamically enabled the optimization and it can ride the train.

Requesting patch 1 be landed on m-c, here's a try build:
https://tbpl.mozilla.org/?tree=Try&rev=4bad76f7150b

This patch has already landed on release 32, and do we still need to request approval for landing it on beta 33, and aurora 34?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98379f3068e3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Yes, you'll still need approval to land patch 1 on Beta33 and Aurora34.

Comment 31

[Douglas Crosher [:dougc]]

Comment on attachment 8474190 [details] [diff] [review]
1. Avoid unaligned accesses in ARM code.

[Feature/regressing bug #]: Bug 1011366.
[User impact if declined]: SIGBUS crashes on ARM devices that have alignment checks enabled when users visit sites that make use of irregexp which appears to include some top sites such as Facebook.
[Describe test coverage new/current, TBPL]: This patch landed on release 32, and is inbound on m-c. We can not reproduce it but have been collecting data from the crash reports and there is some evidence that it eliminated the SIGBUS crashes - we will check the crash reports again after this patch lands.
[Risks and why]: Low risk. It landed on release 32 and we have also enabled the alignment checks in the ARM simulator and verified that it still passes the jit-tests, see patch 2.
[String/UUID change made/needed]: n/a

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ada911a09289
https://hg.mozilla.org/releases/mozilla-beta/rev/5e2a5b6c7a0d

Setting the status flags to fixed for tracking purposes. Feel free to set the back to affected if and when there are future patches in need of uplift.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/98379f3068e3

Comment 34

[Douglas Crosher [:dougc]]

Attachment: 2. Simulator alignment checks and irregexp aligned access support.

Rebase. Have been meaning to get this one reviewed, to help catch regressions.

This patch moves HasAlignmentFault() into a header, alone with some of the flags, so that the simulator can inline these. I am assuming that simulator load and store paths are hot, so we probably need these inlined.

Some of the instructions always generate an alignment fault, and these should really use separate paths that are not conditional on HasAlignmentFault(), but can we leave that to another patch.

The patch also connects up the irregexp code to HasAlignmentFault(). In future we hope to dynamically detect if alignment checks are enabled, see patch 3.

Comment 35

[Douglas Crosher [:dougc]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=220756d429eb

Comment 36

[Douglas Crosher [:dougc]]

Try build of patch 2 looks good. Some orange but it does not appear related to this patch. Requesting checkin.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c54a5bded1ba

BTW, we're a little over a week away from the next merge. This bug has now spread out across multiple release cycles, which is making tracking a royal PITA. Can we consider closing this out and filing new bugs for remaining work?

Comment 38

[Douglas Crosher [:dougc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c54a5bded1ba
> 
> BTW, we're a little over a week away from the next merge. This bug has now
> spread out across multiple release cycles, which is making tracking a royal
> PITA. Can we consider closing this out and filing new bugs for remaining
> work?

Yes, patch 3 has been moved to bug 1077448. Thank you for checking in that patches.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c54a5bded1ba

Comment 40

[u279076]

I don't see any instances of these signatures being reported against Fennec 35.0a1, 34.0a2, nor 33.0 Beta in the last week. There are 8 reports[1] with Fennec 32.0.3, though I suspect that volume is not worth investigation.

1. https://crash-stats.mozilla.com/report/list?signature=js%3A%3Airregexp%3A%3AExecuteCode%28JSContext*%2C+js%3A%3Ajit%3A%3AJitCode*%2C+char16_t+const*%2C+unsigned+int%2C+unsigned+int%2C+js%3A%3AMatchPairs*%29&product=FennecAndroid&query_type=contains&range_unit=weeks&process_type=any&version=FennecAndroid%3A32.0.3&hang_type=any&date=2014-10-07+22%3A00%3A00&range_value=1#reports