SOAuthorizationSession::presentViewController should check WebPageProxy::isClosed()

• Component: WebKit2
• Status: RESOLVED FIXED

[Jiewen Tan]

Check m_isClosed in WebPageProxy::platformWindow.

[Jiewen Tan]

<rdar://problem/52323585>

[Jiewen Tan]

Created attachment 374031 [details]
Patch

[Chris Dumez]

Comment on attachment 374031 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:640
> +    if (m_isClosed)

I would instead update this check in SSO code:
if (!m_page || m_viewController) {
        uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
        return;
    }

->

if (!m_page || m_page->isClosed() || m_viewController) {
        uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
        return;
    }

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 374031 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374031&action=review
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:640
> > +    if (m_isClosed)
> 
> I would instead update this check in SSO code:
> if (!m_page || m_viewController) {
>         uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain
> code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
>         return;
>     }
> 
> ->
> 
> if (!m_page || m_page->isClosed() || m_viewController) {
>         uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain
> code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
>         return;
>     }

Also, do we have API testing for SSO? If so, it'd be good to have a test that starts an SSO request then calls _close on the webView right away.

[Jiewen Tan]

Comment on attachment 374031 [details]
Patch

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

Thanks Chris for reviewing the patch.

>>> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:640
>>> +    if (m_isClosed)
>> 
>> I would instead update this check in SSO code:
>> if (!m_page || m_viewController) {
>>         uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
>>         return;
>>     }
>> 
>> ->
>> 
>> if (!m_page || m_page->isClosed() || m_viewController) {
>>         uiCallback(NO, adoptNS([[NSError alloc] initWithDomain:SOErrorDomain code:kSOErrorAuthorizationPresentationFailed userInfo:nil]).get());
>>         return;
>>     }
> 
> Also, do we have API testing for SSO? If so, it'd be good to have a test that starts an SSO request then calls _close on the webView right away.

Sure thing. However, it still feels weird that caller of m_page->platformWindow() needs to check m_page->isClosed(). SSO tests are all API tests which should be able to reproduce what you suggest. Let me try.

[Jiewen Tan]

Created attachment 374045 [details]
Patch

[Chris Dumez]

Comment on attachment 374045 [details]
Patch

r=me

[Jiewen Tan]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 374045 [details]
> Patch
> 
> r=me

Thanks, Chris.

[WebKit Commit Bot]

Comment on attachment 374045 [details]
Patch

Clearing flags on attachment: 374045

Committed r247410: <https://trac.webkit.org/changeset/247410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.