Bug 212787

Summary: Disable CFNetwork AppSSO interception for Mac Catalyst
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebKit Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, jiewen_tan, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Jiewen Tan 2020-06-04 15:44:40 PDT 
Disable CFNetwork AppSSO interception for Mac Catalyst.
Comment 1 Jiewen Tan 2020-06-04 15:45:01 PDT 
<rdar://problem/63738783>
Comment 2 Jiewen Tan 2020-06-04 15:48:28 PDT 
Created attachment 401082 [details]
Patch
Comment 3 Jiewen Tan 2020-06-04 15:51:48 PDT 
The current patch only disable CFNetwork interception per network session. Let me use the global switch to do it for other processes.
Comment 4 Jiewen Tan 2020-06-04 15:58:13 PDT 
Created attachment 401083 [details]
Patch
Comment 5 Chris Dumez 2020-06-04 15:58:33 PDT 
Comment on attachment 401082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401082&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)

What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.
Comment 6 Chris Dumez 2020-06-04 15:59:04 PDT 
Comment on attachment 401083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401083&action=review

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)

What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.
Comment 7 Jiewen Tan 2020-06-04 16:16:49 PDT 
Comment on attachment 401083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401083&action=review

Thanks Chris for the r+.

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
>> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)
> 
> What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.

The AppSSO framework is only available for iOS, macOS and Catalyst. So we are good for the other two platforms.
Comment 8 Tim Horton 2020-06-04 16:39:02 PDT 
Comment on attachment 401083 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401083&action=review

>>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
>>> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)
>> 
>> What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.
> 
> The AppSSO framework is only available for iOS, macOS and Catalyst. So we are good for the other two platforms.

Is the NSURLSessionConfiguration property available on all platforms? It might be nice to just remove this ifdef for future-proofing 🤷‍♂️
Comment 9 Chris Dumez 2020-06-04 16:39:54 PDT 
(In reply to Tim Horton from comment #8)
> Comment on attachment 401083 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401083&action=review
> 
> >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
> >>> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)
> >> 
> >> What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.
> > 
> > The AppSSO framework is only available for iOS, macOS and Catalyst. So we are good for the other two platforms.
> 
> Is the NSURLSessionConfiguration property available on all platforms? It
> might be nice to just remove this ifdef for future-proofing 🤷‍♂️

Yes, it is available on all platform, merely a no-op on platform that don't support AppSSO.
Comment 10 EWS 2020-06-04 16:54:06 PDT 
Committed r262585: <https://trac.webkit.org/changeset/262585>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401083 [details].
Comment 11 Jiewen Tan 2020-06-04 17:09:46 PDT 
(In reply to Chris Dumez from comment #9)
> (In reply to Tim Horton from comment #8)
> > Comment on attachment 401083 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=401083&action=review
> > 
> > >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1148
> > >>> +#if HAVE(APP_SSO) || PLATFORM(MACCATALYST)
> > >> 
> > >> What about TVOS and WatchOS? It looks like APP_SSO is 1 on macOS and iOS only.
> > > 
> > > The AppSSO framework is only available for iOS, macOS and Catalyst. So we are good for the other two platforms.
> > 
> > Is the NSURLSessionConfiguration property available on all platforms? It
> > might be nice to just remove this ifdef for future-proofing 🤷‍♂️
> 
> Yes, it is available on all platform, merely a no-op on platform that don't
> support AppSSO.

I think we need to support -2 macOS. Therefore, we can't do that now.