Bug 222388 – Add stubs to enable SafariForWebKitDevelopment to launch

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add stubs to enable SafariForWebKitDevelopment to launch

[Alex Christensen]

Created attachment 421462 [details]
Patch

[Alex Christensen]

Created attachment 421466 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 421466 [details]
Patch

r=mews

[Alex Christensen]

r273469

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222417

[Alexey Proskuryakov]

This somehow made all tests crash, https://build.webkit.org/#/builders/36

[Alexey Proskuryakov]

Comment on attachment 421466 [details]
Patch

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

> Source/JavaScriptCore/runtime/SymbolStubsForSafariCompatibility.mm:30
> +#if defined __MAC_OS_X_VERSION_MIN_REQUIRED && __MAC_OS_X_VERSION_MIN_REQUIRED < 110300

I'm not sure if this does what you want it to do. __MAC_OS_X_VERSION_MIN_REQUIRED is the deployment version, which I don't think gets updated for minor macOS releases.

> Source/JavaScriptCore/runtime/SymbolStubsForSafariCompatibility.mm:33
> +// Remove them after rdar://problem/74245355 is fixed and after we no longer support Big Sur.

The second part is not technically correct. It's enough to no longer support versions of Safari that don't have the fix (which will happens years earlier than not supporting the OS). We only keep WebKit trunk compatible with latest shipping Safari version.

[Alex Christensen]

Comment on attachment 421466 [details]
Patch

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

> Source/JavaScriptCore/runtime/SymbolStubsForSafariCompatibility.mm:42
> +String::String(NSString *) { }

This made tests crash in debug.  I need to make this actually call the correct code or the linker gets confused when it doesn't inline the real String(NSString*)

[Alex Christensen]

Created attachment 421559 [details]
Patch

[EWS]

Committed r273515: <https://commits.webkit.org/r273515>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421559 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74763228>

[Darin Adler]

Comment on attachment 421559 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:327
> +#if HAVE(SAFARI_FOR_WEBKIT_DEVELOPMENT_REQUIRING_EXTRA_SYMBOLS)
> +    WTF_EXPORT_PRIVATE String(NSString *);
> +#else
>      String(NSString *string)
>          : String((__bridge CFStringRef)string) { }
> +#endif

Is there any way to keep the inlining but also have the externally exported symbol for the HAVE(SAFARI_FOR_WEBKIT_DEVELOPMENT_REQUIRING_EXTRA_SYMBOLS) case?

[Alex Christensen]

My first attempt at just making a stub symbol for WTF::String::String(NSString *) seems to have worked in release builds where it is always inlined but not debug builds where it is sometimes not inlined.  We could take advantage of that, but it sounds overly complicated and fragile.  I think one function call on operating systems where the function call was always there won't hurt things too badly.