109733 2013-02-13 12:31:50 -0800 Not every load is followed by a call to didFail or didFinish 2013-02-14 09:26:36 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified REOPENED P2 Normal --- 1 abarth webkit-unassigned andersca ap beidson benjamin dgorbik eric sam tmpsantos webkit.review.bot oldest_to_newest 832194 0 abarth 2013-02-13 12:31:50 -0800 Remove bogus ASSERT in WebFrameProxy::didStartProvisionalLoad 832198 1 188146 abarth 2013-02-13 12:34:00 -0800 Created attachment 188146 Patch 832288 2 188146 eric 2013-02-13 13:35:57 -0800 Comment on attachment 188146 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188146&action=review > Source/WebKit2/ChangeLog:15 > + The ASSERT appears to be bogus. This patch removes it. Could you explain better why this ASSERT is bogus? What you think it was trying to test, what's actually going on, etc. It's not clear tdo me when "didStartProvisionalLoad" is called, or why it thinks that the load should be finished when that happens? Presumably this was trying to make sure that this FrameProxy only knows about one load at a time? 832293 3 eric 2013-02-13 13:38:35 -0800 I agree, btw. that this is likely to be a bogus ASSERT. Bug 109485 was about delaying the the frameloader client's "we're done parsing" callback until after the load event is actually done executing. Prior to that change, isLoadingInAPI sense would return false, while we were still in teh middle of executing an onload handler, which seems wrong and was causing loads to end too early for the threaded parser since the loader would not be on the stack when we got around to processing that onload, allowing the load to end. Restated: bug 109485 fixes two bugs, one in the normal and background-thread parser. 832364 4 abarth 2013-02-13 14:41:11 -0800 It's entirely reasonable to start a provisional load before we've finished the previous load. 832379 5 188146 webkit.review.bot 2013-02-13 14:55:50 -0800 Comment on attachment 188146 Patch Clearing flags on attachment: 188146 Committed r142807: <http://trac.webkit.org/changeset/142807> 832380 6 webkit.review.bot 2013-02-13 14:55:54 -0800 All reviewed patches have been landed. Closing bug. 832564 7 andersca 2013-02-13 17:10:31 -0800 (In reply to comment #4) > It's entirely reasonable to start a provisional load before we've finished the previous load. Actually, I think in that case, there’s supposed to be a provisionalLoadDidFail callback with a “canceled” error. 832576 8 beidson 2013-02-13 17:20:23 -0800 (In reply to comment #7) > (In reply to comment #4) > > It's entirely reasonable to start a provisional load before we've finished the previous load. > > Actually, I think in that case, there’s supposed to be a provisionalLoadDidFail callback with a “canceled” error. I think Anders is right. I think the point of the ASSERT is that *every* load needs to end with a didFinish or a didFail before a new load starts. 833145 9 abarth 2013-02-14 09:26:36 -0800 Re-opened per comments above. 188146 2013-02-13 12:34:00 -0800 2013-02-13 14:55:49 -0800 Patch bug-109733-20130213123029.patch text/plain 1514 abarth SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0Mjc3MikKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEzLTAyLTEzICBBZGFtIEJh cnRoICA8YWJhcnRoQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmVtb3ZlIGJvZ3VzIEFTU0VSVCBp biBXZWJGcmFtZVByb3h5OjpkaWRTdGFydFByb3Zpc2lvbmFsTG9hZAorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA5NzMzCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWZ0ZXIgaHR0cDovL3RyYWMud2Via2l0 Lm9yZy9jaGFuZ2VzZXQvMTQyNTU1LCB0aGlzIEFTU0VSVCBpcworICAgICAgICB0cmlnZ2VyaW5n IG9uIHRoZXNlIHRlc3RzOgorCisgICAgICAgIGZhc3QvZG9tL3dpbmRvdy1sb2FkLWNyYXNoLmh0 bWwKKyAgICAgICAgZmFzdC9mcmFtZXMvc2VhbWxlc3Mvc2VhbWxlc3MtaHlwZXJsaW5rLW5hbWVk Lmh0bWwKKyAgICAgICAgZmFzdC9mcmFtZXMvc2VhbWxlc3Mvc2VhbWxlc3MtaHlwZXJsaW5rLmh0 bWwKKworICAgICAgICBUaGUgQVNTRVJUIGFwcGVhcnMgdG8gYmUgYm9ndXMuIFRoaXMgcGF0Y2gg cmVtb3ZlcyBpdC4KKworICAgICAgICAqIFVJUHJvY2Vzcy9XZWJGcmFtZVByb3h5LmNwcDoKKyAg ICAgICAgKFdlYktpdDo6V2ViRnJhbWVQcm94eTo6ZGlkU3RhcnRQcm92aXNpb25hbExvYWQpOgor CiAyMDEzLTAyLTEzICBBbmRlcnMgQ2FybHNzb24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CiAKICAg ICAgICAgTWFrZSBQbHVnaW5Qcm9jZXNzQ29ubmVjdGlvbk1hbmFnZXIgYSBXb3JrUXVldWVNZXNz YWdlUmVjZWl2ZXIKSW5kZXg6IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9XZWJGcmFtZVByb3h5 LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvV2ViRnJhbWVQcm94 eS5jcHAJKHJldmlzaW9uIDE0Mjc2MykKKysrIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9XZWJG cmFtZVByb3h5LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTI5LDcgKzEyOSw2IEBAIGJvb2wgV2Vi RnJhbWVQcm94eTo6aXNEaXNwbGF5aW5nUERGRG9jdW0KIAogdm9pZCBXZWJGcmFtZVByb3h5Ojpk aWRTdGFydFByb3Zpc2lvbmFsTG9hZChjb25zdCBTdHJpbmcmIHVybCkKIHsKLSAgICBBU1NFUlQo bV9sb2FkU3RhdGUgPT0gTG9hZFN0YXRlRmluaXNoZWQpOwogICAgIEFTU0VSVChtX3Byb3Zpc2lv bmFsVVJMLmlzRW1wdHkoKSk7CiAgICAgbV9sb2FkU3RhdGUgPSBMb2FkU3RhdGVQcm92aXNpb25h bDsKICAgICBtX3Byb3Zpc2lvbmFsVVJMID0gdXJsOwo=