Last Comment Bug 732806 - Profiler Crash [@ mozilla::FramePointerStackWalk ]
: Profiler Crash [@ mozilla::FramePointerStackWalk ]
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-04 07:48 PST by Benoit Girard (:BenWa)
Modified: 2012-03-13 04:49 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This patch should help (3.92 KB, patch)
2012-03-05 12:17 PST, Jeff Muizelaar [:jrmuizel]
ehsan: review+
Details | Diff | Splinter Review
Regression (1.08 KB, patch)
2012-03-10 20:06 PST, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-03-04 07:48:32 PST
I'm able to reliably reproduce this crash using this applet:
http://janus.astro.umd.edu/orbits/nbdy/rstar.html

Just play around, it takes me about 1-2 minutes to crash while playing with the applet and interacting with the scroll bar.

bp-21d12f9a-0c74-493d-bfd3-aabd02120304
bp-4e22924e-c48b-4cc4-abe7-85a4b2120304
f5d3ad97-f015-4955-8de5-dc30f2120304
500c0ffa-1444-4238-83c6-d62f32120304
Comment 1 Markus Stange [:mstange] 2012-03-04 12:06:56 PST
The crash address is 0x0 and the crashing line is "void **next = (void**)*bp", so it looks like bp is null there. I think all we need is a null check of bp at the beginning of the function. If bp isn't null at the beginning, it can't become null during the loop because the "next <= bp" condition would break us out of the loop.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-03-05 12:17:40 PST
Created attachment 603014 [details] [diff] [review]
This patch should help
Comment 3 :Ehsan Akhgari 2012-03-07 09:04:31 PST
Comment on attachment 603014 [details] [diff] [review]
This patch should help

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

Looks good, but please make sure that you also modify the other FramePointerStackWalk implementation I added yesterday.  r=me with that.
Comment 4 Markus Stange [:mstange] 2012-03-07 09:15:30 PST
Wouldn't the crash address in Benoit's reports be different from 0x0 if bp were too large (which is what this patch protects from)?
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-03-08 13:32:06 PST
(In reply to Markus Stange from comment #4)
> Wouldn't the crash address in Benoit's reports be different from 0x0 if bp
> were too large (which is what this patch protects from)?

Quite. It seems like we need to check that the bp we start with is sane.
Comment 6 Marco Bonardo [::mak] 2012-03-10 02:47:46 PST
https://hg.mozilla.org/mozilla-central/rev/00afd602bf72
Comment 7 Benoit Girard (:BenWa) 2012-03-10 20:06:33 PST
Created attachment 604711 [details] [diff] [review]
Regression

I think you got the stack start/end backwards, we were always returning.
Comment 9 Marco Bonardo [::mak] 2012-03-13 04:49:18 PDT
https://hg.mozilla.org/mozilla-central/rev/7de086f7aca0

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