Bug 154125

Summary: Need WTFCrash workaround for shipping SafariForWebKitDevelopment binaries.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: Web Template FrameworkAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, ggaren, hi, joepeck
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
mark.lam: review-
proposed patch 2 based on Geoff's suggestion.
none
proposed patch 3 with Joe's feedback. joepeck: review+

Description Mark Lam 2016-02-11 13:19:18 PST
Presently shipping SafariForWebKitDevelopment binaries still expect to link to a WTFCrash function.  We need to provide this function as a workaround until we can update SafariForWebKitDevelopment to use the new inlined version.
Comment 1 Mark Lam 2016-02-11 13:25:20 PST
Created attachment 271079 [details]
proposed patch.
Comment 2 BJ Burg 2016-02-11 15:06:18 PST
Comment on attachment 271079 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=271079&action=review

> Source/WTF/wtf/Assertions.cpp:507
> +// This is a workaround for presently shipping (crica early 2016) SafariForWebKitDevelopment

Nit: 'circa'
Comment 3 Mark Lam 2016-02-11 15:11:59 PST
Comment on attachment 271079 [details]
proposed patch.

I've talked with Geoff offline, and am going to re-implement this in a hopefully cleaner way.  Stay tuned.
Comment 4 Mark Lam 2016-02-11 16:25:38 PST
Created attachment 271103 [details]
proposed patch 2 based on Geoff's suggestion.
Comment 5 Joseph Pecoraro 2016-02-11 16:40:19 PST
Comment on attachment 271103 [details]
proposed patch 2 based on Geoff's suggestion.

View in context: https://bugs.webkit.org/attachment.cgi?id=271103&action=review

> Source/WTF/wtf/Assertions.h:157
> +ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void CRASH()

It doesn't seem proper to introduce a new symbol into the global namespace that is not WTF prefixed. All other symbols in WTF introduces in Assertions.h/cpp are prefixed by "WTF".

Can CRASH stay a macro, but just call to WTFCrashImpl()?

> Source/WTF/wtf/Assertions.h:177
> +#else /* not defined(NDEBUG) && OS(DARWIN) */
> +
> +WTF_EXPORT_PRIVATE NO_RETURN_DUE_TO_CRASH void WTFCrashImpl();
> +#define CRASH() WTFCrashImpl()

And it would be pretty confusing for CRASH to be a macro on some ports, but a symbol on others. It would be nice to be consistent.
Comment 6 Mark Lam 2016-02-11 16:47:10 PST
Created attachment 271107 [details]
proposed patch 3 with Joe's feedback.
Comment 7 Mark Lam 2016-02-11 16:55:13 PST
Comment on attachment 271107 [details]
proposed patch 3 with Joe's feedback.

Will commit manually.
Comment 8 Mark Lam 2016-02-11 17:00:50 PST
Thanks for the review.  Landed in r196458: <http://trac.webkit.org/r196458>.