Last Comment Bug 732806 - Profiler Crash [@ mozilla::FramePointerStackWalk ]
: Profiler Crash [@ mozilla::FramePointerStackWalk ]
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Jeff Muizelaar [:jrmuizel]
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image Benoit Girard (:BenWa) 2012-03-04 07:48:32 PST
I'm able to reliably reproduce this crash using this applet:

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

Comment 1 User image 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 User image Jeff Muizelaar [:jrmuizel] 2012-03-05 12:17:40 PST
Created attachment 603014 [details] [diff] [review]
This patch should help
Comment 3 User image :Ehsan Akhgari (in Taipei, laggy response time) 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 User image 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 User image 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 User image Marco Bonardo [::mak] 2012-03-10 02:47:46 PST
Comment 7 User image Benoit Girard (:BenWa) 2012-03-10 20:06:33 PST
Created attachment 604711 [details] [diff] [review]

I think you got the stack start/end backwards, we were always returning.
Comment 9 User image Marco Bonardo [::mak] 2012-03-13 04:49:18 PDT

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