Bug 136002

Summary: JavaScriptCore should build with newer clang
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: JavaScriptCoreAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, bfulgham, buildbot, commit-queue, fpizlo, ggaren, joepeck, mhahnenb, mrowe, oliver, ossy, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 136533    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Patch v2
none
Patch v3 none

Description David Kilzer (:ddkilzer) 2014-08-15 16:06:31 PDT
New builds of clang triggered warnings-as-errors when compiling JavaScriptCore.  All of the issues were in the form of conspicuous uses of the |this| pointer:

Source/JavaScriptCore/API/OpaqueJSString.cpp:59:10: error: 'this' pointer cannot be null in well-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]

One use is covered by Bug 131578; fixing the rest are covered by this bug.
Comment 1 David Kilzer (:ddkilzer) 2014-08-15 16:06:49 PDT
<rdar://problem/18020616>
Comment 2 David Kilzer (:ddkilzer) 2014-08-26 14:59:05 PDT
Created attachment 237174 [details]
Patch v1
Comment 3 Build Bot 2014-08-27 08:10:16 PDT
Comment on attachment 237174 [details]
Patch v1

Attachment 237174 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5582077030301696

New failing tests:
editing/selection/select-line.html
http/tests/navigation/anchor-basic.html
editing/execCommand/delete-image-in-anchor.html
editing/selection/5195166-1.html
http/tests/navigation/javascriptlink-basic.html
http/tests/navigation/back-to-slow-frame.html
editing/deleting/delete-to-end-of-paragraph.html
http/tests/navigation/forward-and-cancel.html
editing/execCommand/move-up-down-should-skip-hidden-elements.html
editing/pasteboard/emacs-ctrl-k-with-move.html
fast/images/animated-gif-restored-from-bfcache.html
editing/selection/selection-actions.html
editing/deleting/smart-editing-disabled-mac.html
editing/deleting/smart-editing-disabled-win.html
http/tests/navigation/go-back-to-error-page.html
editing/selection/4947387.html
editing/style/make-text-writing-direction-inline-mac.html
editing/style/make-text-writing-direction-inline-win.html
editing/input/editable-container-with-word-wrap-normal.html
http/tests/navigation/error404-goback.html
editing/deleting/delete-surrogatepair.html
http/tests/navigation/error404-basic.html
editing/selection/5195166-2.html
editing/deleting/smart-delete-004.html
fast/history/go-back-to-changed-name.html
http/tests/navigation/javascriptlink-frames.html
http/tests/navigation/anchor-goback.html
editing/deleting/5300379.html
editing/selection/extend-forward-after-set-base-and-extent.html
editing/deleting/smart-delete-003.html
Comment 4 Build Bot 2014-08-27 08:10:21 PDT
Created attachment 237227 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 David Kilzer (:ddkilzer) 2014-08-27 08:21:05 PDT
Comment on attachment 237174 [details]
Patch v1

Wunderbar.  Need more NULL checks added.
Comment 6 David Kilzer (:ddkilzer) 2014-09-03 15:42:36 PDT
Created attachment 237593 [details]
Patch v2
Comment 7 David Kilzer (:ddkilzer) 2014-09-03 15:48:43 PDT
(In reply to comment #6)
> Created an attachment (id=237593) [details]
> Patch v2

Added one change to Source/WebKit2/Shared/API/c/WKString.cpp to fix the layout test crashes.
Comment 8 Geoffrey Garen 2014-09-03 17:16:33 PDT
Comment on attachment 237593 [details]
Patch v2

r=me
Comment 9 WebKit Commit Bot 2014-09-03 17:53:28 PDT
Comment on attachment 237593 [details]
Patch v2

Clearing flags on attachment: 237593

Committed r173245: <http://trac.webkit.org/changeset/173245>
Comment 10 WebKit Commit Bot 2014-09-03 17:53:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogon√°c 2014-09-03 22:53:39 PDT
(In reply to comment #9)
> (From update of attachment 237593 [details])
> Clearing flags on attachment: 237593
> 
> Committed r173245: <http://trac.webkit.org/changeset/173245>

It broke the JSC testapi tests, and now JSC tests don't run on the bots at all because of this regression.
Comment 12 Csaba Osztrogon√°c 2014-09-04 01:06:46 PDT
2014-09-04 01:01:29.768 testapi[53723:507] ERROR: Class ClassC exported more than one init family method via JSExport. Class ClassC will not have a callable JavaScript constructor function.
2014-09-04 01:01:29.770 testapi[53723:507] ERROR: Class ClassCPrime exported more than one init family method via JSExport. Class ClassCPrime will not have a callable JavaScript constructor function.
Comment 13 David Kilzer (:ddkilzer) 2014-09-04 09:19:44 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 237593 [details] [details])
> > Clearing flags on attachment: 237593
> > 
> > Committed r173245: <http://trac.webkit.org/changeset/173245>
> 
> It broke the JSC testapi tests, and now JSC tests don't run on the bots at all because of this regression.

Has it been rolled out?  I'm now surprised that EWS doesn't run JSC tests.  :(
Comment 14 WebKit Commit Bot 2014-09-04 09:27:07 PDT
Re-opened since this is blocked by bug 136533
Comment 15 Brent Fulgham 2014-09-04 10:07:45 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (From update of attachment 237593 [details] [details] [details])
> > > Clearing flags on attachment: 237593
> > > 
> > > Committed r173245: <http://trac.webkit.org/changeset/173245>
> > 
> > It broke the JSC testapi tests, and now JSC tests don't run on the bots at all because of this regression.
> 
> Has it been rolled out?  I'm now surprised that EWS doesn't run JSC tests.  :(

I just marked the roll-out bug as cq+ so that it will apply soon.
Comment 16 David Kilzer (:ddkilzer) 2014-09-04 15:29:21 PDT
(In reply to comment #12)
> 2014-09-04 01:01:29.768 testapi[53723:507] ERROR: Class ClassC exported more than one init family method via JSExport. Class ClassC will not have a callable JavaScript constructor function.
> 2014-09-04 01:01:29.770 testapi[53723:507] ERROR: Class ClassCPrime exported more than one init family method via JSExport. Class ClassCPrime will not have a callable JavaScript constructor function.

One would think that this output was the cause of the failure, but it's actually expected.  :(

The testapi process was crashing due to a NULL-dereference.
Comment 17 David Kilzer (:ddkilzer) 2014-09-04 16:10:17 PDT
Results for JSC stress tests:
    0 failures found.
    OK.

Yay.
Comment 18 David Kilzer (:ddkilzer) 2014-09-05 10:06:14 PDT
Created attachment 237698 [details]
Patch v3
Comment 19 David Kilzer (:ddkilzer) 2014-09-05 10:07:30 PDT
(In reply to comment #18)
> Created an attachment (id=237698) [details]
> Patch v3

All the tests in run-javascriptcore-tests now pass in Debug mode.
Testing Release now; will report back when completed.
Comment 20 David Kilzer (:ddkilzer) 2014-09-05 11:21:02 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=237698) [details] [details]
> > Patch v3
> 
> All the tests in run-javascriptcore-tests now pass in Debug mode.
> Testing Release now; will report back when completed.

Results for JSC stress tests:
    0 failures found.
    OK.

No issues in Release mode either.
Comment 21 Geoffrey Garen 2014-09-05 11:56:48 PDT
Comment on attachment 237698 [details]
Patch v3

r=me
Comment 22 WebKit Commit Bot 2014-09-05 12:33:44 PDT
Comment on attachment 237698 [details]
Patch v3

Clearing flags on attachment: 237698

Committed r173326: <http://trac.webkit.org/changeset/173326>
Comment 23 WebKit Commit Bot 2014-09-05 12:33:50 PDT
All reviewed patches have been landed.  Closing bug.