NavigationSOAuthorizationSession should check the active URL of the responding page after waking up from waiting.
<rdar://problem/53280170>
Created attachment 374933 [details] Patch
Comment on attachment 374933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374933&action=review > Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:70 > + // When the current main frame URL is different from the before one, I think it might be simpler to say: "// When the current main frame URL has changed from the start of authentication, the SOAuthorization is no longer valid." Even better would be to make a method: bool NavigationSOAuthorizationSession::mainFrameURLDidChangeDuringAuthentication() { auto* page = this->page(); return !page || page->pageLoadState().activeURL() != m_waitingPageActiveURL; } > Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:72 > + if (page->pageLoadState().activeURL() != m_waitingPageActiveURL) { Then this just becomes: if (mainFrameURLDidChangeDuringAuthentication()) { abort(); ... And you don't need a comment. :-) > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-79 > - ASSERT_NOT_REACHED(); Why is it okay to remove this assertion? Is it because we now expect to abort if the URL changed during authentication?
Comment on attachment 374933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374933&action=review Thanks Brent for r+ this patch. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:70 >> + // When the current main frame URL is different from the before one, > > I think it might be simpler to say: > > "// When the current main frame URL has changed from the start of authentication, the SOAuthorization is no longer valid." > > Even better would be to make a method: > > bool NavigationSOAuthorizationSession::mainFrameURLDidChangeDuringAuthentication() > { > auto* page = this->page(); > return !page || page->pageLoadState().activeURL() != m_waitingPageActiveURL; > } Fixed. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.mm:72 >> + if (page->pageLoadState().activeURL() != m_waitingPageActiveURL) { > > Then this just becomes: > > if (mainFrameURLDidChangeDuringAuthentication()) { > abort(); > ... > > And you don't need a comment. :-) Named it pageActiveURLDidChangeDuringWaiting. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:-79 >> - ASSERT_NOT_REACHED(); > > Why is it okay to remove this assertion? Is it because we now expect to abort if the URL changed during authentication? Yes.
Created attachment 374991 [details] Patch for landing
Comment on attachment 374991 [details] Patch for landing Clearing flags on attachment: 374991 Committed r247885: <https://trac.webkit.org/changeset/247885>