Bug 10845 – Various bugs in ResourceLoaderWin with local files

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Fisher (:fishd, Google)]

Various bugs in ResourceLoaderWin with local files:

  - m_fileHandle is sometimes compared to NULL and sometimes compared to 
    INVALID_HANDLE_VALUE.  lack of consistency results in bad behavior.

  - ResourceLoader::fileLoadTimer does not call client()->receivedResponse,
    which causes the rest of WebCore to get confused and sometimes crash.

Patch coming up.

[Darin Fisher (:fishd, Google)]

Created attachment 10544 [details]
patch

I chose to #include <windows.h> in ResourceLoaderInternal.h instead of attempting to re-define INVALID_HANDLE_VALUE.  It seems like you guys are trying to minimize headers included by headers to help compile time, so if you want me to do this another way, just say so.

[Darin Fisher (:fishd, Google)]

Created attachment 10545 [details]
patch 2

Actually, this is better.  There was no need for me to check m_fileHandle in the timer callback.

[David Kilzer (:ddkilzer)]

Comment on attachment 10545 [details]
patch 2

NOTE: Using WebKitTools/Scripts/svn-create-patch will avoid creating ChangeLog patches that are "shifted" like the one below.  :)

>Index: ChangeLog
>===================================================================
>--- ChangeLog	(revision 16346)
>+++ ChangeLog	(working copy)
>@@ -1,5 +1,18 @@
> 2006-09-13  MorganL  <[e-mail]>
> 
>+        Reviewed by NOBODY (OOPS!).
>+
>[...]
>+
>[phone]  MorganL  <[e-mail]>
>+
>         Reviewed/landed by aroben.
> 
>         Fixes http://bugzilla.opendarwin.org/show_bug.cgi?id=10833

[Darin Fisher (:fishd, Google)]

Created attachment 10546 [details]
patch 2.1 - using svn-create-patch

Thanks for the suggestion.

[Brady Eidson]

Comment on attachment 10546 [details]
patch 2.1 - using svn-create-patch

 void ResourceLoader::fileLoadTimer(Timer<ResourceLoader>* timer)
 {
+    client()->receivedResponse(this, 0);

This seems to me to introduce incorrect behavior - client->receivedResponse() is only supposed to be called when the HTTP response comes back for a HTTP request.  If it's only data you're talking about, client->receivedData() is all that needs to be called.

Can you tell use what the crash is you're seeing?

[Brady Eidson]

Also, I can go ahead and mention I'll probably be spending some time working on the ResourceLoader and getting a proper request/response infrastructure in place soon.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (From update of attachment 10546 [details] [edit])
>  void ResourceLoader::fileLoadTimer(Timer<ResourceLoader>* timer)
>  {
> +    client()->receivedResponse(this, 0);
> 
> This seems to me to introduce incorrect behavior - client->receivedResponse()
> is only supposed to be called when the HTTP response comes back for a HTTP
> request.  If it's only data you're talking about, client->receivedData() is 
> all that needs to be called.

I see.  I figured that the consumer might be insulated from such details.  Perhaps it would help if there was some documentation for the ResourceLoaderClient.  Afterall, someone is very likely to write code that assumes a receivedResponse occurs before the first receivedData, especially if they only test with HTTP URLs.

I'll try to reproduce the crash and see if this is truly to blame.  I assumed it was merely an oversight :-/

[Darin Fisher (:fishd, Google)]

Hmm... with the latest trunk, I'm not seeing the crash.  I'm also pretty sure that the lack of a receivedResponse notification must be unrelated to what I was seeing since the implementation in loader.cc does nothing on Windows.

So, what about the rest of the patch?  Is the rest of the patch OK?

[Brady Eidson]

I wish we had more docs, too, for *all* parts of the code ;)

ResourceLoaderDelegate is a little slippery I suppose, in the aspect you just identified.  
Regardless, maybe when I do some rewrite and cleanup of it all, I'll *at least* comment those.

As for the rest of your patch, yah, t'is cool - I will land that tommorrow!

[Brady Eidson]

Comment on attachment 10546 [details]
patch 2.1 - using svn-create-patch

Removing the client()->receivedResponse() call in the fileLoadTimer makes this patch totally r-plusable

[Mark Rowe (bdash)]

Landed in r16361, omitting the client()->receivedResponse() call in fileLoadTimer.

[Darin Fisher (:fishd, Google)]

Created attachment 10561 [details]
patch: add receivedResponse call

I spoke with Brady about this, and we came to the conclusion that this was a good idea actually.  It does end up impacting WebKit/COM/WebFrame.cpp, which currently promotes m_provisionalDataSource to m_dataSource only when it gets the receivedResponse callback.  Without that, the WebFrame gets into a confused state, which can mess up the embedding app.

[Brady Eidson]

Comment on attachment 10561 [details]
patch: add receivedResponse call

Yes