SOAuthorizationSession::dismissViewController presentingWindow could be null.
<rdar://problem/65681530>
Created attachment 405261 [details] Patch
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.
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.
Created attachment 405676 [details] Patch
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?
(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.
(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;
(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.
Created attachment 405721 [details] Patch
Comment on attachment 405721 [details] Patch Can we remove the isClosed check?
(In reply to youenn fablet from comment #11) > Comment on attachment 405721 [details] > Patch > > Can we remove the isClosed check? Sure.
Created attachment 405726 [details] Patch for landing
Thanks Youenn and Darin for reviewing this patch!
Committed r265158: <https://trac.webkit.org/changeset/265158> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405726 [details].