When the MobileGestalt cache is invalid, some MG queries will fail in the WebContent process, since it has no access to the daemon. This can be fixed by issuing a temporary extension to the daemon, and having the WebContent process query all relevant MG values while holding the extension. This will bring the values into the in-memory cache, which will be valid after the extension to the daemon has been revoked.
Created attachment 405567 [details] Patch
Created attachment 405584 [details] Patch
Created attachment 405585 [details] Patch
Created attachment 405587 [details] Patch
Comment on attachment 405587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405587&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) Does this code need to be platform conditional? If it doesn't need to be platform conditional, I'd suggest making it unconditional. From the perspective of the WebContent process, it's a better separation of concerns to honor the sandbox extension whenever it is present, regardless of why the UI process issued it. For one thing, the alternative risks issuing an extension and never revoking it!
(In reply to Geoffrey Garen from comment #5) > Comment on attachment 405587 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405587&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 > > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) > > Does this code need to be platform conditional? If it doesn't need to be > platform conditional, I'd suggest making it unconditional. > > From the perspective of the WebContent process, it's a better separation of > concerns to honor the sandbox extension whenever it is present, regardless > of why the UI process issued it. For one thing, the alternative risks > issuing an extension and never revoking it! That's a good point. In this case, I think this is only relevant on iOS, watchOS, and tvOS, since I believe MobileGestalt is not present elsewhere. Thanks for reviewing!
Comment on attachment 405587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405587&action=review >>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 >>> +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) >> >> Does this code need to be platform conditional? If it doesn't need to be platform conditional, I'd suggest making it unconditional. >> >> From the perspective of the WebContent process, it's a better separation of concerns to honor the sandbox extension whenever it is present, regardless of why the UI process issued it. For one thing, the alternative risks issuing an extension and never revoking it! > > That's a good point. In this case, I think this is only relevant on iOS, watchOS, and tvOS, since I believe MobileGestalt is not present elsewhere. > > Thanks for reviewing! How about if we make the calls to MG conditional on platform, but we make the check and consumption of the extension handle cross-platform (by tightening the scope of the #if)?
(In reply to Geoffrey Garen from comment #7) > Comment on attachment 405587 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405587&action=review > > >>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 > >>> +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) > >> > >> Does this code need to be platform conditional? If it doesn't need to be platform conditional, I'd suggest making it unconditional. > >> > >> From the perspective of the WebContent process, it's a better separation of concerns to honor the sandbox extension whenever it is present, regardless of why the UI process issued it. For one thing, the alternative risks issuing an extension and never revoking it! > > > > That's a good point. In this case, I think this is only relevant on iOS, watchOS, and tvOS, since I believe MobileGestalt is not present elsewhere. > > > > Thanks for reviewing! > > How about if we make the calls to MG conditional on platform, but we make > the check and consumption of the extension handle cross-platform (by > tightening the scope of the #if)? Sounds good, will fix! Thanks for reviewing!
Created attachment 405600 [details] Patch
Comment on attachment 405600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405600&action=review r=me > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 > + auto extension = SandboxExtension::create(WTFMove(*parameters.mobileGestaltExtensionHandle)); > + if (extension) { You can merge these two lines as "if (auto extension = ....)".
(In reply to Geoffrey Garen from comment #10) > Comment on attachment 405600 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405600&action=review > > r=me > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:206 > > + auto extension = SandboxExtension::create(WTFMove(*parameters.mobileGestaltExtensionHandle)); > > + if (extension) { > > You can merge these two lines as "if (auto extension = ....)". Great, will do! Thank you!
rdar://problem/66183188
Created attachment 405616 [details] Patch
Created attachment 405685 [details] Patch
Committed r265139: <https://trac.webkit.org/changeset/265139> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405685 [details].
Comment on attachment 405600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405600&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:209 > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) This needs a comment. I believe it’s trying to ask all the questions that need correct answers from everything that runs inside the sandbox. The comment should say what should and should not be on the list and how it was generated. I often discourage comments that say *what* something is doing, but we really need comments that say *why*!
(In reply to Darin Adler from comment #16) > Comment on attachment 405600 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405600&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:209 > > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) > > This needs a comment. I believe it’s trying to ask all the questions that > need correct answers from everything that runs inside the sandbox. The > comment should say what should and should not be on the list and how it was > generated. > > I often discourage comments that say *what* something is doing, but we > really need comments that say *why*! That is a good point. I will add a comment to this code. Thanks for reviewing!
(In reply to Darin Adler from comment #16) > Comment on attachment 405600 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405600&action=review > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:209 > > +#if PLATFORM(IOS_FAMILY) && !PLATFORM(MACCATALYST) > > This needs a comment. I believe it’s trying to ask all the questions that > need correct answers from everything that runs inside the sandbox. The > comment should say what should and should not be on the list and how it was > generated. > > I often discourage comments that say *what* something is doing, but we > really need comments that say *why*! Added comment in <https://trac.webkit.org/changeset/265267/webkit>.