Bug 69424 – REGRESSION (r96595): WTFReportBacktrace listed as the top frame in all assertion backtraces

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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.

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109795 [details]
Patch

[Gavin Peters]

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.

[Adam Roben (:aroben)]

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.

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 109803 [details]
Patch

[Gavin Peters]

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.

[Adam Roben (:aroben)]

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.

[Gavin Peters]

Committed r96708: <http://trac.webkit.org/changeset/96708>