Bug 237183 – [Cocoa] Allow logging to be configured by NSDefaults (without regressing launch time)

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

[Jer Noble]

[Cocoa] Allow logging to be configured by NSDefaults (without regressing launch time)

[Jer Noble]

Created attachment 453177 [details]
Patch

[Jer Noble]

Created attachment 453216 [details]
Patch

[Jer Noble]

Created attachment 453218 [details]
Patch

[Jer Noble]

Created attachment 453316 [details]
Patch

[Jer Noble]

Created attachment 453339 [details]
Patch

[Darin Adler]

Comment on attachment 453339 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:53
> +String wtfLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> +    return logString;
> +}
> +
> +String webCoreLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebCoreLogging"];
> +    return logString;
> +}
> +
> +String webKitLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebKit2Logging"];
> +    return logString;
> +}

We should not use NeverDestroyed here. That makes these non-thread-safe. There’s a reason we don’t do that in the WTF versions of these functions.

If there’s a performance problem with calling NSUserDefaults multiple times, and maybe that’s the whole point of the patch, we can cache the results, but we’d need a thread-safe way to do it, which would involve call_once and not using String for the cross-thread cached result. For example, we could use a NeverDestroyed<RetainPtr<NSString>> if we initialize it in a thread-safe manner, and create a new WTF::String each time these are called.

Unless I am missing something and these are always called on only a single thread? If so we should use the version of NeverDestroyed that includes that check perhaps?

> Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:67
> +#if !PLATFORM(COCOA)
> +String wtfLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WTF::logLevelString();
> +    return logString;
> +}
> +
> +String webCoreLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WebCore::logLevelString();
> +    return logString;
> +}
> +
> +String webKitLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WebKit::logLevelString();
> +    return logString;
> +}
> +#endif

Ditto.

[Jer Noble]

(In reply to Darin Adler from comment #6)
> Comment on attachment 453339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453339&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:53
> > +String wtfLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> > +    return logString;
> > +}
> > +
> > +String webCoreLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebCoreLogging"];
> > +    return logString;
> > +}
> > +
> > +String webKitLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebKit2Logging"];
> > +    return logString;
> > +}
> 
> We should not use NeverDestroyed here. That makes these non-thread-safe.
> There’s a reason we don’t do that in the WTF versions of these functions.
> 
> If there’s a performance problem with calling NSUserDefaults multiple times,
> and maybe that’s the whole point of the patch, we can cache the results, but
> we’d need a thread-safe way to do it, which would involve call_once and not
> using String for the cross-thread cached result. For example, we could use a
> NeverDestroyed<RetainPtr<NSString>> if we initialize it in a thread-safe
> manner, and create a new WTF::String each time these are called.
> 
> Unless I am missing something and these are always called on only a single
> thread? If so we should use the version of NeverDestroyed that includes that
> check perhaps?

Hah! I originally had std::call_once on each of these, but somehow thought that static NeverDestroyed initializers were thread safe. I'll change these back.

> > Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:67
> > +#if !PLATFORM(COCOA)
> > +String wtfLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = WTF::logLevelString();
> > +    return logString;
> > +}
> > +
> > +String webCoreLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = WebCore::logLevelString();
> > +    return logString;
> > +}
> > +
> > +String webKitLogLevelString()
> > +{
> > +    static NeverDestroyed<String> logString = WebKit::logLevelString();
> > +    return logString;
> > +}
> > +#endif
> 
> Ditto.

[Jer Noble]

(In reply to Jer Noble from comment #7)
> (In reply to Darin Adler from comment #6)
> > Comment on attachment 453339 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=453339&action=review
> > 
> > > Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:53
> > > +String wtfLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> > > +    return logString;
> > > +}
> > > +
> > > +String webCoreLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebCoreLogging"];
> > > +    return logString;
> > > +}
> > > +
> > > +String webKitLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebKit2Logging"];
> > > +    return logString;
> > > +}
> > 
> > We should not use NeverDestroyed here. That makes these non-thread-safe.
> > There’s a reason we don’t do that in the WTF versions of these functions.
> > 
> > If there’s a performance problem with calling NSUserDefaults multiple times,
> > and maybe that’s the whole point of the patch, we can cache the results, but
> > we’d need a thread-safe way to do it, which would involve call_once and not
> > using String for the cross-thread cached result. For example, we could use a
> > NeverDestroyed<RetainPtr<NSString>> if we initialize it in a thread-safe
> > manner, and create a new WTF::String each time these are called.
> > 
> > Unless I am missing something and these are always called on only a single
> > thread? If so we should use the version of NeverDestroyed that includes that
> > check perhaps?
> 
> Hah! I originally had std::call_once on each of these, but somehow thought
> that static NeverDestroyed initializers were thread safe. I'll change these
> back.

(Also, while they are currently only ever called from the main thread, but there's no guarantee they will always be in the future, so best to make them thread safe now.)

> > > Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:67
> > > +#if !PLATFORM(COCOA)
> > > +String wtfLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = WTF::logLevelString();
> > > +    return logString;
> > > +}
> > > +
> > > +String webCoreLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = WebCore::logLevelString();
> > > +    return logString;
> > > +}
> > > +
> > > +String webKitLogLevelString()
> > > +{
> > > +    static NeverDestroyed<String> logString = WebKit::logLevelString();
> > > +    return logString;
> > > +}
> > > +#endif
> > 
> > Ditto.

[Jer Noble]

Created attachment 453396 [details]
Patch

[Jer Noble]

Okay, these are all init'd behind a std::call_once or dispatchOnce.

Do we need similar modifications to emptyString() and nullString()?

[Darin Adler]

Comment on attachment 453396 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:44
> +    static NeverDestroyed<String> logString;
> +    static dispatch_once_t onceToken;
> +    dispatch_once(&onceToken, ^{
> +        logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> +    });
> +    return logString;

This is insufficiently thread-safe. Sharing a String across threads still isn’t safe even if you initialize safely. But if logString was this:

    static NeverDestroyed<RetainPtr<NSString>> logString;

Then that would be thread-safe.

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:54
> +    static NeverDestroyed<String> logString;
> +    static dispatch_once_t onceToken;
> +    dispatch_once(&onceToken, ^{
> +        logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebCoreLogging"];
> +    });
> +    return logString;

Ditto.

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:64
> +    static NeverDestroyed<String> logString;
> +    static dispatch_once_t onceToken;
> +    dispatch_once(&onceToken, ^{
> +        logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WebKit2Logging"];
> +    });
> +    return logString;

Ditto.

> Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:57
> +    static NeverDestroyed<String> logString;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
> +        logString = WTF::logLevelString();
> +    });
> +    return logString;

This isn’t thread-safe. The best solution is probably just to call WTF::logLevelString() every time.

> Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:67
> +    static NeverDestroyed<String> logString;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
> +        logString  = WebCore::logLevelString();
> +    });
> +    return logString;

Ditto.

> Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:77
> +    static NeverDestroyed<String> logString;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
> +        logString  = WebKit::logLevelString();
> +    });
> +    return logString;

Ditto.

[Jer Noble]

Created attachment 453400 [details]
Patch

[Jer Noble]

(In reply to Jer Noble from comment #12)
> Created attachment 453400 [details]
> Patch

Obsoleted because I only saw Darin's comments after uploading.

(In reply to Darin Adler from comment #11)
> Comment on attachment 453396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453396&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:44
> > +    static NeverDestroyed<String> logString;
> > +    static dispatch_once_t onceToken;
> > +    dispatch_once(&onceToken, ^{
> > +        logString = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> > +    });
> > +    return logString;
> 
> This is insufficiently thread-safe. Sharing a String across threads still
> isn’t safe even if you initialize safely. But if logString was this:
> 
>     static NeverDestroyed<RetainPtr<NSString>> logString;
> 
> Then that would be thread-safe.

Ugh. It feels like WebKit needs a thread-safe platform-independent string class. It doesn't really feel like a great result if only ports with access to NSString can safely cache these values.

Alternatively, we can go back to an earlier suggestion and mandate these are only ever called from the main thread.

[Jer Noble]

Created attachment 453404 [details]
Patch

[Jer Noble]

Created attachment 453417 [details]
Patch

[Darin Adler]

Comment on attachment 453417 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:42
> +        logString.get() = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];

Do we need the ".get()" on these lines?

[Jer Noble]

(In reply to Darin Adler from comment #16)
> Comment on attachment 453417 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453417&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:42
> > +        logString.get() = [[NSUserDefaults standardUserDefaults] stringForKey:@"WTFLogging"];
> 
> Do we need the ".get()" on these lines?

Yes. Without them, you get a compile error about failing to infer the correct type and assignment operator.

[EWS]

Committed r290654 (247927@main): <https://commits.webkit.org/247927@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89626635>