Bug 75088 - [Qt] Consolidate layout test crash logging
Summary: [Qt] Consolidate layout test crash logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-12-22 07:38 PST by Balazs Kelemen
Modified: 2012-01-18 05:36 PST (History)
2 users (show)

See Also:


Attachments
Patch (12.47 KB, patch)
2011-12-22 07:48 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
null-check added (12.57 KB, patch)
2012-01-12 01:58 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
speculatively fixed mac&win build (14.25 KB, patch)
2012-01-13 10:28 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (13.88 KB, patch)
2012-01-17 02:51 PST, Balazs Kelemen
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-12-22 07:38:51 PST
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.
Comment 1 Balazs Kelemen 2011-12-22 07:48:54 PST
Created attachment 120321 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-22 07:58:48 PST
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.
Comment 3 Balazs Kelemen 2012-01-01 11:37:55 PST
(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.
Comment 4 Simon Hausmann 2012-01-11 07:28:13 PST
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? :)
Comment 5 Balazs Kelemen 2012-01-11 09:38:03 PST
(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.
Comment 6 Balazs Kelemen 2012-01-12 01:58:04 PST
Created attachment 122190 [details]
null-check added
Comment 7 WebKit Review Bot 2012-01-12 02:00:12 PST
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.
Comment 8 Simon Hausmann 2012-01-13 07:57:58 PST
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 :)
Comment 9 Balazs Kelemen 2012-01-13 10:28:52 PST
Created attachment 122449 [details]
speculatively fixed mac&win build
Comment 10 WebKit Review Bot 2012-01-13 10:31:49 PST
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.
Comment 11 Simon Hausmann 2012-01-15 02:32:14 PST
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.
Comment 12 Balazs Kelemen 2012-01-17 02:51:37 PST
Created attachment 122743 [details]
Patch
Comment 13 WebKit Review Bot 2012-01-17 02:53:21 PST
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.
Comment 14 Simon Hausmann 2012-01-17 11:07:07 PST
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?
Comment 15 Balazs Kelemen 2012-01-18 02:27:51 PST
(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?
Comment 16 Simon Hausmann 2012-01-18 03:26:19 PST
(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.
Comment 17 Balazs Kelemen 2012-01-18 05:36:57 PST
Committed r105267: <http://trac.webkit.org/changeset/105267>