Bug 218097 – [macOS] Set preference for overridden languages in the WebContent process after entering the sandbox.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Radar WebKit Bug Importer]

<rdar://problem/70586545>

[Per Arne Vollan]

Created attachment 412126 [details]
Patch

[Alexey Proskuryakov]

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.

[Per Arne Vollan]

(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.

[Alexey Proskuryakov]

How did you test this manually?

[Per Arne Vollan]

(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!

[Alexey Proskuryakov]

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.

[Per Arne Vollan]

Created attachment 412421 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 412425 [details]
Patch

[Per Arne Vollan]

Created attachment 412438 [details]
Patch

[Alexey Proskuryakov]

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.

[Per Arne Vollan]

(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!

[Alexey Proskuryakov]

I usually copy localization directories from shipping Safari.

[Per Arne Vollan]

Created attachment 412645 [details]
Patch

[Per Arne Vollan]

(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.

[Per Arne Vollan]

Created attachment 412651 [details]
Patch

[Per Arne Vollan]

Created attachment 412657 [details]
Patch

[Per Arne Vollan]

Created attachment 412669 [details]
Patch

[Per Arne Vollan]

Created attachment 412674 [details]
Patch

[Per Arne Vollan]

Created attachment 412684 [details]
Patch

[Alexey Proskuryakov]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 412766 [details]
Patch

[Per Arne Vollan]

Created attachment 412934 [details]
Patch

[EWS]

Committed r269584: <https://trac.webkit.org/changeset/269584>

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