| 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> | ||||
| Status: | REOPENED --- | ||||||
| Severity: | Normal | CC: | andersca, ap, beidson, benjamin, dgorbik, eric, sam, tmpsantos, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Adam Barth
2013-02-13 12:31:50 PST
Created attachment 188146 [details]
Patch
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? 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. It's entirely reasonable to start a provisional load before we've finished the previous load. Comment on attachment 188146 [details] Patch Clearing flags on attachment: 188146 Committed r142807: <http://trac.webkit.org/changeset/142807> All reviewed patches have been landed. Closing bug. (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. (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. Re-opened per comments above. |