Bug 157101 – All Books quirks are flakily not applied in modern WebKit (Add a preference to allow clients to turn on the "const" JS quirk)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jon Lee]

Add a preference to allow clients to turn on the "const" JS quirk

rdar://problem/25965028

[Jon Lee]

This should be coupled with removing the iBooks bundle check.

[Tim Horton]

*** Bug 223628 has been marked as a duplicate of this bug. ***

[Tim Horton]

Created attachment 423996 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 423996 [details]
Patch

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

> Source/WebCore/platform/RuntimeApplicationChecks.h:50
> +bool isInAuxiliaryProcess();

I don't know what "auxiliary" means in this context.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 423996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423996&action=review
> 
> > Source/WebCore/platform/RuntimeApplicationChecks.h:50
> > +bool isInAuxiliaryProcess();
> 
> I don't know what "auxiliary" means in this context.

Well, it’s the name used throughout WebKit2 and even in this code already :) “WebKit subprocess”

Looks like the GPUP needs to get the hosting app plumbed to it, too.

[Tim Horton]

Created attachment 424058 [details]
Patch

[Chris Dumez]

Comment on attachment 424058 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:88
> +    if (!ok)

return ok; :)

> Source/WebKit/WebProcess/WebProcess.cpp:488
> +    commonVM().setGlobalConstRedeclarationShouldThrow(parameters.shouldThrowExceptionForGlobalConstantRedeclaration);

Why don't we do the bundle check here? What's the benefit of having a configuration flag set specifically by that app and wire it all the way down to the WebProcess? The previous code seemed simpler.

[Darin Adler]

Comment on attachment 424058 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:82
> +    String clientIdentifierString = xpc_dictionary_get_string(m_initializerMessage, "client-sdk-version");

It’s not necessary to put a string into a WTF::String just to use WTF’s parsing. The charactersToUIntStrict has the same algorithm as String::toUIntStrict, but you can pass it 8-bit characters. Also, it handles null pointers. So this can be:

    auto string = xpc_dictionary_get_string(m_initializerMessage, "client-sdk-version");
    bool ok;
    clientSDKVersion = charactersToUIntStrict(string, string ? std::strlen(string) : 0, &ok);
    return ok;

[Darin Adler]

Might need a reinterpret_cast.

[Tim Horton]

Created attachment 424068 [details]
Patch

[Tim Horton]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 424058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424058&action=review
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:88
> > +    if (!ok)
> 
> return ok; :)

Good point, I stole this from the method below that needs to do one more thing :)

> > Source/WebKit/WebProcess/WebProcess.cpp:488
> > +    commonVM().setGlobalConstRedeclarationShouldThrow(parameters.shouldThrowExceptionForGlobalConstantRedeclaration);
> 
> Why don't we do the bundle check here? What's the benefit of having a
> configuration flag set specifically by that app and wire it all the way down
> to the WebProcess? The previous code seemed simpler.

My understanding is that it's generally considered incorrect to do a bundle check where SPI would do the trick (read: for apps that build alongside WebKit)

(In reply to Darin Adler from comment #8)
> Comment on attachment 424058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424058&action=review
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm:82
> > +    String clientIdentifierString = xpc_dictionary_get_string(m_initializerMessage, "client-sdk-version");
> 
> It’s not necessary to put a string into a WTF::String just to use WTF’s
> parsing. The charactersToUIntStrict has the same algorithm as
> String::toUIntStrict, but you can pass it 8-bit characters. Also, it handles
> null pointers. So this can be:
> 
>     auto string = xpc_dictionary_get_string(m_initializerMessage,
> "client-sdk-version");
>     bool ok;
>     clientSDKVersion = charactersToUIntStrict(string, string ?
> std::strlen(string) : 0, &ok);
>     return ok;

Nice!

[Tim Horton]

Created attachment 424073 [details]
Patch

[Darin Adler]

Comment on attachment 424073 [details]
Patch

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

> Source/WebKit/ChangeLog:28
> +        Add an assertion, and move the quirk to live as a setting on the process
> +        pool configuration, to be set via SPI by Books, instead of as a bundle check.

I know we have long wanted to use SPI or API for this quirk rather than a bundle ID check. I would slightly prefer API over SPI. Is this OK for iBooks? Do we need a bundle ID check to smooth over a transition period? If so, can we make one that works?

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:212
> +    xpc_dictionary_set_string(bootstrapMessage.get(), "client-sdk-version", String::number(applicationSDKVersion()).utf8().data());
>      xpc_dictionary_set_string(bootstrapMessage.get(), "process-identifier", String::number(m_launchOptions.processIdentifier.toUInt64()).utf8().data());

Seems like we ought to make a CString::number function for cases like this.

[Tim Horton]

(In reply to Darin Adler from comment #13)
> Comment on attachment 424073 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424073&action=review
> 
> > Source/WebKit/ChangeLog:28
> > +        Add an assertion, and move the quirk to live as a setting on the process
> > +        pool configuration, to be set via SPI by Books, instead of as a bundle check.
> 
> I know we have long wanted to use SPI or API for this quirk rather than a
> bundle ID check.

At some point, I'd like to get rid of all of the com.apple.* bundle checks, but this one takes precedence because it's breaking things :)

> I would slightly prefer API over SPI.

I'm pretty sure there is zero precedent for making a nitpicky JavaScript behavior configuration like this API. If we want to change course for the project as a whole, that would need a larger conversation.

> Is this OK for iBooks? Do we need a bundle ID check to smooth over a transition period? If
> so, can we make one that works?

We do not need a temporary check, because the current check is already not working :) I checked with Books folks.

[Darin Adler]

(In reply to Tim Horton from comment #14)
> > I would slightly prefer API over SPI.
>
> I'm pretty sure there is zero precedent for making a nitpicky JavaScript
> behavior configuration like this API. If we want to change course for the
> project as a whole, that would need a larger conversation.

Ideally long term the burden of argument goes the other way for future WebKit. I’d like us to think in terms of API, not SPI, whenever there’s a real requirement. Even a small one. Minimizing SPI.

[Maciej Stachowiak]

An SPI is certainly an upgrade to a bundle check.

If the "const" quirk is for compatibility with some existing books, then other EPUB readers using WebKit might want the same quirk. If it's for something truly specific to only the iBooks app (like script they inject that they didn't want to rewrite it), then maybe not.

In any case, it seems reasonable to land this without having resolved the question.

[Tim Horton]

I have been entirely unable to reproduce the debug bot's Webauthn failures (even with a macOS debug WK2 build on Big Sur, exactly like the bot), but they really do seem unique to this patch.

[Tim Horton]

Created attachment 424115 [details]
Patch

[Darin Adler]

(In reply to Maciej Stachowiak from comment #16)
> In any case, it seems reasonable to land this without having resolved the
> question.

Absolutely.

I said "long term" to try to make it clear that I wasn’t talking about "in this patch", but maybe that wasn’t obvious enough.

[Tim Horton]

I'm about ready to blow past the bot, even if only to have it fail on the post-commit bots and collect more information about the failing configuration, since I am coming up empty with ideas or repro. But let me post one patch to try to catch it first...

[Tim Horton]

Created attachment 424184 [details]
not for landing, just debugging

[Tim Horton]

Created attachment 424185 [details]
not for landing, just debugging

[Tim Horton]

OK, I think the latest test results reveal the problem; in WKTR, we're getting the bundle identifier "WebKitTestRunner" instead of "com.apple.WebKit.WebKitTestRunner" for WKTR

Because _NSGetProgName (which feeds client-identifier, which is what I used in this patch) ≠ [[NSBundle mainBundle] bundleIdentifier] (which is what we used to use) in this case.

(I don't understand why it works locally, but this makes sense).

In the interests of not going crazy I'm going to do one of two things, whichever works:

1) Use XPC API to get the bundle identifier of the UI process instead of shipping it across.

or

2) Send [[NSBundle mainBundle] bundleIdentifier] separately in the bootstrap message.

I don't think it's a good idea to change client-identifier, since it seems to be used for a few things downstream.

[Tim Horton]

Nope, still wrong. clientIdentifier comes from [[NSBundle mainBundle] bundleIdentifier]; only falls back to _NSGetProgname() if it's empty.

[Tim Horton]

OK, we need to plumb it in parallel, and read *from* WebCore::applicationBundleIdentifier(), because of the WebCore::setApplicationBundleIdentifierOverride() that WKTR uses.

[Tim Horton]

Created attachment 424219 [details]
Patch

[Tim Horton]

Comment on attachment 424219 [details]
Patch

Phew! The remaining failures on the debug bot are intermittently happening for other people's patches.

[EWS]

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

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

[Alex Christensen]

Comment on attachment 424219 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:120
> +    if (!delegate.getClientBundleIdentifier(parameters.clientBundleIdentifier))
> +        exit(EXIT_FAILURE);

This makes apps with the empty string as the bundle identifier unable to launch a WKWebView.  I accidentally made such an app and it looked like the web process was in a crash loop.  Might be a compatibility issue, might not.