199755 2019-07-12 13:36:31 -0700 SOAuthorizationSession::presentViewController should check WebPageProxy::isClosed() 2019-07-12 17:53:37 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jiewen_tan jiewen_tan cdumez commit-queue webkit-bug-importer oldest_to_newest 1552502 0 jiewen_tan 2019-07-12 13:36:31 -0700 Check m_isClosed in WebPageProxy::platformWindow. 1552503 1 jiewen_tan 2019-07-12 13:36:40 -0700 <rdar://problem/52323585> 1552504 2 374031 jiewen_tan 2019-07-12 13:40:14 -0700 Created attachment 374031 Patch 1552521 3 374031 cdumez 2019-07-12 14:14:21 -0700 Comment on attachment 374031 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; } 1552529 4 cdumez 2019-07-12 14:33:04 -0700 (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. 1552551 5 374031 jiewen_tan 2019-07-12 15:05:06 -0700 Comment on attachment 374031 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. 1552579 6 374045 jiewen_tan 2019-07-12 15:54:36 -0700 Created attachment 374045 Patch 1552585 7 374045 cdumez 2019-07-12 16:03:14 -0700 Comment on attachment 374045 Patch r=me 1552586 8 jiewen_tan 2019-07-12 16:08:07 -0700 (In reply to Chris Dumez from comment #7) > Comment on attachment 374045 [details] > Patch > > r=me Thanks, Chris. 1552629 9 374045 commit-queue 2019-07-12 17:53:36 -0700 Comment on attachment 374045 Patch Clearing flags on attachment: 374045 Committed r247410: <https://trac.webkit.org/changeset/247410> 1552630 10 commit-queue 2019-07-12 17:53:37 -0700 All reviewed patches have been landed. Closing bug. 374031 2019-07-12 13:40:14 -0700 2019-07-12 15:54:35 -0700 Patch bug-199755-20190712134013.patch text/plain 1374 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ3MzkyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDRjMGMzZjE4MzZmNjVhMzhk ZTQyZTM4N2FjM2UxMjRmNTg5NjczZWEuLmI2NzliYzk3MjY5OWJjN2YxYmNjZjQ0NDBhMzk1YmVm OWQzMmEyYjIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTktMDctMTIgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBDaGVjayBtX2lzQ2xvc2VkIGlu IFdlYlBhZ2VQcm94eTo6cGxhdGZvcm1XaW5kb3cKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE5OTc1NQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNTIz MjM1ODU+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg cGFnZUNsaWVudCgpIGlzIG5vdCBndWFyYW50ZWVkIHRvIGJlIG5vbiBudWxsIGFsbCB0aGUgdGlt ZS4gVGhlcmVmb3JlLCB3ZSBzaG91bGQgY2hlY2sKKyAgICAgICAgbV9pc0Nsb3NlZCBiZWZvcmUg dXNpbmcgaXQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvbWFjL1dlYlBhZ2VQcm94eU1hYy5tbToK KyAgICAgICAgKFdlYktpdDo6V2ViUGFnZVByb3h5OjpwbGF0Zm9ybVdpbmRvdyk6CisKIDIwMTkt MDctMTIgIFBhd2VsIFN0YW5layAgPHAuc3RhbmVrQG1ldHJvbG9naWNhbC5jb20+CiAKICAgICAg ICAgQ29ubmVjdGlvbjo6d2FpdEZvclN5bmNSZXBseSgpIHVzZXMgd2FsbCB0aW1lIGNsb2NrIGZv ciB0aW1lb3V0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9tYWMvV2ViUGFn ZVByb3h5TWFjLm1tIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvbWFjL1dlYlBhZ2VQcm94eU1h Yy5tbQppbmRleCA4N2M2OGIzMzQ4NTFhYTg1OTcxNTljYmUwNDU4ZjhjZmJiZDYyZTdjLi42YjE4 NDFlN2E3YzAwZjNjOTE0MGRjZjRhOWVhMTA5MmE5YTM2ZjQ5IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L1VJUHJvY2Vzcy9tYWMvV2ViUGFnZVByb3h5TWFjLm1tCisrKyBiL1NvdXJjZS9XZWJL aXQvVUlQcm9jZXNzL21hYy9XZWJQYWdlUHJveHlNYWMubW0KQEAgLTYzNyw2ICs2MzcsOCBAQCB2 b2lkIFdlYlBhZ2VQcm94eTo6c3RhcnRXaW5kb3dEcmFnKCkKIAogTlNXaW5kb3cgKldlYlBhZ2VQ cm94eTo6cGxhdGZvcm1XaW5kb3coKQogeworICAgIGlmIChtX2lzQ2xvc2VkKQorICAgICAgICBy ZXR1cm4gbnVsbHB0cjsKICAgICByZXR1cm4gcGFnZUNsaWVudCgpLnBsYXRmb3JtV2luZG93KCk7 CiB9CiAK 374045 2019-07-12 15:54:36 -0700 2019-07-12 17:53:36 -0700 Patch bug-199755-20190712155435.patch text/plain 1876 jiewen_tan U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ3MzkyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDRjMGMzZjE4MzZmNjVhMzhk ZTQyZTM4N2FjM2UxMjRmNTg5NjczZWEuLjIzMzhlNmI5ZWY5YWUzZDRlYmYwM2YwM2ExMGEwMzJj YWI4NTk5ZWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTktMDctMTIgIEppZXdlbiBU YW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KKworICAgICAgICBTT0F1dGhvcml6YXRpb25TZXNz aW9uOjpwcmVzZW50Vmlld0NvbnRyb2xsZXIgc2hvdWxkIGNoZWNrIFdlYlBhZ2VQcm94eTo6aXND bG9zZWQoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTk5NzU1CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS81MjMyMzU4NT4KKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZWJQYWdlUHJveHk6OnBhZ2VDbGll bnQoKSBpcyBub3QgZ3VhcmFudGVlZCB0byBiZSBub24gbnVsbCBhbGwgdGhlIHRpbWUuIFRoZXJl Zm9yZSwgd2Ugc2hvdWxkIGNoZWNrCisgICAgICAgIFdlYlBhZ2VQcm94eTo6aXNDbG9zZWQoKSBi ZWZvcmUgdXNpbmcgaXQuCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQ29jb2EvU09BdXRob3JpemF0 aW9uL1NPQXV0aG9yaXphdGlvblNlc3Npb24ubW06CisgICAgICAgIChXZWJLaXQ6OlNPQXV0aG9y aXphdGlvblNlc3Npb246OnByZXNlbnRWaWV3Q29udHJvbGxlcik6CisKIDIwMTktMDctMTIgIFBh d2VsIFN0YW5layAgPHAuc3RhbmVrQG1ldHJvbG9naWNhbC5jb20+CiAKICAgICAgICAgQ29ubmVj dGlvbjo6d2FpdEZvclN5bmNSZXBseSgpIHVzZXMgd2FsbCB0aW1lIGNsb2NrIGZvciB0aW1lb3V0 CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9Db2NvYS9TT0F1dGhvcml6YXRp b24vU09BdXRob3JpemF0aW9uU2Vzc2lvbi5tbSBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0Nv Y29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRpb25TZXNzaW9uLm1tCmluZGV4IGM5ODky ZGRkYmEwN2ZjYTZhYzJmZDRkMWQ4MTg3YTYyZWJlNGY5MzcuLmJjZGIwYzI4MTZkNDVkYWYxMWVm YmFlZTEyZDI5MDE3MGE0ZWQwZjAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNz L0NvY29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRpb25TZXNzaW9uLm1tCisrKyBiL1Nv dXJjZS9XZWJLaXQvVUlQcm9jZXNzL0NvY29hL1NPQXV0aG9yaXphdGlvbi9TT0F1dGhvcml6YXRp b25TZXNzaW9uLm1tCkBAIC0xODMsNyArMTgzLDcgQEAgdm9pZCBTT0F1dGhvcml6YXRpb25TZXNz aW9uOjpwcmVzZW50Vmlld0NvbnRyb2xsZXIoU09BdXRob3JpemF0aW9uVmlld0NvbnRyb2xsZXIK IHsKICAgICBBU1NFUlQobV9zdGF0ZSA9PSBTdGF0ZTo6QWN0aXZlKTsKICAgICAvLyBPbmx5IGV4 cGVjdCBhdCBtb3N0IG9uZSBVSSBzZXNzaW9uIGZvciB0aGUgd2hvbGUgYXV0aG9yaXphdGlvbiBz ZXNzaW9uLgotICAgIGlmICghbV9wYWdlIHx8IG1fdmlld0NvbnRyb2xsZXIpIHsKKyAgICBpZiAo IW1fcGFnZSB8fCBtX3BhZ2UtPmlzQ2xvc2VkKCkgfHwgbV92aWV3Q29udHJvbGxlcikgewogICAg ICAgICB1aUNhbGxiYWNrKE5PLCBhZG9wdE5TKFtbTlNFcnJvciBhbGxvY10gaW5pdFdpdGhEb21h aW46U09FcnJvckRvbWFpbiBjb2RlOmtTT0Vycm9yQXV0aG9yaXphdGlvblByZXNlbnRhdGlvbkZh aWxlZCB1c2VySW5mbzpuaWxdKS5nZXQoKSk7CiAgICAgICAgIHJldHVybjsKICAgICB9Cg==