Bug 214816 – SOAuthorizationSession::dismissViewController could crash on calling WebPageProxy::platformWindow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

SOAuthorizationSession::dismissViewController presentingWindow could be null.

[Jiewen Tan]

<rdar://problem/65681530>

[Jiewen Tan]

Created attachment 405261 [details]
Patch

[Darin Adler]

Comment on attachment 405261 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:300
>      if (m_page && m_page->platformWindow()) {

WebKit coding style proposes a style called early exit. Since this code already has "return" statements in it, I think it’s a good candidate for that style.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:301
> +        if (auto *presentingWindow = m_page->platformWindow()) {

Why is it valuable to check this for null again? The line of code just above this already checks it for null. This patch seems to be based on an incorrect assumption that it’s not.

[Jiewen Tan]

Comment on attachment 405261 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:300
>>      if (m_page && m_page->platformWindow()) {
> 
> WebKit coding style proposes a style called early exit. Since this code already has "return" statements in it, I think it’s a good candidate for that style.

There is another if closure after, which is in parallel with this one. Just doing an early return here might not be sufficient.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:301
>> +        if (auto *presentingWindow = m_page->platformWindow()) {
> 
> Why is it valuable to check this for null again? The line of code just above this already checks it for null. This patch seems to be based on an incorrect assumption that it’s not.

Right, I checked that already. I will double check where could cause a null pointer dereference on m_page.

[Jiewen Tan]

Created attachment 405676 [details]
Patch

[youenn fablet]

Comment on attachment 405676 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        needs to check WebPageProxy:isClosed() befored calling platformWindow().

It seems that WebPageProxy::platformWindow implementation is brittle.
Shouldn't we then update WebPageProxy::platformWindow so that it checks that m_pageClient is not null and, if null, return nil?
Is it possible to write a test?

[Jiewen Tan]

(In reply to youenn fablet from comment #6)
> Comment on attachment 405676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405676&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        needs to check WebPageProxy:isClosed() befored calling platformWindow().
> 
> It seems that WebPageProxy::platformWindow implementation is brittle.
> Shouldn't we then update WebPageProxy::platformWindow so that it checks that
> m_pageClient is not null and, if null, return nil?
> Is it possible to write a test?

Unfortunately, it's not trivial to do the null check on platformWindow().
NSWindow *WebPageProxy::platformWindow()
{
    return pageClient().platformWindow();
}

pageClient() returns a reference. And for pageClient():
PageClient& WebPageProxy::pageClient() const
{
    ASSERT(m_pageClient);
    return *m_pageClient;
}

It seems to me a too large a change to make pageClient() return a pointer instead of a reference for fixing this crash and might very well be too large to land for the upcoming release.

I remember last time when I tried to write such a test, I failed. See Bug 199755. So, it's not trivial to write a test.

[Darin Adler]

(In reply to Jiewen Tan from comment #7)
> Unfortunately, it's not trivial to do the null check on platformWindow().
> NSWindow *WebPageProxy::platformWindow()
> {
>     return pageClient().platformWindow();
> }

    return m_pageClient ? m_pageClient->platformWindow() : nullptr;

[Jiewen Tan]

(In reply to Darin Adler from comment #8)
> (In reply to Jiewen Tan from comment #7)
> > Unfortunately, it's not trivial to do the null check on platformWindow().
> > NSWindow *WebPageProxy::platformWindow()
> > {
> >     return pageClient().platformWindow();
> > }
> 
>     return m_pageClient ? m_pageClient->platformWindow() : nullptr;

Done.

[Jiewen Tan]

Created attachment 405721 [details]
Patch

[youenn fablet]

Comment on attachment 405721 [details]
Patch

Can we remove the isClosed check?

[Jiewen Tan]

(In reply to youenn fablet from comment #11)
> Comment on attachment 405721 [details]
> Patch
> 
> Can we remove the isClosed check?

Sure.

[Jiewen Tan]

Created attachment 405726 [details]
Patch for landing

[Jiewen Tan]

Thanks Youenn and Darin for reviewing this patch!

[EWS]

Committed r265158: <https://trac.webkit.org/changeset/265158>

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