Bug 214965 – [iOS] Issue a temporary extension to the MobileGestalt daemon when the MobileGestalt cache is invalid

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 405567 [details]
Patch

[Per Arne Vollan]

Created attachment 405584 [details]
Patch

[Per Arne Vollan]

Created attachment 405585 [details]
Patch

[Per Arne Vollan]

Created attachment 405587 [details]
Patch

[Geoffrey Garen]

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!

[Per Arne Vollan]

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

[Geoffrey Garen]

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)?

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 405600 [details]
Patch

[Geoffrey Garen]

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 = ....)".

[Per Arne Vollan]

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

[Per Arne Vollan]

rdar://problem/66183188

[Per Arne Vollan]

Created attachment 405616 [details]
Patch

[Per Arne Vollan]

Created attachment 405685 [details]
Patch

[EWS]

Committed r265139: <https://trac.webkit.org/changeset/265139>

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

[Darin Adler]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

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