931251 - crash in SaveSharedScriptData

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-196dce49-f494-421f-b4bd-de7bf2131024.
=============================================================

New crash on Firefox 25.

Currently the number 7 top crash in Firefox 25 beta 10.

No useful URLs or comments and this does not appear to be device or Android version specific.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

May be related to bug 908478.

Comment 2

[Lucas Rocha (:lucasr)]

Naveed, any help debugging this crash here would be welcome ;-)

Comment 3

[Naveed Ihsanullah [:naveed]]

Till please take a look. While I think it is unlikely to be related to your SharedScriptData code this needs someone to chase it and your name is at the top of the stack. 

What was the first date we saw this? 10/25? Can we run a bisect to narrow this down?

Comment 4

[Till Schneidereit [:till]]

Looks like the SharedScriptData in NULL when creating the hashmap lookup in SaveSharedScriptData[1]. The only explanation I have for this would be a GC. As the pointer is successfully dereferenced in the preceding line of of JSScript::fullyInitFromEmitter[2], there's really only one line that looks like it's to blame: the AutoLockForExclusiveAccess in SaveSharedScriptData[3].

Bhackett, can you take a look at this and see if you can explain it?

[1]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1475
[2]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1850
[3]: http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1473

Comment 5

[Brian Hackett [Laid off!]]

AutoLockForExclusiveAccess only takes a lock.  No matter what it did though it couldn't change a non-NULL local variable to NULL.

Comment 6

[Till Schneidereit [:till]]

Right, it was a long shot. On IRC, Bhackett told me that the line blamed in the stack might be bogus. I'll dig into SaveSharedScriptData some more.

Comment 7

[Till Schneidereit [:till]]

I don't think this is an Android-only issue. It's certainly a lot more pronounced on Android, but there are reports from other platforms, too[1]. The first report is from a v25 Nightly built in 2013-07-31, so the regression has to have been introduced before that. The only two bugs I can see as being potentially related are bug 885758 and bug 897507. For the latter, I don't see any way how the relevant patch[2] should be able to cause this.

Bhackett, that leaves your patch, with the theory being that the crash occurs when accessing cx->scriptDataTable(). Do you think that's possible?


[1]: https://crash-stats.mozilla.com/search/?signature=%3DSaveSharedScriptData&date=%3E2013-08-01&product=Firefox&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform
[2]: http://hg.mozilla.org/mozilla-central/rev/91bc683b2f45

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Till Schneidereit [:till] from comment #7)
> Bhackett, that leaves your patch, with the theory being that the crash
> occurs when accessing cx->scriptDataTable(). Do you think that's possible?

No, I don't.  Bug 885758 is a lot of code motion that isn't supposed to make any functional changes.  The change made here was:

  69.106 -    ScriptDataTable::AddPtr p = rt->scriptDataTable.lookupForAdd(l);
  69.107 +    ScriptDataTable::AddPtr p = cx->scriptDataTable().lookupForAdd(l);

With cx->scriptDataTable() returning exactly runtime->scriptDataTable.

Comment 9

[Till Schneidereit [:till]]

Ok, then I'm officially stumped. I don't see what could be crashing here and have no good ideas on how to debug it. Without STR, I don't know how to proceed.

Comment 10

[Till Schneidereit [:till]]

Given that we don't know what's going on here, I'm going to set bug 935903 as a dependency, as it's at least related to the area this crash is in. I don't see how, but there's a >0% chance that landing that fixes this crash, which we should quickly see in the stats.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Kevin - Can you add any summary from crash-stats? Maybe the type of device or the android version?

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

I loaded URLs on the most common crashing device Samsung Galaxy S2 GT-I9100. 

The devices are not that interesting nor the Android API levels and can be viewed by anyone visiting https://crash-stats.mozilla.com/report/list?product=FennecAndroid&query_search=signature&query_type=contains&reason_type=contains&range_value=28&range_unit=days&hang_type=any&process_type=any&signature=SaveSharedScriptData and viewing the "Signature summary" and "Mobile devices" expanded sections.

Looks like this may be less crashy in 26 beta 2 or people hitting this crash are not using Firefox.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Till Schneidereit [:till] from comment #10)
> Given that we don't know what's going on here, I'm going to set bug 935903
> as a dependency, as it's at least related to the area this crash is in. I
> don't see how, but there's a >0% chance that landing that fixes this crash,
> which we should quickly see in the stats.

Till do you want to try uplifting the fix in but 935903 so we can collect a week's worth of data and see if there's a reduction in volume here?

Comment 14

[Till Schneidereit [:till]]

Yes, that is a good idea. I'll attach a patch for uplifting in bug 935903

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

So looking at this again 26 might be a bit less crashy for this signature. 25 betas seem to crash 2x more often than 26 betas.

Comment 16

[Till Schneidereit [:till]]

I don't see any Fennec reports from 26.0b5 or above. If I'm reading the release calendar correctly, the patch should have been part of b6, not 5, though. Still, things don't look too bad, I think.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

We're going to build FF26 in that case, untracking this as it is no longer a release blocking concern.

Comment 18

[Till Schneidereit [:till]]

Sadly, I now see crashes for 26.0b6 and 7 :(

Nothing we can do for 26 in any case, so I'll investigate again after it has been released and we have some meaningful new numbers.

Comment 19

[bhavana bajaj [:bajaj]]

Don't see this in top-crashes for Firefox 27 Beta 1 and Beta 2 either. :kbrosnan can you please confirm ?
I'll put this on the nom list for now.

Comment 20

[Kevin Brosnan [Ex-Mozilla]]

Fixed by bug 935903. No more crashes on Firefox for Android after 26 beta 2. This looks to still crash very rarely on other platforms. Seems sensible to file a new bug for those crashes if we want to track them.

Comment 21

[u279076]

18 crashes for Firefox desktop builds, 10 of which are dupes, and 0 crashes on Firefox for Android:
https://crash-stats.mozilla.com/report/list?signature=SaveSharedScriptData&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2014-02-25+23%3A00%3A00&range_value=1#tab-reports

If we want to follow this up with another bug to resolve this on Desktop we can but the volume is not high enough that I'd prioritize it.

Comment 22

[Joshua Mitchell (Inactive)]

Issue is resolved - clearing old keywords - qa-wanted clean-up