Bug 109733 – Not every load is followed by a call to didFail or didFinish

• Component: New Bugs
• Status: REOPENED

[Adam Barth]

Remove bogus ASSERT in WebFrameProxy::didStartProvisionalLoad

[Adam Barth]

Created attachment 188146 [details]
Patch

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Adam Barth]

It's entirely reasonable to start a provisional load before we've finished the previous load.

[WebKit Review Bot]

Comment on attachment 188146 [details]
Patch

Clearing flags on attachment: 188146

Committed r142807: <http://trac.webkit.org/changeset/142807>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Anders Carlsson]

(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.

[Brady Eidson]

(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.

[Adam Barth]

Re-opened per comments above.