Summary: SafariForWebKitDevelopment crashes failing to find a symbol: Starting SafariForWebKitDevelopment with DYLD_FRAMEWORK_PATH set to point to built WebKit in /Users/dcrousso/Documents/Projects/WebKit/WebKitBuild/Release. dyld: Symbol not found: __ZN9Inspector17BackendDispatcher19reportProtocolErrorEN3WTF8OptionalIlEENS0_15CommonErrorCodeERKNS1_6StringE Referenced from: /System/Library/PrivateFrameworks/WebInspector.framework/Versions/A/WebInspector Expected in: /Users/username/Documents/Projects/WebKit/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore in /System/Library/PrivateFrameworks/WebInspector.framework/Versions/A/WebInspector Notes: - The exported symbol changed when we switched from WTF::Optional to std::optional - I think we may need to provide a Legacy WTF::Optional implementation for Legacy Safari support.
Created attachment 296033 [details] [PATCH] Proposed Fix Devin, can you test this patch? We can probably slim down wtf/DeprecatedOptional quite a bit more, but this should be enough to at least verify you can run Safari.
Comment on attachment 296033 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296033&action=review > Source/WTF/wtf/DeprecatedOptional.h:27 > +// version of Safari uses. Once Safari stops using them, we should remove them. I'll address the grammar issue here. > Source/WTF/wtf/DeprecatedOptional.h:43 > + Optional() > + : m_isEngaged(false) > + { > + } Most of these are probably not necessary, in trunk we only need to if check and extract a value if there was one.
Created attachment 296034 [details] [PATCH] Proposed Fix - Super Slim
(In reply to comment #1) > Devin, can you test this patch? This works for me :) Thanks!
Comment on attachment 296034 [details] [PATCH] Proposed Fix - Super Slim View in context: https://bugs.webkit.org/attachment.cgi?id=296034&action=review Oops! Nice, r=me with suggestion. > Source/WTF/wtf/DeprecatedOptional.h:46 > + typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type m_value; We can use std::optional<T> to implement this.
Comment on attachment 296034 [details] [PATCH] Proposed Fix - Super Slim View in context: https://bugs.webkit.org/attachment.cgi?id=296034&action=review > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:94 > + // This is necessary for legacy versions of Safari. Remove it when those versions of Safari are no longer supported. This is needed for the shipping version of Safari. TOT is not required to work with “legacy” versions. > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:95 > + void reportProtocolError(WTF::DeprecatedOptional<long> relatedRequestId, CommonErrorCode, const String& errorMessage); I think this can be a private member function.
(In reply to comment #5) > Comment on attachment 296034 [details] > [PATCH] Proposed Fix - Super Slim > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296034&action=review > > Oops! Nice, r=me with suggestion. > > > Source/WTF/wtf/DeprecatedOptional.h:46 > > + typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type m_value; > > We can use std::optional<T> to implement this. One concern brought up to me in person was that the data layout should be the same as the original WTF::Optional. I am not confident in changing the implementation given all the specificity here (alignment).
(In reply to comment #7) > (In reply to comment #5) > > Comment on attachment 296034 [details] > > [PATCH] Proposed Fix - Super Slim > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=296034&action=review > > > > Oops! Nice, r=me with suggestion. > > > > > Source/WTF/wtf/DeprecatedOptional.h:46 > > > + typename std::aligned_storage<sizeof(T), std::alignment_of<T>::value>::type m_value; > > > > We can use std::optional<T> to implement this. > > One concern brought up to me in person was that the data layout should be > the same as the original WTF::Optional. I am not confident in changing the > implementation given all the specificity here (alignment). Right. So it is OK. The layout of std::optional is not guaranteed to be the same.
(In reply to comment #6) > Comment on attachment 296034 [details] > [PATCH] Proposed Fix - Super Slim > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296034&action=review > > > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:94 > > + // This is necessary for legacy versions of Safari. Remove it when those versions of Safari are no longer supported. > > This is needed for the shipping version of Safari. TOT is not required to > work with “legacy” versions. I reworded. > > Source/JavaScriptCore/inspector/InspectorBackendDispatcher.h:95 > > + void reportProtocolError(WTF::DeprecatedOptional<long> relatedRequestId, CommonErrorCode, const String& errorMessage); > > I think this can be a private member function. Yep, this looks good.
<https://trac.webkit.org/changeset/209326>