Bug 49018 – [GTK] response.isNull() assert when using directory file URI

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Nicolas Dufresne]

When using file:/// (or any directory) we get an assertion. This is due to Soup backend not sending a proper response prior to the first data chunk. Patch coming.

[Nicolas Dufresne]

Created attachment 72974 [details]
Fix assert using directory file URI

[WebKit Review Bot]

Attachment 72974 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/network/soup/ResourceHandleSoup.cpp']" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nicolas Dufresne]

Created attachment 72976 [details]
Fix assert using directory file URI

Fixed tabs instead of space in ChangeLog

[WebKit Review Bot]

Attachment 72976 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/network/soup/ResourceHandleSoup.cpp']" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nicolas Dufresne]

Created attachment 72978 [details]
 Fix assert using directory file URI

Sorry for the noise, small manipulation error here. This is the version without tabs.

[Sergio Villar Senin]

Nice catch!. Some comments to your patch

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:-694
> -    if (equalIgnoringCase(protocol, "http") || equalIgnoringCase(protocol, "https")) {

Although not directly related to the bug I think you can indeed include style fixes.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:-699
> -    if (equalIgnoringCase(protocol, "file") || equalIgnoringCase(protocol, "ftp") || equalIgnoringCase(protocol, "ftps")) {

If I am not wrong, this has nothing to do with the bug so, I guess you either should attach another patch or maybe a different bug, Martin?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:793
>      GOwnPtr<GError> error;

You're indeed right, we're not notifying about the didReceiveResponse for directories but:

1) why don't you just remove the condition in line 534 then?, and
2) in order to keep the consistency of the new design for ResourceHandleSoup, the didReceiveResponse should be called before any read, so it's better to keep the check in sendRequestCallback.

[Nicolas Dufresne]

(In reply to comment #6)
> Nice catch!. Some comments to your patch
> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:-694
> > -    if (equalIgnoringCase(protocol, "http") || equalIgnoringCase(protocol, "https")) {
> 
> Although not directly related to the bug I think you can indeed include style fixes.

Ok, but I'll be careful next time, didn't notice I did that.

> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:-699
> > -    if (equalIgnoringCase(protocol, "file") || equalIgnoringCase(protocol, "ftp") || equalIgnoringCase(protocol, "ftps")) {
> 
> If I am not wrong, this has nothing to do with the bug so, I guess you either should attach another patch or maybe a different bug, Martin?

I'll let Martin reply, normally I would split this up in a git branch, but WebKit process with ChangeLog is such a *** that I prefer keep related one liner together.

> 
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:793
> >      GOwnPtr<GError> error;
> 
> You're indeed right, we're not notifying about the didReceiveResponse for directories but:
> 
> 1) why don't you just remove the condition in line 534 then?, and

That would mean calling didReceiveResponse multiple time in the case of HTTP. Also there was discussion about Soup future where FTP would fire callbacks that triggers didReceiveResponse too, and potentially other protocols. Checking if a response was send right before passing the first buffer seems to be the most generic solution at the moment.

> 2) in order to keep the consistency of the new design for ResourceHandleSoup, the didReceiveResponse should be called before any read, so it's better to keep the check in sendRequestCallback.

That's pretty much explained in 1), but one addition is that currently didReceiveResponse() is called during the first read and not before.

[Nicolas Dufresne]

Martin, there was questions for you in previous comments, so adding you in CC.

[Martin Robinson]

(In reply to comment #7)

> > > WebCore/platform/network/soup/ResourceHandleSoup.cpp:-699
> > > -    if (equalIgnoringCase(protocol, "file") || equalIgnoringCase(protocol, "ftp") || equalIgnoringCase(protocol, "ftps")) {
> > 
> > If I am not wrong, this has nothing to do with the bug so, I guess you either should attach another patch or maybe a different bug, Martin?
> 
> I'll let Martin reply, normally I would split this up in a git branch, but WebKit process with ChangeLog is such a *** that I prefer keep related one liner together.

Unrelated bug fixes should be in separate bugs and patches. Additionally, each patch should, if possible, include tests for that change (and especially changes in WebCore). In some cases there is no sane way to test a change, this should probably be outlined in the ChangeLog. The webkit-patch tool makes working with ChangeLogs and patches a lot easier. Definitely look into that.

[Nicolas Dufresne]

Created attachment 73309 [details]
Fix assert using directory file URI

- Added unit test
- Removed unrelated style fix
- Removed unrelated non-working FTP shceme filtering

[Nicolas Dufresne]

Created attachment 73316 [details]
Fix assert using directory file URI

The style check script consider as garbage an empty file, so why not put some information about the directory with keep-empty file. Hopefully this won't be considered garbage.

[Sergio Villar Senin]

(In reply to comment #7)
> (In reply to comment #6)
> > You're indeed right, we're not notifying about the didReceiveResponse for directories but:
> > 
> > 1) why don't you just remove the condition in line 534 then?, and
> 
> That would mean calling didReceiveResponse multiple time in the case of HTTP. Also there was discussion about Soup future where FTP would fire callbacks that triggers didReceiveResponse too, and potentially other protocols. Checking if a response was send right before passing the first buffer seems to be the most generic solution at the moment.

Maybe I'm missing something but that did receive response is only called for "file:" URIs. HTTP requests don't match that condition.

[Nicolas Dufresne]

(In reply to comment #12)
> > That would mean calling didReceiveResponse multiple time in the case of HTTP. Also there was discussion about Soup future where FTP would fire callbacks that triggers didReceiveResponse too, and potentially other protocols. Checking if a response was send right before passing the first buffer seems to be the most generic solution at the moment.
> 
> Maybe I'm missing something but that did receive response is only called for "file:" URIs. HTTP requests don't match that condition.

client->didReceiveResponse() is called in signals fired between the notification for the request being sent and the first data buffer (see gotHeadersCallback(), contentSniffedCallback()). client->didReceiveResponse() is also the only way to set a response and MUST be called before didReceiveData(). I'm trying to guaranty that this will always happen and will not happend twice. This way, if someone add support for protocols with proper response (ftp, gopher, etc.) we can just concentrate on the soup backend and know that the binding is ready for it (aside the file/ftp/ftps filter that could be removed when the GIO backend is fixed).

[Sergio Villar Senin]

(In reply to comment #13)
> (In reply to comment #12)
> > > That would mean calling didReceiveResponse multiple time in the case of HTTP. Also there was discussion about Soup future where FTP would fire callbacks that triggers didReceiveResponse too, and potentially other protocols. Checking if a response was send right before passing the first buffer seems to be the most generic solution at the moment.
> > 
> > Maybe I'm missing something but that did receive response is only called for "file:" URIs. HTTP requests don't match that condition.
> 
> client->didReceiveResponse() is called in signals fired between the notification for the request being sent and the first data buffer (see gotHeadersCallback(), contentSniffedCallback()). client->didReceiveResponse() is also the only way to set a response and MUST be called before didReceiveData(). I'm trying to guaranty that this will always happen and will not happend twice. This way, if someone add support for protocols with proper response (ftp, gopher, etc.) we can just concentrate on the soup backend and know that the binding is ready for it (aside the file/ftp/ftps filter that could be removed when the GIO backend is fixed).

All the callbacks you mention are only executed for HTTP requests, for file: uri's none of them are called. That's why didreceiveresponse won't be called twice for file:URI's.

[Nicolas Dufresne]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> All the callbacks you mention are only executed for HTTP requests, for file: uri's none of them are called. That's why didreceiveresponse won't be called twice for file:URI's.

I completely understand that, but you forgot that FTP, FTPS is also enabled (and those are currently handled with GIO, which mean no response). Your solution to do case by case handling is just not future proof. We need to be able to change the backend (e.g. add new protocols, add proper response to FTP) without always getting weird assert and crash.

[Gustavo Noronha (kov)]

Comment on attachment 73316 [details]
Fix assert using directory file URI

As discussed on IRC, given the API is not set in stone, and the conceptually more correct fix for the current situation is to make the check on sendRequestCallback less strict, we'll go with that and rely on ASSERT to make sure we don't forget to dispatch a response where needed.

[Nicolas Dufresne]

Created attachment 73407 [details]
Fix assert using directory file URI

As requested I've reused the check that decided if GIO should be used and send a response in this case in sendRequestCallback(), also added assert to check we don't send twice and that we have sent a response.

At first I though that we could simply check for m_response.isNull() in sendRequestCallback(), but found out that contentSniffedCallback() may be called after with HTTP streams, which leads to sending two responses.

[Martin Robinson]

Comment on attachment 73407 [details]
Fix assert using directory file URI

View in context: https://bugs.webkit.org/attachment.cgi?id=73407&action=review

> LayoutTests/fast/loader/local-iFrame-directory-from-local.html:8
> +            var localDirectoryLocation = "file:///tmp/LayoutTests/fast/loader/resources/directory";

Sorry. I did not get a chance to give you this feedback before you posted a new patch. Using the "/tmp" directory will not work on Windows machines. If possible this should be changed to a directory relative to the test file.

Additionally the test should use "iframe" instead of "iFrame" to match the rest of the tests in the directory.

> LayoutTests/fast/loader/local-iFrame-directory-from-local.html:16
> +            var localDirectoryElement = document.createElement("iframe");
> +            localDirectoryElement.setAttribute("id", "myDirectory");
> +            localDirectoryElement.setAttribute("src", localDirectoryLocation);
> +            localDirectoryElement.setAttribute("width", "96%");
> +            localDirectoryElement.setAttribute("height", "70%");

Is it possible to simply include this element in the HTML source instead of adding it dynamically? You might need to use the onload event to run the test at the proper time.

> LayoutTests/fast/loader/local-iFrame-directory-from-local.html:22
> +            var directoryDocument =  document.getElementById("myDirectory").contentDocument;

Is there a guarantee that the load will complete by this point?

[Nicolas Dufresne]

(In reply to comment #18)
> (From update of attachment 73407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73407&action=review
> 
> > LayoutTests/fast/loader/local-iFrame-directory-from-local.html:8
> > +            var localDirectoryLocation = "file:///tmp/LayoutTests/fast/loader/resources/directory";
> 
> Sorry. I did not get a chance to give you this feedback before you posted a new patch. Using the "/tmp" directory will not work on Windows machines. If possible this should be changed to a directory relative to the test file.
> 
> Additionally the test should use "iframe" instead of "iFrame" to match the rest of the tests in the directory.

All this is base on local-iFrame-source-from-local.html (yes named with a big F) in the same directory. I assumed the test was correct. From this bug point of view, the test reproduce the ASSERT we had, but I'm not a JS expert. If you have a better example to propose I'll take, also if you consider this test wrong, then you should file a bug against those others tests (I think at least two other tests in this directory uses this template).

[Sergio Villar Senin]

(In reply to comment #17)
> Created an attachment (id=73407) [details]
> Fix assert using directory file URI
> 
> As requested I've reused the check that decided if GIO should be used and send a response in this case in sendRequestCallback(), also added assert to check we don't send twice and that we have sent a response.
> 
> At first I though that we could simply check for m_response.isNull() in sendRequestCallback(), but found out that contentSniffedCallback() may be called after with HTTP streams, which leads to sending two responses.

You could check with handle->shouldContentSniff() if content sniffing is activated and thus complement the isNull() check. Have you tried it?

[Martin Robinson]

> All this is base on local-iFrame-source-from-local.html (yes named with a big F) in the same directory. I assumed the test was correct. From this bug point of view, the test reproduce the ASSERT we had, but I'm not a JS expert. If you have a better example to propose I'll take, also if you consider this test wrong, then you should file a bug against those others tests (I think at least two other tests in this directory uses this template).

I think the iFrame naming is an exceptional case among unit test naming. Perhaps a better template for this test would be iframe-crash-on-missing-image.xhtml. It's typical for crash to be in the name, when tseting crashes, for instance.

[Nicolas Dufresne]

Created attachment 73535 [details]
Fix assert using directory file URI

Updated patch.

Code:
- Added more assert()
- Moved readCallback() assert lower since sending a response is not required if read failed.
- Use d->m_response for data:// to avoid asserting on !d->m_response.isNull() later
- Using handle->shouldContentSniff() along with d->m_response.isNull() to decide if a response should be sent in sendRequestCallback()

Test:
- renamed test to crash-display-local-directory.html
- Creating iframe in HTML and not using JS
- Using iframe onload() instead of body onload()
- Using <p> instead of <div> and <br/>

[Sergio Villar Senin]

(In reply to comment #22)
> Created an attachment (id=73535) [details]
> Fix assert using directory file URI
> 
> Updated patch.

The patch looks good to me, don't know if we need such amount of new asserts tough :). Being a bit picky I wouldn't say "Ensure a response for protocol using GIO.", we're ensuring it for any protocol not directly handled by WebKitGtk+, it could be gio, libsoup or whatever.

And regarding the comment in the ChangeLog I suggest something more descriptive like:
"Do ensure that didReceiveResponse happens before any call to didReceiveData. That was not true for file: URIs pointing to directories and thus was triggering an assertion." I think that's a bit better because your patch is really fixing that instead of only the directory issue mentioned in the bug. Just my 2 cents.

Thx!

> 
> Code:
> - Added more assert()
> - Moved readCallback() assert lower since sending a response is not required if read failed.
> - Use d->m_response for data:// to avoid asserting on !d->m_response.isNull() later
> - Using handle->shouldContentSniff() along with d->m_response.isNull() to decide if a response should be sent in sendRequestCallback()
> 
> Test:
> - renamed test to crash-display-local-directory.html
> - Creating iframe in HTML and not using JS
> - Using iframe onload() instead of body onload()
> - Using <p> instead of <div> and <br/>

[Nicolas Dufresne]

(In reply to comment #23)
> (In reply to comment #22)
> > Created an attachment (id=73535) [details] [details]
> > Fix assert using directory file URI
> > 
> > Updated patch.
> 
> The patch looks good to me, don't know if we need such amount of new asserts tough :). Being a bit picky I wouldn't say "Ensure a response for protocol using GIO.", we're ensuring it for any protocol not directly handled by WebKitGtk+, it could be gio, libsoup or whatever.

All the asserts have been triggered once by at least one test. The numerous assert is just an indicator of how spaghetti is that code ;)

I was doing generic stuff and you guys complained, so I've did it for what exist, and in this case soup and data was working, so I only fixed GIO (nothing else exist atm).

> 
> And regarding the comment in the ChangeLog I suggest something more descriptive like:
> "Do ensure that didReceiveResponse happens before any call to didReceiveData. That was not true for file: URIs pointing to directories and thus was triggering an assertion." I think that's a bit better because your patch is really fixing that instead of only the directory issue mentioned in the bug. Just my 2 cents.

No, it was working for normal files, only directories where affected. FTP and FTPs always do errors (not mounted error), so they are not affected.

> 
> Thx!
> 

Just want to leave a note that it gets really frustrating when reviews are being done 1 line at the time. Please, consider doing full review next time, you'll save time and I'll save time too.

Thanks for the review, I don't see anymore issues now.

[Sergio Villar Senin]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > Created an attachment (id=73535) [details] [details] [details]
> > > Fix assert using directory file URI
> > > 
> > > Updated patch.
> > 
> > The patch looks good to me, don't know if we need such amount of new asserts tough :). Being a bit picky I wouldn't say "Ensure a response for protocol using GIO.", we're ensuring it for any protocol not directly handled by WebKitGtk+, it could be gio, libsoup or whatever.
> 
> All the asserts have been triggered once by at least one test. The numerous assert is just an indicator of how spaghetti is that code ;)
> 
> I was doing generic stuff and you guys complained, so I've did it for what exist, and in this case soup and data was working, so I only fixed GIO (nothing else exist atm).
> 
> > 
> > And regarding the comment in the ChangeLog I suggest something more descriptive like:
> > "Do ensure that didReceiveResponse happens before any call to didReceiveData. That was not true for file: URIs pointing to directories and thus was triggering an assertion." I think that's a bit better because your patch is really fixing that instead of only the directory issue mentioned in the bug. Just my 2 cents.
> 
> No, it was working for normal files, only directories where affected. FTP and FTPs always do errors (not mounted error), so they are not affected.

"That was not true for file: URIs pointing to directories" I didn't mention normal files at all.

> Just want to leave a note that it gets really frustrating when reviews are being done 1 line at the time. Please, consider doing full review next time, you'll save time and I'll save time too.

I have learned several things since I'm a webkitter, and one of them is that the review process could take some time, but it works, and it ensures that the project moves forward. We're part of a bigger project and entering in such a big community requires to accept some rules.

I'm not a reviewer yet, but if you want to work in webkit my advice would be that you should be more patient, as reviewers are very busy guys that have to do a lot of things and ensure that the quality of the contributions meets webkit requirements.

[Nicolas Dufresne]

Created attachment 74152 [details]
Fix assert using directory file URI

Clarified commit and changelog messages along with the comment mentioned.

[Sergio Villar Senin]

Sorry for not spotting this before but I think this is totally useless

+    String protocol = handle->firstRequest().url().protocol();

[Nicolas Dufresne]

(In reply to comment #27)
> Sorry for not spotting this before but I think this is totally useless
> 
> +    String protocol = handle->firstRequest().url().protocol();

Oops, good catch, will fix this afternoon (currently on PST).

[Nicolas Dufresne]

Created attachment 74663 [details]
Fix assert using directory file URI

This was a long afternoon ;), so I removed unused variable.

[Martin Robinson]

Comment on attachment 74663 [details]
Fix assert using directory file URI

Great. Thank you.

[WebKit Commit Bot]

Comment on attachment 74663 [details]
Fix assert using directory file URI

Rejecting patch 74663 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
.................................
fast/js/sputnik/Unicode/Unicode_510 .........................................
fast/layers ............................
fast/leaks ..
fast/lists ..........................................................
fast/loader ......
fast/loader/crash-display-local-directory.html -> failed

Exiting early after 1 failures. 14830 tests run.
219.16s total testing time

14829 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
5 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6366018

[Nicolas Dufresne]

(In reply to comment #31)
> fast/loader/crash-display-local-directory.html -> failed
The test pass on my machine so I need help here. If somebody could provide me with the actual error being thrown.

[Nicolas Dufresne]

(In reply to comment #32)
> Full output: http://queues.webkit.org/results/6366018
Wait, is this test run on OSX ? Maybe Safari does not implement directory listing (or not in the backend) ?

Definatly need help here !

[Martin Robinson]

Maybe this test should be a platform-specific test in platform/gtk. Does it rely on some behavior specific to soup?

[Nicolas Dufresne]

(In reply to comment #34)
> Maybe this test should be a platform-specific test in platform/gtk. Does it rely on some behavior specific to soup?

Actually the full story is that the iframe content is not dumped by DRT, which is nice because it makes the result browser agnostic as long as all browser supports listing local directory.

One note though, I just realized that WebKit's default security policy is way too excessive with local file. I ran the test in stock Chrome and Epiphany and realized that it actually fails, but pass in GTK's DRT. The javascript operation required to check that the load succeeded get blocked because it pretends that file:/// and file:///mydir is not in the same domain/port. But as we mostly share the same settings for DRT I would be surprise this is the root cause of this failure. Would still be nice to find if there is any rational for this behavior (that we don't see on Opera and Firefox, need to file a bug I guess).

As you said, we could make this GTK only, that would work, but I kind of liked the idea that directory listing in general had at least one test. Will look into this tomorrow. 

Thanks for your time.

[Nicolas Dufresne]

Created attachment 74782 [details]
Same patch but with test moved to GTK platform test

I simply moved the test to the GTK platform specific.

[Martin Robinson]

Comment on attachment 74782 [details]
Same patch but with test moved to GTK platform test

Okay. Let's try this again.

[WebKit Commit Bot]

Comment on attachment 74782 [details]
Same patch but with test moved to GTK platform test

Clearing flags on attachment: 74782

Committed r72695: <http://trac.webkit.org/changeset/72695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.