Bug 213214

Summary: Stop calling userPreferredLanguages() in the UIProcess
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, benjamin, bfulgham, cmarcelo, darin, ews-watchlist, ggaren, mmaxfield, pvollan, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=148671
https://bugs.webkit.org/show_bug.cgi?id=213488
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-06-15 14:26:04 PDT
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.
Comment 1 Chris Dumez 2020-06-15 14:26:50 PDT
<rdar://problem/64317593>
Comment 2 Chris Dumez 2020-06-15 14:28:34 PDT
Created attachment 401938 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-06-15 14:53:47 PDT
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.
Comment 4 Chris Dumez 2020-06-15 14:58:17 PDT
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.
Comment 5 Alexey Proskuryakov 2020-06-15 15:34:08 PDT
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.
Comment 6 Chris Dumez 2020-06-15 15:59:13 PDT
(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.
Comment 7 Alexey Proskuryakov 2020-06-15 16:08:04 PDT
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.
Comment 8 Alexey Proskuryakov 2020-06-15 16:17:12 PDT
> 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?
Comment 9 Chris Dumez 2020-06-15 16:22:07 PDT
(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.
Comment 10 Chris Dumez 2020-06-15 16:26:48 PDT
(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.
Comment 11 Chris Dumez 2020-06-15 17:06:23 PDT
Created attachment 401958 [details]
Patch
Comment 12 Chris Dumez 2020-06-15 17:07:09 PDT
(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.
Comment 13 Chris Dumez 2020-06-15 17:10:27 PDT
Created attachment 401959 [details]
Patch
Comment 14 Alexey Proskuryakov 2020-06-15 18:02:16 PDT
I won't have time to think about this deeply soon, but given that you made these scenarios work, the patch must be OK.
Comment 15 Per Arne Vollan 2020-06-16 08:26:19 PDT
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?
Comment 16 Chris Dumez 2020-06-16 08:27:28 PDT
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.
Comment 17 Chris Dumez 2020-06-16 08:29:25 PDT
Created attachment 402006 [details]
Patch
Comment 18 Chris Dumez 2020-06-16 08:30:08 PDT
Committed r263094: <https://trac.webkit.org/changeset/263094>
Comment 19 Chris Dumez 2020-06-16 08:32:59 PDT
(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.
Comment 20 Darin Adler 2020-06-16 09:38:27 PDT
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.
Comment 21 Chris Dumez 2020-06-16 10:08:15 PDT
(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.
Comment 22 Chris Dumez 2020-06-16 10:10:50 PDT
Reopening to attach new patch.
Comment 23 Chris Dumez 2020-06-16 10:10:51 PDT
Created attachment 402017 [details]
Patch
Comment 24 Darin Adler 2020-06-16 10:14:08 PDT
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.
Comment 25 Darin Adler 2020-06-16 10:16:11 PDT
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.
Comment 26 Chris Dumez 2020-06-16 10:22:44 PDT
Created attachment 402018 [details]
Patch
Comment 27 Chris Dumez 2020-06-16 10:24:01 PDT
Created attachment 402019 [details]
Patch
Comment 28 Darin Adler 2020-06-16 10:34:23 PDT
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!
Comment 29 Chris Dumez 2020-06-16 10:35:59 PDT
Created attachment 402020 [details]
Patch
Comment 30 Darin Adler 2020-06-16 10:36:28 PDT
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.
Comment 31 Chris Dumez 2020-06-16 10:36:29 PDT
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 :)
Comment 32 Chris Dumez 2020-06-16 10:37:36 PDT
Created attachment 402021 [details]
Patch
Comment 33 Chris Dumez 2020-06-16 10:37:58 PDT
(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.
Comment 34 EWS 2020-06-16 11:10:05 PDT
Committed r263100: <https://trac.webkit.org/changeset/263100>

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