Bug 20073 – Pending resource requests not always stopped when new request is loaded

• Component: New Bugs
• Status: NEW

[Daniel Jalkut]

I've discovered a situation where scheduled resource requests for a frame are not stopped when a new request is loaded into the frame.

The worst case scenario is that the requests are then attempted to be fulfilled while the loader is in provisional mode, where the SubresourceLoader will fail by default, leaving the CachedResource in "error" state. This cached resource will then live on, fulfilling future requests for the resource. If for instance the resource in question is an image, it will cause a permanent "broken image" for that resource until the CachedResource is cleared.

I will attach to this bug two files:

WebCoreLoaderBug.zip - This demo project is my best and most concise attempt to lead you to reproduce the bug yourself. Unfortunately, it's not 100%. It happens quite predictably and regularly on my PowerPC G5, but I think it has a lot to do with timing. Considering the nature of the bug, the frame must go into provisional mode and still be in provisional mode while a pending resource request is serviced, in order for the bug to reveal itself.

FrameLoaderFix.patch - On my machine, this one line of code (and many lines of comments) seems to fix the bug. When a new request is opened in a frame, one would expect that all outstanding requests from the previous frame content would be canceled.  Indeed, stopAllLoaders() does get called, but the very similarly named "stopLoading()" does not get reached.  It's important for stopLoading() to be reached, because otherwise the crucial call to cancelRequest() in loader.cpp is not reached, and the queued requests are left in the priority queue.

[Daniel Jalkut]

Created attachment 22331 [details]
Possible patch which needs careful review by somebody experienced with the loader.

I am not so much providing this patch as a proposed fix,  but more of an example of one possibly crude way of addressing the problem. I'm hoping somebody with experience will see to deciding whether this or another solution would be most appropriate.

Thanks for your time!

[Daniel Jalkut]

Created attachment 22332 [details]
This example project reproduces the bug scenario on my machine

Note that the sample project reproduces the bug on my machine, but may require coercing to get it to exactly cause the bug on your machine. All the same, it demonstrates the type of scenario (albeit in a contrived way) that could lead to the type of bad timing that causes the bug to rear its head.

[Daniel Jalkut]

Note: I have searched for a workaround to this problem. I hoped that calling "stopLoading" on the affected frame would cause "the good stop" to get reached, but it doesn't (it uses the same faulty "stopAllLoaders" method).

So far the best workaround I've come across is, when I need to fully reload a request in the web frame, to literally throw away the existing WebView and start from scratch, replacing the old WebView in the view hierarchy, and then loadHTMLString to start fresh.

This seems to be the only way to guarantee that the stale broken-image CachedResource objects don't get used perpetually by my WebFrame.

If anybody has a better workaround to suggest I'd be glad to hear it.

Daniel

[Eric Seidel (no email)]

Comment on attachment 22331 [details]
Possible patch which needs careful review by somebody experienced with the loader.

Bradee should review this

[Eric Seidel (no email)]

Just wanted to check in with Bradee about the patch... ?

[Brady Eidson]

Comment on attachment 22331 [details]
Possible patch which needs careful review by somebody experienced with the loader.

I'd really like Anders to take a look at this one when he gets back...  this one line change has so much potential to be... very complicated.

[Maciej Stachowiak]

I looked at this patch, and I think I'll need to read a bunch of loader code to figure out if it's correct. Or get Brady to explain in person.

[Maciej Stachowiak]

Comment on attachment 22331 [details]
Possible patch which needs careful review by somebody experienced with the loader.

r- for the following:

1) The comment is too long.
2) No ChangeLog entry.
3) No test case.
4) After review of the code, I am not sure if true or false is right. The difference is that stopLoading(true) will send an unload event, while false will not. The best way to check which is right is to check if other browsers send an unload even in this situation.

Conceptually, it seems right to stop loading here, just these details need to be worked out.

[Daniel Jalkut]

Maciej - thanks for reviewing the code.  I apologize for the long comment. It probably would have been better-suited to the bug report, but I wanted to make sure nobody assumed that the code should be integrated without careful consideration.

Thank you for taking a careful look at it. I am not an experienced WebKit contributor, so I did not know enough to include a test case or a change log entry. I just submitted the bug because I thought it might be an important problem to address for the long term health of the project.

It would be nice if there was a clear means of submitting bugs that did not require meeting the same test that "patch submissions" normally have to meet. Maybe this is asking for too much review work on the WebKit team's behalf. But it seems as though my contribution here is judged by WebKit patch standards, when it was really meant more as a glorified (hopefully useful) bug report.

Daniel