Bug 165351 – REGRESSION(r208985): SafariForWebKitDevelopment Symbol Not Found looking for method with WTF::Optional

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 296034 [details]
[PATCH] Proposed Fix - Super Slim

[Devin Rousso]

(In reply to comment #1)
> Devin, can you test this patch?

This works for me :) Thanks!

[Yusuke Suzuki]

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.

[Joseph Pecoraro]

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

[Yusuke Suzuki]

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/209326>