Bug 213357

Summary: [iOS, macOS] Allow access to the container manager to support Mail InjectedBundle
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, ddkilzer, pvollan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Catalina crash log
none
Patch none

Brent Fulgham
Reported 2020-06-18 15:19:12 PDT
The Mail Injected Bundle requires access to the container manager to support certain OS operations. We do not need this access for web browsing, and should limit this access to this one case. This patch creates a dynamic mach extension to the container manager for this single use case.
Attachments
Patch (6.61 KB, patch)
2020-06-18 15:22 PDT, Brent Fulgham
no flags
Patch (7.01 KB, patch)
2020-06-18 15:28 PDT, Brent Fulgham
no flags
Patch for landing (7.75 KB, patch)
2020-06-19 13:45 PDT, Brent Fulgham
no flags
Catalina crash log (57.48 KB, text/plain)
2020-06-19 17:51 PDT, Ryan Haddad
no flags
Patch (7.80 KB, patch)
2020-06-20 12:19 PDT, Brent Fulgham
no flags
Brent Fulgham
Comment 1 2020-06-18 15:20:15 PDT
Brent Fulgham
Comment 2 2020-06-18 15:22:06 PDT
Brent Fulgham
Comment 3 2020-06-18 15:28:37 PDT
Darin Adler
Comment 4 2020-06-18 16:13:56 PDT
Comment on attachment 402242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402242&action=review Is there any more scalable way to do this? Seems unfortunate to hardcode a client app bundle ID again. > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:299 > +#else How about this instead? #elif PLATFORM(IOS) return WebCore::IOSApplication::isMobileMail() #else return false; #endif
David Kilzer (:ddkilzer)
Comment 5 2020-06-19 02:32:51 PDT
Comment on attachment 402242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402242&action=review >> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:299 >> +#else > > How about this instead? > > #elif PLATFORM(IOS) > return WebCore::IOSApplication::isMobileMail() > #else > return false; > #endif The problem is that it’s not just MobileMail that needs access. We see other apps that use the WebContent process and talk to containermanagerd as well. This only fixes the majority of cases, but not all of them.
Brent Fulgham
Comment 6 2020-06-19 13:44:24 PDT
Comment on attachment 402242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402242&action=review >>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:299 >>> +#else >> >> How about this instead? >> >> #elif PLATFORM(IOS) >> return WebCore::IOSApplication::isMobileMail() >> #else >> return false; >> #endif > > The problem is that it’s not just MobileMail that needs access. We see other apps that use the WebContent process and talk to containermanagerd as well. > > This only fixes the majority of cases, but not all of them. Darin: Will do! David: I'll add telemetry-backtrace so we get a backtrace if this is hit by anyone else. I'm very interested to see how that's possible.
Brent Fulgham
Comment 7 2020-06-19 13:45:31 PDT
Created attachment 402319 [details] Patch for landing
EWS
Comment 8 2020-06-19 14:12:20 PDT
Committed r263287: <https://trac.webkit.org/changeset/263287> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402319 [details].
Ryan Haddad
Comment 9 2020-06-19 17:51:32 PDT
This change caused testing to exit early on Catalina due to crashes: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x0000000104e58d95 WebKit::AuxiliaryProcess::initializeSandbox(WebKit::AuxiliaryProcessInitializationParameters const&, WebKit::SandboxInitializationParameters&) + 6783 (AuxiliaryProcessMac.mm:693) 1 com.apple.WebKit 0x00000001050fcdb0 WebKit::WebProcess::initializeSandbox(WebKit::AuxiliaryProcessInitializationParameters const&, WebKit::SandboxInitializationParameters&) + 196 (WebProcessCocoa.mm:589) 2 com.apple.WebKit 0x0000000104c8e1ff WebKit::AuxiliaryProcess::initialize(WebKit::AuxiliaryProcessInitializationParameters const&) + 103 3 com.apple.WebKit 0x000000010506cab9 void WebKit::XPCServiceInitializer<WebKit::WebProcess, WebKit::XPCServiceInitializerDelegate>(WTF::OSObjectPtr<NSObject<OS_xpc_object>*>, NSObject<OS_xpc_object>*, NSObject<OS_xpc_object>*) + 645 (XPCServiceEntryPoint.h:135) 4 com.apple.WebKit 0x000000010506c7fe WebContentServiceInitializer + 61 (WebContentServiceEntryPoint.mm:53) https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/6485
Ryan Haddad
Comment 10 2020-06-19 17:51:49 PDT
Created attachment 402363 [details] Catalina crash log
Ryan Haddad
Comment 11 2020-06-19 18:08:18 PDT
Brent Fulgham
Comment 12 2020-06-20 12:19:25 PDT
EWS
Comment 13 2020-06-20 16:36:05 PDT
Committed r263320: <https://trac.webkit.org/changeset/263320> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402397 [details].
Note You need to log in before you can comment on or make changes to this bug.