Bug 69424 - REGRESSION (r96595): WTFReportBacktrace listed as the top frame in all assertion backtraces
Summary: REGRESSION (r96595): WTFReportBacktrace listed as the top frame in all assert...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-05 07:39 PDT by Adam Roben (:aroben)
Modified: 2011-10-05 08:51 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.93 KB, patch)
2011-10-05 08:14 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (2.19 KB, patch)
2011-10-05 08:42 PDT, Gavin Peters
aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-10-05 07:39:59 PDT
To reproduce:

1. Insert an ASSERT_NOT_REACHED() in a commonly-called function
2. Run the program that calls the function

The backtrace you get has WTFReportBacktrace listed as the top frame.
Comment 1 Adam Roben (:aroben) 2011-10-05 07:40:29 PDT
This started happening in <http://trac.webkit.org/changeset/96595>. Looks like we need to skip two frames instead of one now, since the stack trace includes both WTFGetBacktrace and WTFReportBacktrace.
Comment 2 Gavin Peters 2011-10-05 08:00:34 PDT
Better make it an argument, defaulting to one.  Otherwise callers (who don't exist yet, but I'll add one soon) to WTFGetBacktrace will miss the possibly most important frame.
Comment 3 Gavin Peters 2011-10-05 08:14:34 PDT
Created attachment 109795 [details]
Patch
Comment 4 Gavin Peters 2011-10-05 08:15:49 PDT
Comment on attachment 109795 [details]
Patch

With apologies, here's a fix that should make WTFReportBacktrace() go back to working, and leave WTFGetBacktrace at maximum usefulness.
Comment 5 Adam Roben (:aroben) 2011-10-05 08:27:26 PDT
Comment on attachment 109795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109795&action=review

> Source/JavaScriptCore/wtf/Assertions.cpp:172
>      *size = backtrace(stack, *size);

We need to support skipFrames here, too. (If we can't implement skipFrames in WTFGetBacktrace on all ports, perhaps WTFReportBacktrace should just do the skipping on its own.)

> Source/JavaScriptCore/wtf/Assertions.cpp:202
> +    WTFGetBacktrace(samples, &frames, 2);
>  
>      for (int i = 1; i < frames; ++i) {

This doesn't seem right. We tell WTFGetBacktrace to skip two frames, but then we skip a third frame when starting the loop.
Comment 6 Gavin Peters 2011-10-05 08:34:20 PDT
Comment on attachment 109795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109795&action=review

>> Source/JavaScriptCore/wtf/Assertions.cpp:172
>>      *size = backtrace(stack, *size);
> 
> We need to support skipFrames here, too. (If we can't implement skipFrames in WTFGetBacktrace on all ports, perhaps WTFReportBacktrace should just do the skipping on its own.)

You're right.

>> Source/JavaScriptCore/wtf/Assertions.cpp:202
>>      for (int i = 1; i < frames; ++i) {
> 
> This doesn't seem right. We tell WTFGetBacktrace to skip two frames, but then we skip a third frame when starting the loop.

It isn't.  That's what I get for writing the fix too quickly; I'm testing another now.
Comment 7 Gavin Peters 2011-10-05 08:42:01 PDT
Created attachment 109803 [details]
Patch
Comment 8 Gavin Peters 2011-10-05 08:42:44 PDT
Comment on attachment 109803 [details]
Patch

Now here is a better CL, the logic is kept in WTFReportBacktrace, so callers to WTFGetBacktrace should just expect to get WTFGetBacktrace + themselves on the stack.
Comment 9 Adam Roben (:aroben) 2011-10-05 08:45:26 PDT
Comment on attachment 109803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109803&action=review

> Source/JavaScriptCore/wtf/Assertions.cpp:199
> +    static const int maxFrames = 31;
> +    static const int skipFrames = 2;
> +    void* samples[maxFrames + skipFrames];
> +    int frames = maxFrames + skipFrames;

I don't think the name maxFrames is so great anymore. We request more than maxFrames, after all. I'd suggest calling the two constants framesToShow and framesToSkip.

Other than that this all looks good.
Comment 10 Gavin Peters 2011-10-05 08:51:12 PDT
Committed r96708: <http://trac.webkit.org/changeset/96708>