Bug 218097 - [macOS] Set preference for overridden languages in the WebContent process after entering the sandbox.
Summary: [macOS] Set preference for overridden languages in the WebContent process aft...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-22 13:19 PDT by Per Arne Vollan
Modified: 2020-11-09 10:00 PST (History)
8 users (show)

See Also:


Attachments
Patch (8.28 KB, patch)
2020-10-22 13:42 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (12.72 KB, patch)
2020-10-27 07:47 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (13.36 KB, patch)
2020-10-27 09:06 PDT, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.23 KB, patch)
2020-10-27 10:16 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.25 KB, patch)
2020-10-29 07:28 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (17.05 KB, patch)
2020-10-29 08:15 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.18 KB, patch)
2020-10-29 09:09 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (16.42 KB, patch)
2020-10-29 11:37 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (21.18 KB, patch)
2020-10-29 13:13 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (21.21 KB, patch)
2020-10-29 14:17 PDT, Per Arne Vollan
ap: review+
Details | Formatted Diff | Diff
Patch (21.16 KB, patch)
2020-10-30 10:10 PDT, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.12 KB, patch)
2020-11-02 09:59 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-10-22 13:19:26 PDT
Currently, the preference for overridden languages in the WebContent process is set before entering the sandbox, which leaves behind an open connection to opendirectoryd. This preference should be set after entering the sandbox to avoid this.
Comment 1 Radar WebKit Bug Importer 2020-10-22 13:19:48 PDT
<rdar://problem/70586545>
Comment 2 Per Arne Vollan 2020-10-22 13:42:10 PDT
Created attachment 412126 [details]
Patch
Comment 3 Alexey Proskuryakov 2020-10-23 10:35:26 PDT
Comment on attachment 412126 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Currently, the preference for overridden languages in the WebContent process is set before entering the sandbox,

How did you test that the functionality still works? This is not really covered by regression tests.
Comment 4 Per Arne Vollan 2020-10-23 13:41:18 PDT
(In reply to Alexey Proskuryakov from comment #3)
> Comment on attachment 412126 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412126&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        Currently, the preference for overridden languages in the WebContent process is set before entering the sandbox,
> 
> How did you test that the functionality still works? This is not really
> covered by regression tests.

That is a good point. This was tested manually, but I can look into creating an API test for this.
Comment 5 Alexey Proskuryakov 2020-10-23 13:44:40 PDT
How did you test this manually?
Comment 6 Per Arne Vollan 2020-10-23 14:19:27 PDT
(In reply to Alexey Proskuryakov from comment #5)
> How did you test this manually?

I verified in the debugger that the languages read from the preference 'AppleLanguages' in the UI process was written to preferences in the WebContent process. I aim to create an API test for this next, where the language preference will be overridden in memory in the UI process.

Thanks for reviewing!
Comment 7 Alexey Proskuryakov 2020-10-23 15:33:31 PDT
This amount of manual testing is not sufficient, please test end to end that the language is set and honored. 

It was very intentional that variables were set form bootstrap message, as letting any CFBundle code run permanently fixes it on the value that is in effect at the time.
Comment 8 Per Arne Vollan 2020-10-27 07:47:54 PDT
Created attachment 412421 [details]
Patch
Comment 9 Per Arne Vollan 2020-10-27 07:50:54 PDT
(In reply to Alexey Proskuryakov from comment #7)
> This amount of manual testing is not sufficient, please test end to end that
> the language is set and honored. 
> 

I have added a regression test in the latest patch.

> It was very intentional that variables were set form bootstrap message, as
> letting any CFBundle code run permanently fixes it on the value that is in
> effect at the time.

Thanks, that is very helpful information. I believe the latest patch does not alter the previous behavior in that regard.

Thanks for reviewing!
Comment 10 Per Arne Vollan 2020-10-27 09:06:25 PDT
Created attachment 412425 [details]
Patch
Comment 11 Per Arne Vollan 2020-10-27 10:16:21 PDT
Created attachment 412438 [details]
Patch
Comment 12 Alexey Proskuryakov 2020-10-27 10:50:01 PDT
Comment on attachment 412438 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/OverrideAppleLanguagesPreference.mm:39
> +    [[NSUserDefaults standardUserDefaults] setVolatileDomain:dict forName:NSArgumentDomain];

This part is good I think.

> Tools/TestWebKitAPI/Tests/WebKit/OverrideAppleLanguagesPreference.mm:47
> +        return [webView stringByEvaluatingJavaScript:@"window.internals.userPreferredLanguages()[0]"];

This test does not verify the challenging side of the proposed change, which is using correct localization resources. While I'm not 100% certain that it's wrong, I recall that CFBundle was initialized right after calling XPCServiceMain, so event handler is too late.

Please at least verify manually that the right localization is picked when setting AppleLanguages in UI process.
Comment 13 Per Arne Vollan 2020-10-27 14:19:22 PDT
(In reply to Alexey Proskuryakov from comment #12)
> Comment on attachment 412438 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412438&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/OverrideAppleLanguagesPreference.mm:39
> > +    [[NSUserDefaults standardUserDefaults] setVolatileDomain:dict forName:NSArgumentDomain];
> 
> This part is good I think.
> 
> > Tools/TestWebKitAPI/Tests/WebKit/OverrideAppleLanguagesPreference.mm:47
> > +        return [webView stringByEvaluatingJavaScript:@"window.internals.userPreferredLanguages()[0]"];
> 
> This test does not verify the challenging side of the proposed change, which
> is using correct localization resources. While I'm not 100% certain that
> it's wrong, I recall that CFBundle was initialized right after calling
> XPCServiceMain, so event handler is too late.
> 

I was able to verify in the debugger that the CFBundle localization code related to the XPC bootstrap message was run after the initialization function, so I think we should be ok.

> Please at least verify manually that the right localization is picked when
> setting AppleLanguages in UI process.

What is the best way to verify this? It seems the WebContent process has only one localization (en.lproj).

Thanks for reviewing!
Comment 14 Alexey Proskuryakov 2020-10-27 17:08:29 PDT
I usually copy localization directories from shipping Safari.
Comment 15 Per Arne Vollan 2020-10-29 07:28:51 PDT
Created attachment 412645 [details]
Patch
Comment 16 Per Arne Vollan 2020-10-29 07:34:57 PDT
(In reply to Alexey Proskuryakov from comment #14)
> I usually copy localization directories from shipping Safari.

I have confirmed locally that the correct localization is picked up with this patch.

This was tested by manually adding localized strings in Spanish and Norwegian in the Xcode IDE to WebCore.framework. I then set the system language to Spanish, and launched Safari in the debugger with the command line option '-AppleLanguages (nb)'. Safari was then started with Norwegian localization, and I confirmed in the debugger that the function CFBundleCopyLocalizedString returned Norwegian strings in the WebContent process.
Comment 17 Per Arne Vollan 2020-10-29 08:15:18 PDT
Created attachment 412651 [details]
Patch
Comment 18 Per Arne Vollan 2020-10-29 09:09:01 PDT
Created attachment 412657 [details]
Patch
Comment 19 Per Arne Vollan 2020-10-29 11:37:50 PDT
Created attachment 412669 [details]
Patch
Comment 20 Per Arne Vollan 2020-10-29 13:13:17 PDT
Created attachment 412674 [details]
Patch
Comment 21 Per Arne Vollan 2020-10-29 14:17:33 PDT
Created attachment 412684 [details]
Patch
Comment 22 Alexey Proskuryakov 2020-10-29 14:45:07 PDT
Comment on attachment 412684 [details]
Patch

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

Thank you for testing! It would be useful to check the full scenario with localizations in the API test. But it doesn't have to be part of this patch to test something that's preexisting behavior.

> LayoutTests/ChangeLog:8
> +        Add test for sandbox access to cfprefsd.daemon, and mark cfprefsd tests as failing for Mojave, since CFPrefs

This is surprising. CFPREFS_DIRECT_MODE is not enabled on Catalina either, so doesn't the failing result need to be in platform/mac-catalina?
Comment 23 Per Arne Vollan 2020-10-29 15:01:39 PDT
(In reply to Alexey Proskuryakov from comment #22)
> Comment on attachment 412684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412684&action=review
> 
> Thank you for testing! It would be useful to check the full scenario with
> localizations in the API test. But it doesn't have to be part of this patch
> to test something that's preexisting behavior.
> 
> > LayoutTests/ChangeLog:8
> > +        Add test for sandbox access to cfprefsd.daemon, and mark cfprefsd tests as failing for Mojave, since CFPrefs
> 
> This is surprising. CFPREFS_DIRECT_MODE is not enabled on Catalina either,
> so doesn't the failing result need to be in platform/mac-catalina?

Ah, yes! I will move the expectation file to the Catalina folder.

Thanks for reviewing!
Comment 24 Per Arne Vollan 2020-10-30 10:10:41 PDT
Created attachment 412766 [details]
Patch
Comment 25 Per Arne Vollan 2020-11-02 09:59:14 PST
Created attachment 412934 [details]
Patch
Comment 26 EWS 2020-11-09 10:00:45 PST
Committed r269584: <https://trac.webkit.org/changeset/269584>

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