crash in SaveSharedScriptData

VERIFIED FIXED in Firefox 26

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: kbrosnan, Assigned: till)

Tracking

({crash, topcrash-android-armv7})

25 Branch
mozilla28
crash, topcrash-android-armv7
Points:
---

Firefox Tracking Flags

(firefox24 unaffected, firefox25 wontfix, firefox26- verified, firefox27- verified, firefox28 verified, fennec26+)

Details

(crash signature)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
May be related to bug 908478.

Updated

4 years ago
Keywords: regressionwindow-wanted, steps-wanted
Naveed, any help debugging this crash here would be welcome ;-)
Flags: needinfo?(nihsanullah)
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?
Assignee: nobody → till
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 4

4 years ago
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
Flags: needinfo?(bhackett1024)
AutoLockForExclusiveAccess only takes a lock.  No matter what it did though it couldn't change a non-NULL local variable to NULL.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
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
Flags: needinfo?(bhackett1024)
OS: Android → All
Version: 24 Branch → 25 Branch
(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.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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.
Depends on: 935903
Kevin - Can you add any summary from crash-stats? Maybe the type of device or the android version?
tracking-fennec: ? → 26+
Flags: needinfo?(kbrosnan)
(Reporter)

Comment 12

4 years ago
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.
Flags: needinfo?(kbrosnan)
(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?
Flags: needinfo?(till)
(Assignee)

Comment 14

4 years ago
Yes, that is a good idea. I'll attach a patch for uplifting in bug 935903
Flags: needinfo?(till)
(Reporter)

Comment 15

4 years ago
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.

Updated

4 years ago
tracking-firefox26: ? → +
tracking-firefox27: ? → +
(Assignee)

Comment 16

4 years ago
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.
We're going to build FF26 in that case, untracking this as it is no longer a release blocking concern.
tracking-firefox26: + → -
(Assignee)

Comment 18

4 years ago
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.
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.
tracking-firefox27: + → ?

Updated

4 years ago
Flags: needinfo?(kbrosnan)
(Reporter)

Comment 20

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(kbrosnan)
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
tracking-firefox27: ? → -
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.
Status: RESOLVED → VERIFIED
status-firefox25: affected → wontfix
status-firefox26: affected → verified
status-firefox27: affected → verified
status-firefox28: --- → verified
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: regressionwindow-wanted, steps-wanted
You need to log in before you can comment on or make changes to this bug.