Last Comment Bug 676023 - crash js::StackIter::settleOnNewState
: crash js::StackIter::settleOnNewState
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
: 676571 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-08-02 11:08 PDT by Luke Wagner [:luke]
Modified: 2011-10-28 02:57 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

telemetry patch (2.31 KB, patch)
2011-08-02 11:09 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review
fix and a test (2.72 KB, patch)
2011-08-04 16:28 PDT, Luke Wagner [:luke]
dvander: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-08-02 11:08:18 PDT
StackIter::settleOnNewState is showing up in crash-stats for FF7 and up.  The total number is low, but its #28 for FF7.0a2.  The crash happens at part of settleOnNewState that is sensitive to outside code breaking stack invariants.  I'd like to land some telemetry patches to get more details about how its crashing and release-build-assert some stronger invariants that will hopefully turn up a reproducible test-case.
Comment 1 Luke Wagner [:luke] 2011-08-02 11:09:34 PDT
Created attachment 550130 [details] [diff] [review]
telemetry patch

Here's the first patch.  Pushing to try first; maybe we'll get lucky.
Comment 2 Luke Wagner [:luke] 2011-08-02 13:18:23 PDT
Comment on attachment 550130 [details] [diff] [review]
telemetry patch

No luck; try's clean.
Comment 3 Luke Wagner [:luke] 2011-08-02 13:30:42 PDT
Landed telemetry patch:
Comment 4 Marco Bonardo [::mak] 2011-08-03 02:20:24 PDT
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-08-03 03:22:25 PDT
Comment on attachment 550130 [details] [diff] [review]
telemetry patch

>+        JS_ASSERT(false && "About to dereference invalid slot");

Is && really what you mean?
Comment 6 Luke Wagner [:luke] 2011-08-03 10:10:01 PDT
(In reply to comment #5)
> Is && really what you mean?

It is.
Comment 7 Luke Wagner [:luke] 2011-08-04 11:58:51 PDT
*** Bug 676571 has been marked as a duplicate of this bug. ***
Comment 8 Luke Wagner [:luke] 2011-08-04 12:01:34 PDT
Reopening since the patch was telemetry.

So far, it looks like all of the crashes have moved (as of buildid 20110804030732) from StackIter::settleOnNewState to CrashIfInvalidSlot.  I'll try examining a dump on windows.
Comment 9 Luke Wagner [:luke] 2011-08-04 16:28:01 PDT
Created attachment 550878 [details] [diff] [review]
fix and a test

Looking at the OS X crash stacks (the only ones with more than one frame), all the stacks include an error that calls a debugger throw hook than then later throws an exception.

Looking at the crash dumps, the bad 'vp' is pointing into the stack frame.  This could happen if sp_ was too low.  One way sp_ could be too low is if the intepreter popped sp but did not advance the pc.  This is only checked at JSOP_CALL, so I looked there and found one such instance that crashes in CrashIfInvalidSlot.  This patch fixes the bug and hopefully will fix this crash.
Comment 10 David Anderson [:dvander] 2011-08-04 19:34:17 PDT
Comment on attachment 550878 [details] [diff] [review]
fix and a test

Review of attachment 550878 [details] [diff] [review]:

nice test case!
Comment 12 Marco Bonardo [::mak] 2011-08-05 09:09:20 PDT
Comment 13 Luke Wagner [:luke] 2011-08-08 09:42:49 PDT
Looks like that fixes it.  CrashInvalidSlot was hitting >50 times a day, getting to #12 on crash stats.  With the patch, it seems to have disappeared.
Comment 14 Luke Wagner [:luke] 2011-08-08 09:46:21 PDT
Comment on attachment 550878 [details] [diff] [review]
fix and a test

Simple patch, should decrease random crashiness for Firebug users, requesting approval for 7.
Comment 15 Johnathan Nightingale [:johnath] 2011-08-09 14:34:16 PDT
Comment on attachment 550878 [details] [diff] [review]
fix and a test

Land it this week please, so that it makes it before migration
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 16:43:17 PDT
qa- as no QA fix verification needed

Note You need to log in before you can comment on or make changes to this bug.