Bug 81065

Summary: WebKitTestRunner crashes when running release Tests
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Tools / TestsAssignee: Stephanie Lewis <slewis>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, fpizlo, ggaren, jberlin, mrowe, slewis, webkit.review.bot
Priority: P2 Keywords: InRadar, MakingBotsRed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch - replace ostringstream
ggaren: review+, buildbot: commit-queue-
second patch
buildbot: commit-queue-
patch - this time with exports none

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