Bug 213214 – Stop calling userPreferredLanguages() in the UIProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Stop calling userPreferredLanguages() in the UIProcess since this can be slow and may keep the main thread busy. Nowadays, the WebProcess is able to call it by itself anyway. The sandbox is allowing it.

[Chris Dumez]

<rdar://problem/64317593>

[Chris Dumez]

Created attachment 401938 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 401938 [details]
Patch

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

> Source/WTF/ChangeLog:3
> +        Stop calling userPreferredLanguages() in the UIProcess

Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option.

It's not visible in this patch whether that goes in overrideLanguages already.

[Chris Dumez]

Comment on attachment 401938 [details]
Patch

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

>> Source/WTF/ChangeLog:3
>> +        Stop calling userPreferredLanguages() in the UIProcess
> 
> Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option.
> 
> It's not visible in this patch whether that goes in overrideLanguages already.

I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess.

I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that.

[Alexey Proskuryakov]

Comment on attachment 401938 [details]
Patch

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

>>> Source/WTF/ChangeLog:3
>>> +        Stop calling userPreferredLanguages() in the UIProcess
>> 
>> Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option.
>> 
>> It's not visible in this patch whether that goes in overrideLanguages already.
> 
> I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess.
> 
> I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that.

We perform work to make CFLocale languages in WebProcess match UI process, but it's very much not obvious if they are going to return the same result always. I would object to landing this at the current level of understanding.

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 401938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401938&action=review
> 
> >>> Source/WTF/ChangeLog:3
> >>> +        Stop calling userPreferredLanguages() in the UIProcess
> >> 
> >> Does this work when the preference is not global? One can write NSUserDefaults for just the UI process bundle ID, or even from command line with -AppleLanguages option.
> >> 
> >> It's not visible in this patch whether that goes in overrideLanguages already.
> > 
> > I do not expect any behavior change here. We merely call userPreferredLanguages() from the WebProcess instead of calling it in the UIProcess and sending it over IPC. userPreferredLanguages() just returns the overrides if set otherwise ends up calling CFLocaleCopyPreferredLanguages() on Cocoa ports. Calling CFLocaleCopyPreferredLanguages() from the WebProcess works (As confirmed by Per and validated locally) so there is no reason we need to call it from the UIProcess.
> > 
> > I don't know what overrideLanguages is used for but my patch keeps getting them from the configuration and sending it over IPC. I did not change that.
> 
> We perform work to make CFLocale languages in WebProcess match UI process,
> but it's very much not obvious if they are going to return the same result
> always. I would object to landing this at the current level of understanding.

I do not understand what you mean by "We perform work to make CFLocale languages in WebProcess match UI process". All this patch does is call CFLocaleCopyPreferredLanguages() from the WebProcess instead of the UIProcess (like it used to a while back).

I change language in system Preferences on both macOS and iOS and navigator.languages returns the right languages.

[Alexey Proskuryakov]

I explained it in the first comment why testing just the global defaults is insufficient. We need to make this work in all these situations:

1. AppleLanguages user default is different between WebContent and UI process. 

2. Available localizations are different between WebContent and UI process.

CFLocaleCopyPreferredLanguages takes both defaults and localizations into consideration. There is no "just" about moving it between processes.

[Alexey Proskuryakov]

> like it used to a while back

I'm not sure what exactly you mean by "a while back". Was that a recent change? Do you know which one?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #7)
> I explained it in the first comment why testing just the global defaults is
> insufficient. We need to make this work in all these situations:
> 
> 1. AppleLanguages user default is different between WebContent and UI
> process. 
> 
> 2. Available localizations are different between WebContent and UI process.
> 
> CFLocaleCopyPreferredLanguages takes both defaults and localizations into
> consideration. There is no "just" about moving it between processes.

Ok, I think I am able to reproduce the issue you were concerned about. I did:
dvdo defaults write com.apple.Safari AppleLanguages '("en-US", "fr-FR")'

In the UIProcess, I see both but in the WebContent process I only see en-US. I did not know this was a supported mechanism to change the locale of Safari.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Alexey Proskuryakov from comment #7)
> > I explained it in the first comment why testing just the global defaults is
> > insufficient. We need to make this work in all these situations:
> > 
> > 1. AppleLanguages user default is different between WebContent and UI
> > process. 
> > 
> > 2. Available localizations are different between WebContent and UI process.
> > 
> > CFLocaleCopyPreferredLanguages takes both defaults and localizations into
> > consideration. There is no "just" about moving it between processes.
> 
> Ok, I think I am able to reproduce the issue you were concerned about. I did:
> dvdo defaults write com.apple.Safari AppleLanguages '("en-US", "fr-FR")'
> 
> In the UIProcess, I see both but in the WebContent process I only see en-US.
> I did not know this was a supported mechanism to change the locale of Safari.

There is code in XPCServiceMain.mm that tries to deal with AppleLanguages. Did not seem to work for me. Will try and figure out why.

[Chris Dumez]

Created attachment 401958 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #11)
> Created attachment 401958 [details]
> Patch

This iteration forwards AppleLanguages UserDefaults from the UIProcess to the WebContent process to keep it working.

[Chris Dumez]

Created attachment 401959 [details]
Patch

[Alexey Proskuryakov]

I won't have time to think about this deeply soon, but given that you made these scenarios work, the patch must be OK.

[Per Arne Vollan]

Comment on attachment 401959 [details]
Patch

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

R=me. After this patch, can the language related code in XPCServiceMain.mm be removed?

> Source/WebKit/UIProcess/WebProcessPool.cpp:1085
> +    userPreferredLanguages();

Is this call needed?

[Chris Dumez]

Comment on attachment 401959 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1085
>> +    userPreferredLanguages();
> 
> Is this call needed?

Hahah lol, I had added it for debugging purposes :) Will drop. Thanks for catching.

[Chris Dumez]

Created attachment 402006 [details]
Patch

[Chris Dumez]

Committed r263094: <https://trac.webkit.org/changeset/263094>

[Chris Dumez]

(In reply to Per Arne Vollan from comment #15)
> Comment on attachment 401959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401959&action=review
> 
> R=me. After this patch, can the language related code in XPCServiceMain.mm
> be removed?

No, my patch actually relies on the code in XPCServiceMain to forward the AppleLanguages UserDefaults from the UIProcess to the WebProcess.

[Darin Adler]

Comment on attachment 401959 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:261
> +        Vector<String> overrideLanguages;
> +        overrideLanguages.reserveInitialCapacity([languages count]);
> +        for (NSString *language in languages)
> +            overrideLanguages.uncheckedAppend(language);
> +        return overrideLanguages;

The makeNSArray function in VectorCocoa.h does this for you; you don’t have to write it out.

[Chris Dumez]

(In reply to Darin Adler from comment #20)
> Comment on attachment 401959 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401959&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:261
> > +        Vector<String> overrideLanguages;
> > +        overrideLanguages.reserveInitialCapacity([languages count]);
> > +        for (NSString *language in languages)
> > +            overrideLanguages.uncheckedAppend(language);
> > +        return overrideLanguages;
> 
> The makeNSArray function in VectorCocoa.h does this for you; you don’t have
> to write it out.

I assume you mean makeVector<String>(). It looks great, I did not know it existed. I will fix shortly, thanks.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 402017 [details]
Patch

[Darin Adler]

Comment on attachment 402017 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253
>      NeverDestroyed<Vector<String>> overrideLanguages = []() {

Just noticed that "static" is missing here, so we are probably leaking vectors.

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:255
>          NSArray *languages = [[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"];
> -        if (!languages)
> -            return Vector<WTF::String> { };
> -
> -        Vector<String> overrideLanguages;
> -        overrideLanguages.reserveInitialCapacity([languages count]);
> -        for (NSString *language in languages)
> -            overrideLanguages.uncheckedAppend(language);
> -        return overrideLanguages;
> +        return makeVector<String>(languages);

Could go one step further and make this one line.

[Darin Adler]

Comment on attachment 402017 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253
>>      NeverDestroyed<Vector<String>> overrideLanguages = []() {
> 
> Just noticed that "static" is missing here, so we are probably leaking vectors.

Should probably be static const NeverDestroyed.

[Chris Dumez]

Created attachment 402018 [details]
Patch

[Chris Dumez]

Created attachment 402019 [details]
Patch

[Darin Adler]

Comment on attachment 402019 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:255
> +    static const NeverDestroyed<Vector<String>> overrideLanguages = []() {
> +        return makeVector<String>([[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"]);
>      }();

Oh, don’t even need a lambda now!

[Chris Dumez]

Created attachment 402020 [details]
Patch

[Darin Adler]

Comment on attachment 402019 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:256
>      return overrideLanguages.get();

Not sure we need to write the get() here; I think it works without it.

[Chris Dumez]

Comment on attachment 402020 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:253
> +    static const NeverDestroyed<Vector<String>> overrideLanguages = makeVector<String>([[NSUserDefaults standardUserDefaults] valueForKey:@"AppleLanguages"]);

Well, that is much more concise :)

[Chris Dumez]

Created attachment 402021 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #30)
> Comment on attachment 402019 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402019&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm:256
> >      return overrideLanguages.get();
> 
> Not sure we need to write the get() here; I think it works without it.

It is indeed not needed, I dropped it.

[EWS]

Committed r263100: <https://trac.webkit.org/changeset/263100>

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