Bug 75088 – [Qt] Consolidate layout test crash logging

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Balazs Kelemen]

For asserts (and manually called CRASH-es) we print the backtrace twice, once with WTFReportBacktrace - which just prints adresses - and once with the signal handler. Also the logic of generating the backtrace is duplicated in DRT and WTR.

[Balazs Kelemen]

Created attachment 120321 [details]
Patch

[WebKit Review Bot]

Attachment 120321 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Tools/DumpRenderTree/qt/main.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

(In reply to comment #2)
> Attachment 120321 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1
> 
> Tools/DumpRenderTree/qt/main.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This could only be fixed by including config.h in DRT's main.cpp (because of header cross-dependencies) but I don't think we should do that.

[Simon Hausmann]

Comment on attachment 120321 [details]
Patch

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

> Source/JavaScriptCore/wtf/Assertions.cpp:260
> +    globalHook();

As ironic as it may sound in the context of crash handling, but shouldn't we do a null pointer check here? :)

[Balazs Kelemen]

(In reply to comment #4)
> (From update of attachment 120321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120321&action=review
> 
> > Source/JavaScriptCore/wtf/Assertions.cpp:260
> > +    globalHook();
> 
> As ironic as it may sound in the context of crash handling, but shouldn't we do a null pointer check here? :)

Yep, definitely we should.

[Balazs Kelemen]

Created attachment 122190 [details]
null-check added

[WebKit Review Bot]

Attachment 122190 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Tools/DumpRenderTree/qt/main.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 122190 [details]
null-check added

I think the patch is a good idea, but it doesn't seem to build on mac and win yet - according to the EWS :)

[Balazs Kelemen]

Created attachment 122449 [details]
speculatively fixed mac&win build

[WebKit Review Bot]

Attachment 122449 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Tools/DumpRenderTree/qt/main.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 122449 [details]
speculatively fixed mac&win build

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

> Source/JavaScriptCore/wtf/Assertions.cpp:317
>  #endif
> +#endif
>          const int frameNumber = i - framesToSkip + 1;
>          if (mangledName || cxaDemangled)

The second #endif closes the #if OS(DARWIN) || OS(LINUX) and results in the line below to be compiled on windows, causing the win-ews failure.

[Balazs Kelemen]

Created attachment 122743 [details]
Patch

[WebKit Review Bot]

Attachment 122743 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Tools/DumpRenderTree/qt/main.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 122743 [details]
Patch

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

r=me, but please fix the style issue and add the missing "static" before landing.

> Source/JavaScriptCore/wtf/Assertions.cpp:328
> +WTFCrashHookFunction globalHook = 0;

Missing static?

[Balazs Kelemen]

(In reply to comment #14)
> (From update of attachment 122743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122743&action=review
> 
> r=me, but please fix the style issue and add the missing "static" before landing.

Unfortunately the includes do not work in alphabetical order because wtf/Assertions.h use some definitions from wtf/ExportMacros.h. Normally this is not an issue because config.h provides such headers but since this is the main.cpp of DRT we don't include config.h and I guess this is intentional. Do you think it's ok to change on that?

[Simon Hausmann]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 122743 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=122743&action=review
> > 
> > r=me, but please fix the style issue and add the missing "static" before landing.
> 
> Unfortunately the includes do not work in alphabetical order because wtf/Assertions.h use some definitions from wtf/ExportMacros.h. Normally this is not an issue because config.h provides such headers but since this is the main.cpp of DRT we don't include config.h and I guess this is intentional. Do you think it's ok to change on that?

Ah right, ok :)

So only "static" is left.

[Balazs Kelemen]

Committed r105267: <http://trac.webkit.org/changeset/105267>