|Summary:||Not every load is followed by a call to didFail or didFinish|
|Product:||WebKit||Reporter:||Adam Barth <abarth>|
|Component:||New Bugs||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||andersca, ap, beidson, benjamin, dgorbik, eric, sam, tmpsantos, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Adam Barth 2013-02-13 12:31:50 PST
Remove bogus ASSERT in WebFrameProxy::didStartProvisionalLoad
Comment 2 Eric Seidel (no email) 2013-02-13 13:35:57 PST
Comment on attachment 188146 [details] 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?
Comment 3 Eric Seidel (no email) 2013-02-13 13:38:35 PST
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.
Comment 4 Adam Barth 2013-02-13 14:41:11 PST
It's entirely reasonable to start a provisional load before we've finished the previous load.
Comment 5 WebKit Review Bot 2013-02-13 14:55:50 PST
Comment on attachment 188146 [details] Patch Clearing flags on attachment: 188146 Committed r142807: <http://trac.webkit.org/changeset/142807>
Comment 6 WebKit Review Bot 2013-02-13 14:55:54 PST
All reviewed patches have been landed. Closing bug.
Comment 7 Anders Carlsson 2013-02-13 17:10:31 PST
(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.
Comment 8 Brady Eidson 2013-02-13 17:20:23 PST
(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.
Comment 9 Adam Barth 2013-02-14 09:26:36 PST
Re-opened per comments above.