Bug 199085 – Implement a new SPI to inform clients about AppSSO

• Status: NEW
• Component: WebKit Misc.

[Jiewen Tan]

Implement a new SPI to inform clients about AppSSO.

[Jiewen Tan]

<rdar://problem/50028246>

[Jiewen Tan]

Created attachment 372598 [details]
Patch

[Brent Fulgham]

GTK build error:

Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp.o -c /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp
In file included from ../../Source/WebKit/UIProcess/VisitedLinkStore.h:31,
                 from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp:27:
../../Source/WebKit/UIProcess/WebPageProxy.h:44:10: fatal error: SOAuthorizationLoadPolicy.h: No such file or directory
 #include "SOAuthorizationLoadPolicy.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

[Brent Fulgham]

Comment on attachment 372598 [details]
Patch

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

This seems reasonable, but we need a WK API review (ggaren?), and please fix the build issues I highlighted. r- to fix the build.

> Source/WebKit/UIProcess/API/APINavigationClient.h:35
> +#include "SOAuthorizationLoadPolicy.h"

This needs to be protected with HAVE(APP_SSO).

> Source/WebKit/UIProcess/WebPageProxy.h:44
> +#include "SOAuthorizationLoadPolicy.h"

This needs to be protected by HAVE(APP_SSO)

[Jiewen Tan]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 372598 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372598&action=review
> 
> This seems reasonable, but we need a WK API review (ggaren?), and please fix
> the build issues I highlighted. r- to fix the build.
> 
> > Source/WebKit/UIProcess/API/APINavigationClient.h:35
> > +#include "SOAuthorizationLoadPolicy.h"
> 
> This needs to be protected with HAVE(APP_SSO).
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:44
> > +#include "SOAuthorizationLoadPolicy.h"
> 
> This needs to be protected by HAVE(APP_SSO)

Thanks Brent for reviewing the patch. I was working on another branch locally, and therefore couldn't make this change timely.

Also, I might just remove the extension name in this SPI as we might not be able to get that information before the session start. Also, since the UI can be shown out of process, it might not event make sense to misguide clients to show that name. Therefore, I might need a separate SPI to tell clients what extension is showing when the UI is actually handled by the UI process.

[Jiewen Tan]

Created attachment 372662 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372662 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:10
> +        navigations. Therefore, clients can make a informed decision whether this is the right moment

make a informed decision whether => make an informed decision about whether

> Source/WebKit/ChangeLog:12
> +        pass along a human readable name for the extension such that clients could do whatever they

could do => can do

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:109
> +        if (!weakThis)
> +            return;

Why use a weak reference instead of a strong reference? What will keep the SOAuthorizationSession alive if the call to decidePolicyForSOAuthorizationLoad returns asynchronously?

If we do want to use a weak reference, after checking the weak reference, we should put it into a strong reference and then use data members directly instead of using weakThis->, for both readability and protection from null pointer crashes.

Or, if we switch to capturing a strong reference, we can also use data members directly.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:119
> +        // FIXME<rdar://problem/48909336>: Replace the below with AppSSO constants.

FIXME: <rdar://problem/48909336> Replace the below with AppSSO constants.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:189
> +    if (self.allowSOAuthorizationLoad) {
> +        completionHandler(policy);
> +        return;
> +    }

Seems like we should also have a test for invoking the completion handler on the next tick of the runloop, in case that exposes any object lifetime issues.

[Jiewen Tan]

Comment on attachment 372662 [details]
Patch

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

Thanks Geoff for r+ this patch.

>> Source/WebKit/ChangeLog:10
>> +        navigations. Therefore, clients can make a informed decision whether this is the right moment
> 
> make a informed decision whether => make an informed decision about whether

Fixed.

>> Source/WebKit/ChangeLog:12
>> +        pass along a human readable name for the extension such that clients could do whatever they
> 
> could do => can do

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:109
>> +            return;
> 
> Why use a weak reference instead of a strong reference? What will keep the SOAuthorizationSession alive if the call to decidePolicyForSOAuthorizationLoad returns asynchronously?
> 
> If we do want to use a weak reference, after checking the weak reference, we should put it into a strong reference and then use data members directly instead of using weakThis->, for both readability and protection from null pointer crashes.
> 
> Or, if we switch to capturing a strong reference, we can also use data members directly.

Ultimately, the website datastore will keep it alive. I don't know why we would want to use a strong reference to extend the lifetime of the current SOAuthorizationSession if the corresponding datastore dies or another SOAuthorizationSession suppresses the current one. Therefore, a weakPtr seems appropriate.

With regard to adding a protector like strong reference to the lambda scope, I feel like it is not necessary. If this becomes null at any point of the execution of this lambda, that means there is a bug at any of those method this lambda tries to call. Adding a protector would effectively hide the real bug.

For readability, I could capture this as well. And then, we could just use the weakThis once at the very beginning of the lambda.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:119
>> +        // FIXME<rdar://problem/48909336>: Replace the below with AppSSO constants.
> 
> FIXME: <rdar://problem/48909336> Replace the below with AppSSO constants.

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:189
>> +    }
> 
> Seems like we should also have a test for invoking the completion handler on the next tick of the runloop, in case that exposes any object lifetime issues.

Sounds great. Added.

[Jiewen Tan]

Created attachment 372894 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372894 [details]
Patch for landing

Clearing flags on attachment: 372894

Committed r246829: <https://trac.webkit.org/changeset/246829>