Bug 81065 - WebKitTestRunner crashes when running release Tests
Summary: WebKitTestRunner crashes when running release Tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephanie Lewis
URL:
Keywords: InRadar, MakingBotsRed
Depends on:
Blocks:
 
Reported: 2012-03-13 18:03 PDT by Stephanie Lewis
Modified: 2012-03-16 17:31 PDT (History)
8 users (show)

See Also:


Attachments
patch - replace ostringstream (46.46 KB, patch)
2012-03-15 21:26 PDT, Stephanie Lewis
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
second patch (46.42 KB, patch)
2012-03-16 14:27 PDT, Stephanie Lewis
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch - this time with exports (50.48 KB, patch)
2012-03-16 16:45 PDT, Stephanie Lewis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephanie Lewis 2012-03-13 18:03:14 PDT
WebKitTestRunner crashes when running release due to the ostringstream object used to dump output.  When copying strings out of it it allocates the strings with FastMalloc and then tries to free them with system free.  I am moving WebKitTestRunner off of ostringstream.

Radar <rdar://problem/10944309>
Comment 1 Alexey Proskuryakov 2012-03-14 10:11:58 PDT
This is confusing. Is it known that there is some deep problem preventing stringstream from working in WTR?

I don't think that we should be abandoning parts of C++ any time some misconfiguration in the project breaks such part (assuming it is a misconfiguration in the project).
Comment 2 Geoffrey Garen 2012-03-14 12:31:53 PDT
> Is it known that there is some deep problem preventing stringstream from working in WTR?

Yes.

> I don't think that we should be abandoning parts of C++ any time some misconfiguration in the project breaks such part (assuming it is a misconfiguration in the project).

The problem isn't misconfiguration -- the problem is that C++ libraries that do some allocations in inlined functions and some allocations in out-of-line functions are incompatible with our FastMalloc configuration.
Comment 3 Alexey Proskuryakov 2012-03-14 14:17:37 PDT
There is nothing magic in FastMalloc, we're just overriding global new and delete operators and using STL as designed. In a correctly configured project with a correct C++ implementation, this should just work. If we believe that we're doing it correctly, then we should file a bug against clang.
Comment 4 Anders Carlsson 2012-03-14 14:18:58 PDT
Stephanie, what are the mismatched calls to operator new and operator delete? It seems to me that it's better to try to fix that problem instead of rewriting huge parts of WKTR.
Comment 5 Stephanie Lewis 2012-03-14 16:05:05 PDT
I talked to Anders and we looked at the disassembly.  The call to allocate the string is inlined in the WebKitTestRunner file and so when it allocates it uses the FastMalloc override.  The destructor is not inlined, so it uses the system free.

Anders suggested just turning off FastMalloc for WebKitTestRunner which is what I'm going to do since I'm running into encoding differences replacing ostringstream.
Comment 6 Mark Rowe (bdash) 2012-03-14 16:09:29 PDT
It is possible that could cause the opposite problem if WebKitTestRunner uses anything from WTF that inlines an allocation but not a deletion, as the code built in to JavaScriptCore would still be compiled with FastMalloc enabled.
Comment 7 Stephanie Lewis 2012-03-15 21:25:36 PDT
Based on Mark's comment I went back to replacing ostringstream.  Uploading patch
Comment 8 Stephanie Lewis 2012-03-15 21:26:21 PDT
Created attachment 132191 [details]
patch - replace ostringstream
Comment 9 WebKit Review Bot 2012-03-15 21:29:19 PDT
Attachment 132191 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner..." exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:198:  One line control clauses should not use braces.  [whitespace/braces] [4]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1229:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Build Bot 2012-03-15 21:45:57 PDT
Comment on attachment 132191 [details]
patch - replace ostringstream

Attachment 132191 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11955982
Comment 11 Geoffrey Garen 2012-03-15 22:24:18 PDT
Comment on attachment 132191 [details]
patch - replace ostringstream

r=me if you can get Windows to agree.
Comment 12 Stephanie Lewis 2012-03-16 14:27:39 PDT
Created attachment 132379 [details]
second patch

verifying I fixed Windows
Comment 13 WebKit Review Bot 2012-03-16 14:29:05 PDT
Attachment 132379 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner..." exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Build Bot 2012-03-16 15:22:24 PDT
Comment on attachment 132379 [details]
second patch

Attachment 132379 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11967454
Comment 15 Stephanie Lewis 2012-03-16 16:45:12 PDT
Created attachment 132416 [details]
patch - this time with exports

This time with exports for Windows.
Comment 16 Stephanie Lewis 2012-03-16 17:31:06 PDT
Committed http://trac.webkit.org/projects/webkit/changeset/111097