Bug 136002 – JavaScriptCore should build with newer clang

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

<rdar://problem/18020616>

[David Kilzer (:ddkilzer)]

Created attachment 237174 [details]
Patch v1

[Build Bot]

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

[Build Bot]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 237174 [details]
Patch v1

Wunderbar.  Need more NULL checks added.

[David Kilzer (:ddkilzer)]

Created attachment 237593 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(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.

[Geoffrey Garen]

Comment on attachment 237593 [details]
Patch v2

r=me

[WebKit Commit Bot]

Comment on attachment 237593 [details]
Patch v2

Clearing flags on attachment: 237593

Committed r173245: <http://trac.webkit.org/changeset/173245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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.

[David Kilzer (:ddkilzer)]

(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.  :(

[WebKit Commit Bot]

Re-opened since this is blocked by bug 136533

[Brent Fulgham]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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

Yay.

[David Kilzer (:ddkilzer)]

Created attachment 237698 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[Geoffrey Garen]

Comment on attachment 237698 [details]
Patch v3

r=me

[WebKit Commit Bot]

Comment on attachment 237698 [details]
Patch v3

Clearing flags on attachment: 237698

Committed r173326: <http://trac.webkit.org/changeset/173326>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.