104472 2012-12-09 02:12:46 -0800 [Windows]ASSERTION failed in Windows: css3/css3-modsel-33.html 2012-12-10 21:18:52 -0800 1 1 1 Unclassified WebKit WebKit API 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 0 huangxueqing webkit-unassigned bdakin bfulgham roger_fong thorton webkit.review.bot oldest_to_newest 786718 0 huangxueqing 2012-12-09 02:12:46 -0800 In WebFrameLoaderClient::dispatchDidLayout, |milestones| maybe was |DidFirstLayout & DidFirstVisuallyNonEmptyLayout|, which will call webView->frameLoadDelegatePrivate(&frameLoadDelegatePrivate) twice, we should rest |frameLoadDelegatePrivate| after the first one, otherwise |T** operator&()| will hit |ASSERT(!m_ptr)|. 786732 1 178402 huangxueqing 2012-12-09 03:02:02 -0800 Created attachment 178402 patch 787559 2 178402 bfulgham 2012-12-10 11:12:44 -0800 Comment on attachment 178402 patch View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review I think this looks like a good change, but I wonder if things might be clearer if we didn't reuse the variable for two different purposes. > Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429 > + frameLoadDelegatePrivate = 0; This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout? 787560 3 bfulgham 2012-12-10 11:13:19 -0800 Tim, what do you think about making these two separate variables, rather than reusing the same COMPtr? 787566 4 178402 thorton 2012-12-10 11:22:16 -0800 Comment on attachment 178402 patch View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review >> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429 >> + frameLoadDelegatePrivate = 0; > > This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout? I agree, Brent's plan (two separate locals inside the if blocks) is clearer. 787595 5 178402 bfulgham 2012-12-10 11:40:34 -0800 Comment on attachment 178402 patch View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review Thanks for taking the time to post this fix. I think it looks good, but we would like you to move the COMPtr declaration inside the two "if" cases. This will make it clearer that the COMPtr is not needed elsewhere. For this reason, r-, but please update the patch and I'll approve it. >>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429 >>> + frameLoadDelegatePrivate = 0; >> >> This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout? > > I agree, Brent's plan (two separate locals inside the if blocks) is clearer. Okay -- please move the scope of "COMPtr<IWebFrameLoadDelegatePrivate> frameLoadDelegatePrivate" to be enclosed inside the two cases. 787968 6 178695 huangxueqing 2012-12-10 20:13:23 -0800 Created attachment 178695 patch 787970 7 huangxueqing 2012-12-10 20:19:44 -0800 (In reply to comment #5) > (From update of attachment 178402 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review > > Thanks for taking the time to post this fix. I think it looks good, but we would like you to move the COMPtr declaration inside the two "if" cases. This will make it clearer that the COMPtr is not needed elsewhere. > For this reason, r-, but please update the patch and I'll approve it. > > >>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429 > >>> + frameLoadDelegatePrivate = 0; > >> > >> This seems like a good change, but would it be clearer if we used separate COMPtrs for the two possible cases? Maybe we should have each one declared within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout? > > > > I agree, Brent's plan (two separate locals inside the if blocks) is clearer. > > Okay -- please move the scope of "COMPtr<IWebFrameLoadDelegatePrivate> frameLoadDelegatePrivate" to be enclosed inside the two cases. Done, it seems more reasonable, thanks for review. 787996 8 178695 webkit.review.bot 2012-12-10 20:59:55 -0800 Comment on attachment 178695 patch Rejecting attachment 178695 from commit-queue. New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html Full output: http://queues.webkit.org/results/15260214 788003 9 178695 webkit.review.bot 2012-12-10 21:18:48 -0800 Comment on attachment 178695 patch Clearing flags on attachment: 178695 Committed r137246: <http://trac.webkit.org/changeset/137246> 788004 10 webkit.review.bot 2012-12-10 21:18:52 -0800 All reviewed patches have been landed. Closing bug. 178402 2012-12-09 03:02:02 -0800 2012-12-10 20:13:23 -0800 patch 104472.patch text/plain 1708 huangxueqing SW5kZXg6IFNvdXJjZS9XZWJLaXQvd2luL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJj ZS9XZWJLaXQvd2luL0NoYW5nZUxvZwkocmV2aXNpb24gMTM3MDY0KQ0KKysrIFNvdXJjZS9XZWJL aXQvd2luL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQ0KQEAgLTEsMyArMSwxOSBAQA0KKzIwMTIt MTItMDkgIFh1ZXFpbmcgSHVhbmcgIDxodWFuZ3h1ZXFpbmdAYmFpZHUuY29tPgorCisgICAgICAg IEFTU0VSVElPTiBmYWlsZWQgaW4gV2luZG93czogY3NzMy9jc3MzLW1vZHNlbC0zMy5odG1sCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDQ0NzIKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbiBXZWJGcmFt ZUxvYWRlckNsaWVudDo6ZGlzcGF0Y2hEaWRMYXlvdXQsIG1pbGVzdG9uZXMgbWF5YmUgCisgICAg ICAgIERpZEZpcnN0TGF5b3V0ICYgRGlkRmlyc3RWaXN1YWxseU5vbkVtcHR5TGF5b3V0LCB3aGlj aCB3aWxsIGNhbGwgCisgICAgICAgIHdlYlZpZXctPmZyYW1lTG9hZERlbGVnYXRlUHJpdmF0ZSgm ZnJhbWVMb2FkRGVsZWdhdGVQcml2YXRlKSB0d2ljZSwgCisgICAgICAgIHdlIHNob3VsZCByZXNl dCBmcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGUgYWZ0ZXIgdGhlIGZpcnN0IG9uZSwgCisgICAgICAg IG90aGVyd2lzZSwgVCoqIG9wZXJhdG9yJigpIHdpbGwgaGl0IEFTU0VSVCghbV9wdHIpIGluIENP TVB0ci5oLgorCisgICAgICAgICogV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGllbnQu Y3BwOgorICAgICAgICAoV2ViRnJhbWVMb2FkZXJDbGllbnQ6OmRpc3BhdGNoRGlkTGF5b3V0KToK KwogMjAxMi0xMi0wOCAgU2Vva2p1IEt3b24gIDxzZW9ranUua3dvbkBnbWFpbC5jb20+CiAKICAg ICAgICAgUmVtb3ZlIHVudXNlZCBoZWFkZXJzCkluZGV4OiBTb3VyY2UvV2ViS2l0L3dpbi9XZWJD b3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVudC5jcHANCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBTb3Vy Y2UvV2ViS2l0L3dpbi9XZWJDb3JlU3VwcG9ydC9XZWJGcmFtZUxvYWRlckNsaWVudC5jcHAJKHJl dmlzaW9uIDEzNzA1MykNCisrKyBTb3VyY2UvV2ViS2l0L3dpbi9XZWJDb3JlU3VwcG9ydC9XZWJG cmFtZUxvYWRlckNsaWVudC5jcHAJKHdvcmtpbmcgY29weSkNCkBAIC00MjYsNiArNDI2LDcgQEAN CiAgICAgICAgICAgICBmcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGUtPmRpZEZpcnN0TGF5b3V0SW5G cmFtZSh3ZWJWaWV3LCBtX3dlYkZyYW1lKTsKICAgICB9CiAKKyAgICBmcmFtZUxvYWREZWxlZ2F0 ZVByaXZhdGUgPSAwOwogICAgIGlmIChtaWxlc3RvbmVzICYgRGlkRmlyc3RWaXN1YWxseU5vbkVt cHR5TGF5b3V0KSB7CiAgICAgICAgIGlmIChTVUNDRUVERUQod2ViVmlldy0+ZnJhbWVMb2FkRGVs ZWdhdGVQcml2YXRlKCZmcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGUpKSAmJiBmcmFtZUxvYWREZWxl Z2F0ZVByaXZhdGUpCiAgICAgICAgICAgICBmcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGUtPmRpZEZp cnN0VmlzdWFsbHlOb25FbXB0eUxheW91dEluRnJhbWUod2ViVmlldywgbV93ZWJGcmFtZSk7Cg== 178695 2012-12-10 20:13:23 -0800 2012-12-10 21:18:48 -0800 patch 104472.patch text/plain 2014 huangxueqing SW5kZXg6IFNvdXJjZS9XZWJLaXQvd2luL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJj ZS9XZWJLaXQvd2luL0NoYW5nZUxvZwkocmV2aXNpb24gMTM3MjQwKQ0KKysrIFNvdXJjZS9XZWJL aXQvd2luL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQ0KQEAgLTEsMyArMSwxNyBAQA0KKzIwMTIt MTItMTAgIFh1ZXFpbmcgSHVhbmcgIDxodWFuZ3h1ZXFpbmdAYmFpZHUuY29tPgorCisgICAgICAg IEFTU0VSVElPTiBmYWlsZWQgaW4gV2luZG93czogY3NzMy9jc3MzLW1vZHNlbC0zMy5odG1sCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDQ0NzIKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbiBXZWJGcmFt ZUxvYWRlckNsaWVudDo6ZGlzcGF0Y2hEaWRMYXlvdXQsIG1pbGVzdG9uZXMgbWF5YmUKKyAgICAg ICAgRGlkRmlyc3RMYXlvdXQgJiBEaWRGaXJzdFZpc3VhbE5vbkVtcHR5TGF5b3V0LCB3ZSBzaG91 bGQKKyAgICAgICAgdXNlIHNlcGVyYXRlIENPTVB0cnMgZm9yIHRoZSB0d28gY2FzZXMgcmF0aGVy IHRoYW4gcmV1c2Ugb25lLiAKKworICAgICAgICAqIFdlYkNvcmVTdXBwb3J0L1dlYkZyYW1lTG9h ZGVyQ2xpZW50LmNwcDoKKyAgICAgICAgKFdlYkZyYW1lTG9hZGVyQ2xpZW50OjpkaXNwYXRjaERp ZExheW91dCk6CisKIDIwMTItMTItMDggIFNlb2tqdSBLd29uICA8c2Vva2p1Lmt3b25AZ21haWwu Y29tPgogCiAgICAgICAgIFJlbW92ZSB1bnVzZWQgaGVhZGVycwpJbmRleDogU291cmNlL1dlYktp dC93aW4vV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGllbnQuY3BwDQo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 DQotLS0gU291cmNlL1dlYktpdC93aW4vV2ViQ29yZVN1cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGll bnQuY3BwCShyZXZpc2lvbiAxMzcxMDUpDQorKysgU291cmNlL1dlYktpdC93aW4vV2ViQ29yZVN1 cHBvcnQvV2ViRnJhbWVMb2FkZXJDbGllbnQuY3BwCSh3b3JraW5nIGNvcHkpDQpAQCAtNDE5LDE0 ICs0MTksMTUgQEANCiB2b2lkIFdlYkZyYW1lTG9hZGVyQ2xpZW50OjpkaXNwYXRjaERpZExheW91 dChMYXlvdXRNaWxlc3RvbmVzIG1pbGVzdG9uZXMpCiB7CiAgICAgV2ViVmlldyogd2ViVmlldyA9 IG1fd2ViRnJhbWUtPndlYlZpZXcoKTsKLSAgICBDT01QdHI8SVdlYkZyYW1lTG9hZERlbGVnYXRl UHJpdmF0ZT4gZnJhbWVMb2FkRGVsZWdhdGVQcml2YXRlOwogCiAgICAgaWYgKG1pbGVzdG9uZXMg JiBEaWRGaXJzdExheW91dCkgeworICAgICAgICBDT01QdHI8SVdlYkZyYW1lTG9hZERlbGVnYXRl UHJpdmF0ZT4gZnJhbWVMb2FkRGVsZWdhdGVQcml2YXRlOwogICAgICAgICBpZiAoU1VDQ0VFREVE KHdlYlZpZXctPmZyYW1lTG9hZERlbGVnYXRlUHJpdmF0ZSgmZnJhbWVMb2FkRGVsZWdhdGVQcml2 YXRlKSkgJiYgZnJhbWVMb2FkRGVsZWdhdGVQcml2YXRlKQogICAgICAgICAgICAgZnJhbWVMb2Fk RGVsZWdhdGVQcml2YXRlLT5kaWRGaXJzdExheW91dEluRnJhbWUod2ViVmlldywgbV93ZWJGcmFt ZSk7CiAgICAgfQogCiAgICAgaWYgKG1pbGVzdG9uZXMgJiBEaWRGaXJzdFZpc3VhbGx5Tm9uRW1w dHlMYXlvdXQpIHsKKyAgICAgICAgQ09NUHRyPElXZWJGcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGU+ IGZyYW1lTG9hZERlbGVnYXRlUHJpdmF0ZTsKICAgICAgICAgaWYgKFNVQ0NFRURFRCh3ZWJWaWV3 LT5mcmFtZUxvYWREZWxlZ2F0ZVByaXZhdGUoJmZyYW1lTG9hZERlbGVnYXRlUHJpdmF0ZSkpICYm IGZyYW1lTG9hZERlbGVnYXRlUHJpdmF0ZSkKICAgICAgICAgICAgIGZyYW1lTG9hZERlbGVnYXRl UHJpdmF0ZS0+ZGlkRmlyc3RWaXN1YWxseU5vbkVtcHR5TGF5b3V0SW5GcmFtZSh3ZWJWaWV3LCBt X3dlYkZyYW1lKTsKICAgICB9Cg==