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.
<rdar://problem/70586545>
Created attachment 412126 [details] Patch
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.
(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.
How did you test this manually?
(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!
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.
Created attachment 412421 [details] Patch
(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!
Created attachment 412425 [details] Patch
Created attachment 412438 [details] Patch
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.
(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!
I usually copy localization directories from shipping Safari.
Created attachment 412645 [details] Patch
(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.
Created attachment 412651 [details] Patch
Created attachment 412657 [details] Patch
Created attachment 412669 [details] Patch
Created attachment 412674 [details] Patch
Created attachment 412684 [details] Patch
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?
(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!
Created attachment 412766 [details] Patch
Created attachment 412934 [details] Patch
Committed r269584: <https://trac.webkit.org/changeset/269584> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412934 [details].