11554 2006-11-09 08:56:14 -0800 REGRESSION (r17682): Assertion failure in IconLoader::didFinishLoading() when the icon request returns 404 or 403 2006-11-10 13:25:07 -0800 1 1 1 Unclassified WebKit Page Loading 420+ Mac OS X 10.4 CLOSED FIXED data:text/html,<link%20rel="shortcut%20icon"%20href="http://www.apple.com/filenotfound"> Regression P1 Normal --- 1 mitz mitz beidson oldest_to_newest 46386 0 mitz 2006-11-09 08:56:14 -0800 The assertion in IconLoader::didFinishLoading() is hit when the icon URL request returns a result code outside the 200-299 range. The assertion fails because in this case, finishLoading() is called first from didReceiveResponse(). 46392 1 beidson 2006-11-09 09:40:38 -0800 I'll look into this soon... definitely a regression from before some work Darin and I did a couple of days ago 46355 2 beidson 2006-11-09 16:12:51 -0800 I thought the problem here was due to refactoring Darin and I did earlier causing a preexisting ASSERT to be hit - I see now what maciej changed in 17682. Looking at it now 46347 3 beidson 2006-11-09 17:30:40 -0800 Maciej's addition of didFailWithError changed what we should be checking in this method - very small change, at that. Patch is coming 46346 4 11449 beidson 2006-11-09 17:35:31 -0800 Created attachment 11449 Proposed fix Maciej's didFailWithError addition brought this one about - simple fix. 46310 5 beidson 2006-11-09 20:27:51 -0800 Discussed this with Maciej on IRC, I need to explore a little more - he claims didFailWithError should be terminal for a ResourceHandler, and didFinishLoading should not be called after. 46314 6 mitz 2006-11-09 23:12:35 -0800 (In reply to comment #5) > Discussed this with Maciej on IRC, I need to explore a little more - he claims > didFailWithError should be terminal for a ResourceHandler, and didFinishLoading > should not be called after. > To clarify my initial description, didFailWithError is never called in this case, only didReceiveResponse is. I think perhaps the way to deal with this situation is to change the behavior of didReceiveResponse for statuses outside the 200-299 range, so that it will only set an "invalid icon data" flag, and later in finishLoading to examine this flag, and if it is set, skip the icon processing and go straight to clearLoadingState. The flag could also be used to save the work of copying of data in didReceiveData. 46316 7 beidson 2006-11-09 23:57:58 -0800 Yah, I totally jumped to an assumption when I was debugging and saw m_handle == 0 - I assumed it could only happen in two places, but can really happen in 3. Addressing your suggestion - Darin and I actually went through great lengths to remove as moving flags and vars as possible - it would be a shame to put one back in. With this greater, complete understand of what's happening, I think my original patch becomes valid again, in a slightly expanded form. Stay tuned... 46320 8 11456 beidson 2006-11-10 00:37:53 -0800 Created attachment 11456 This is a more complete fix to this bug, and another bug too! 46321 9 11456 mitz 2006-11-10 00:47:07 -0800 Comment on attachment 11456 This is a more complete fix to this bug, and another bug too! r=me 46323 10 beidson 2006-11-10 01:19:02 -0800 Committed in r17702, please verify 11449 2006-11-09 17:35:31 -0800 2006-11-10 00:37:53 -0800 Proposed fix patch.txt text/plain 1107 beidson SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDE3Njk5 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMDYtMTEt MDkgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTExNTU0CisgICAgICAgIFNtYWxsIGxvYWRlciByZS1mYWN0b3IgY2xlYW51 cAorCisgICAgICAgICogbG9hZGVyL2ljb24vSWNvbkxvYWRlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpJY29uTG9hZGVyOjpkaWRGaW5pc2hMb2FkaW5nKTogY2FsbCBmaW5pc2hMb2FkaW5nKCkg b25seSBpZiB0aGVyZQorICAgICAgICBpcyBhY3R1YWxseSBzdGlsbCBhbiBhY3RpdmUgbG9hZGVy LgorCiAyMDA2LTExLTA5ICBNYWNpZWogU3RhY2hvd2lhayAgPG1qc0BhcHBsZS5jb20+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgQW5kZXJzLgpJbmRleDogbG9hZGVyL2ljb24vSWNvbkxvYWRlci5j cHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PQotLS0gbG9hZGVyL2ljb24vSWNvbkxvYWRlci5jcHAJKHJldmlzaW9uIDE3 Njk5KQorKysgbG9hZGVyL2ljb24vSWNvbkxvYWRlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEy MCw4ICsxMjAsMTAgQEAKIAogdm9pZCBJY29uTG9hZGVyOjpkaWRGaW5pc2hMb2FkaW5nKFJlc291 cmNlSGFuZGxlKiBoYW5kbGUpCiB7Ci0gICAgQVNTRVJUKGhhbmRsZSA9PSBtX2hhbmRsZSk7Ci0g ICAgZmluaXNoTG9hZGluZyhoYW5kbGUtPnVybCgpKTsKKyAgICBpZiAobV9oYW5kbGUpIHsKKyAg ICAgICAgQVNTRVJUKGhhbmRsZSA9PSBtX2hhbmRsZSk7CisgICAgICAgIGZpbmlzaExvYWRpbmco aGFuZGxlLT51cmwoKSk7CisgICAgfQogfQogCiB2b2lkIEljb25Mb2FkZXI6OmZpbmlzaExvYWRp bmcoY29uc3QgS1VSTCYgaWNvblVSTCkK 11456 2006-11-10 00:37:53 -0800 2006-11-10 00:47:07 -0800 This is a more complete fix to this bug, and another bug too! patch.txt text/plain 1920 beidson SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDE3NzAx KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMDYtMTEt MTAgIEJyYWR5IEVpZHNvbiA8YmVpZHNvbkBhcHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTE1NTQKKyAgICAgICAgRml4IHRoZSBhYm92ZSBidWcgKGluIGRpZEZpbmlz aExvYWRpbmcpIGFuZCBhbHNvIGFub3RoZXIgcG90ZW50aWFsIGJ1ZyBpbiBkaWRGYWlsV2l0aEVy cm9yCisgICAgICAgIGluIGNhc2UgdGhlIGljb24gbG9hZCBmYWlscyBhZnRlciBzb21lIGRhdGEg aGFzIGJlZW4gcmVjZWl2ZWQuIAorCisgICAgICAgICogbG9hZGVyL2ljb24vSWNvbkxvYWRlci5j cHA6CisgICAgICAgIChXZWJDb3JlOjpJY29uTG9hZGVyOjpkaWRGYWlsV2l0aEVycm9yKTogQ2xl YXIgdGhlIGJ1ZmZlciBzbyBoYWxmLWFuLWltYWdlIGlzbid0IGNvbW1pdGVkIHRvIHRoZSBEQgor ICAgICAgICBvbiBlcnJvci4gIEFsc28sIGFzIGEgbG9hZGVyIHJlLWZhY3RvcmluZyBzYW5pdHkg Y2hlY2ssIGFkZGVkIGFuIGFzc2VydGlvbgorICAgICAgICAoV2ViQ29yZTo6SWNvbkxvYWRlcjo6 ZGlkRmluaXNoTG9hZGluZyk6IElmIGFuIGljb24gbG9hZGVyIHJlc3VsdGVkIGluIGFuIGVycm9y LXJlc3BvbnNlLCB0aGUgaWNvbiAKKyAgICAgICAgaXMgYWxyZWFkeSBjb21taXR0ZWQgdG8gdGhl IERCLiAgU2tpcCBkb2luZyB0aGF0IHN0ZXAgdHdpY2UuCisKIDIwMDYtMTEtMDkgIE9saXZlciBI dW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBCcmFkeS4KSW5k ZXg6IGxvYWRlci9pY29uL0ljb25Mb2FkZXIuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIGxvYWRlci9pY29u L0ljb25Mb2FkZXIuY3BwCShyZXZpc2lvbiAxNzcwMCkKKysrIGxvYWRlci9pY29uL0ljb25Mb2Fk ZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMTUsMTMgKzExNSwxOSBAQAogCiB2b2lkIEljb25M b2FkZXI6OmRpZEZhaWxXaXRoRXJyb3IoUmVzb3VyY2VIYW5kbGUqIGhhbmRsZSwgY29uc3QgUmVz b3VyY2VFcnJvciYpCiB7CisgICAgQVNTRVJUKG1fbG9hZElzSW5Qcm9ncmVzcyk7CisgICAgbV9i dWZmZXIuY2xlYXIoKTsKICAgICBmaW5pc2hMb2FkaW5nKGhhbmRsZS0+dXJsKCkpOwogfQogCiB2 b2lkIEljb25Mb2FkZXI6OmRpZEZpbmlzaExvYWRpbmcoUmVzb3VyY2VIYW5kbGUqIGhhbmRsZSkK IHsKLSAgICBBU1NFUlQoaGFuZGxlID09IG1faGFuZGxlKTsKLSAgICBmaW5pc2hMb2FkaW5nKGhh bmRsZS0+dXJsKCkpOworICAgIC8vIElmIHRoZSBpY29uIGxvYWQgcmVzdWx0ZWQgYW4gZXJyb3It cmVzcG9uc2UgZWFybGllciwgdGhlIFJlc291cmNlSGFuZGxlIHdhcyBraWxsZWQgYW5kIGljb24g ZGF0YSBjb21taXRlZCB2aWEgZmluaXNoTG9hZGluZygpCisgICAgLy8gSW4gdGhhdCBjYXNlIHRo aXMgZGlkRmluaXNoTG9hZGluZyBjYWxsYmFjayBpcyBwb2ludGxlc3MgYW5kIHdlIGJhaWwuICBP dGhlcndpc2UsIGZpbmlzaExvYWRpbmcoKSBhcyBleHBlY3RlZAorICAgIGlmIChtX2xvYWRJc0lu UHJvZ3Jlc3MpIHsKKyAgICAgICAgQVNTRVJUKGhhbmRsZSA9PSBtX2hhbmRsZSk7CisgICAgICAg IGZpbmlzaExvYWRpbmcoaGFuZGxlLT51cmwoKSk7CisgICAgfQogfQogCiB2b2lkIEljb25Mb2Fk ZXI6OmZpbmlzaExvYWRpbmcoY29uc3QgS1VSTCYgaWNvblVSTCkK