[soup] URL of the ResourceResponse passed to willSendRequest is incorrect

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

In case on redirection, the ResourceHandleSoup::restartedCallback() will be called. This will construct a ResourceRequest and a ResourceResponse (redirect response) and pass them to willSendRequest().

In case of a redirection from X to Y, one would expect that the objects passed to willSendRequest() are:
1. ResourceRequest.url() == Y
2. ResourceResponse.url() == X

Unfortunately, this is currently not the case because the uri of the SoupMessage passed to ResourceHandleSoup::restartedCallback() was already updated to the redirected one.
Therefore, we end up with:
1. ResourceRequest.url() == Y
2. ResourceResponse.url() == Y

As a consequence, the FrameLoaderClient has no way to know the original URL before redirection and we cannot display it properly in the DRT if dumpResourceLoadCallbacks() is enabled.

It is possible to notice the issue with:
http/tests/misc/will-send-request-returns-null-on-redirect.html

[Chris Dumez]

Created attachment 139218 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 139218 [details]
Proposed patch

This looks reasonable to me, but I'm unfamiliar with what the correct behavior is. Does this affect tests? The patch should probably include new results so we can confirm that this is progression.

[Chris Dumez]

FYI, only those 3 tests expect redirect responses in willSendRequest:

- http/tests/misc/will-send-request-returns-null-on-redirect.html
- http/tests/loading/redirect-methods.html
- http/tests/loading/307-after-303-after-post.html

All 3 of them are currently skipped in both GTK and EFL ports.

[Martin Robinson]

(In reply to comment #3)
> FYI, only those 3 tests expect redirect responses in willSendRequest:
> 
> - http/tests/misc/will-send-request-returns-null-on-redirect.html
> - http/tests/loading/redirect-methods.html
> - http/tests/loading/307-after-303-after-post.html
> 
> All 3 of them are currently skipped in both GTK and EFL ports.

Perhaps we can unskip them after this change? That would be awesome.

[Chris Dumez]

Unfortunately, those tests cannot be unskipped at this point because they are missing other functionality. I will implement those missing functionality in separate patches so that the tests can be unskipped in this bug report.

[Martin Robinson]

The GTK+ port's DRT has a bit more functionality than than the EFL port's. What's still missing for GTK+?

This patch looks okay to me, but you should probably make a note in the ChangeLog that this will fix these three tests in the future. I'd also like to get an informal review from some people more familiar with the soup backend.

[Chris Dumez]

I added the other bugs (for EFL port) as dependency and I will wait for those to land before rebasing this path and unskipping the 3 test cases for EFL port.

The GTK port is also missing similar functionality to unskip the tests (at least willSendRequestReturnsNullOnRedirect) but I'm less familiar with the code. I can take a look at it afterwards though.

[Chris Dumez]

With this patch, it appears the following tests can be unskipped in GTK port:
- http/tests/loading/redirect-methods.html
- http/tests/loading/307-after-303-after-post.html

The following test does not pass yet because of missing support for willSendRequestReturnsNullOnRedirect(). I'll look into it.

[Chris Dumez]

(In reply to comment #8)
> With this patch, it appears the following tests can be unskipped in GTK port:
> - http/tests/loading/redirect-methods.html
> - http/tests/loading/307-after-303-after-post.html
> 
> The following test does not pass yet because of missing support for willSendRequestReturnsNullOnRedirect(). I'll look into it.

Please disregard this last comment, none of the tests are passing on GTK port (test_expectations.txt fooled me). Based on the output, it seems it is due to the following features missing:
- willSendRequestReturnsNullOnRedirect()
- Missing "willPerformClientRedirectToURL" messages (part of dumpFrameLoadCallbacks)
- Missing "didReceiveServerRedirectForProvisionalLoadForFrame" messages (part of dumpFrameLoadCallbacks)
- Missing "didCancelClientRedirectForFrame" messages (part of dumpFrameLoadCallbacks)

[Gustavo Noronha (kov)]

Comment on attachment 139218 [details]
Proposed patch

Hmm. I think you are right, but this is only a partial fix. The URL will be the old one, but all the headers will be the new ones, which is also not expected. I think we'll need to store the response we get earlier by reintroducing a got-headers signal handler, and then use it here.

[Chris Dumez]

Created attachment 139760 [details]
Patch

Proper fix proposal based on Gustavo Noronha's input.

[Chris Dumez]

Comment on attachment 139760 [details]
Patch

Clearing flags as the patch is wrong (Response storing code)

[Chris Dumez]

Created attachment 139766 [details]
Patch

Ok, here is the fixed patch.

2 things I would like to mention:
 - The m_response is now probably updated twice (first in gotHeadersCallback() when we get the headers and then in sendRequestCallback())
 - Would it make sense to move the WEB_TIMING receiveHeadersEnd update code from sendRequestCallback() to gotHeadersCallback() now that we listen for the "got-headers" signal?

[Sergio Villar Senin]

(In reply to comment #13)
> Created an attachment (id=139766) [details]
> Patch
> 
> Ok, here is the fixed patch.
> 
> 2 things I would like to mention:
>  - The m_response is now probably updated twice (first in gotHeadersCallback() when we get the headers and then in sendRequestCallback())

Indeed, thing is that content sniffing has not happened by the time got-headers is emitted so we need to wait for sendRequestCallback in that case

>  - Would it make sense to move the WEB_TIMING receiveHeadersEnd update code from sendRequestCallback() to gotHeadersCallback() now that we listen for the "got-headers" signal?

I think it'd make total sense

In any case I think that this patch should include the unskipping of the 3 tests you mention. I think we should wait till you land the required DRT stuff to push this in. Anyway if Gustavo and Martin are ok with that, they're the reviewers :)

[Chris Dumez]

Created attachment 139782 [details]
Patch

Move WEB_TIMING receiveHeadersEnd update code from sendRequestCallback() to new gotHeadersCallback() callback based on Sergio Villar Senin's feedback.

[Gustavo Noronha (kov)]

Comment on attachment 139782 [details]
Patch

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

I agree with Sergio it would be better to land this while unskipping the tests. I had a couple of nits, so I'll mark this r- so it gets out of the review queue, but the code looks good to me.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:218
> +// Called afer receiving all headers for the message.

I don't think this comment adds value, I suggest removing it. However, it may be puzzling for someone reading the code to understand exactly why we handle got-headers and update d->m_response while doing it, so...

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:236
> +    ResourceResponse response;
> +    response.updateFromSoupMessage(msg);
> +
> +    d->m_response = response;

... if you could add something to the effect of 'We need to have the original response to feed to willSendRequest in case we are redirected, so we store it here.' close to these lines, I think it would help our future readers =)

[Chris Dumez]

Created attachment 139959 [details]
Patch

Update the patch to take Gustavo Noronha's feedback into consideration.
Note that I cannot unskip the 3 tests until the 2 bugs in dependency get closed (need formal review).

[Gustavo Noronha (kov)]

Comment on attachment 139959 [details]
Patch

OK, looks like we can now unskip those tests (thus cq-), rest of the patch looks good, though.

[Chris Dumez]

Created attachment 140283 [details]
Patch

Here is the updated patch which unskips:
- http/tests/misc/will-send-request-returns-null-on-redirect.html
- http/tests/loading/307-after-303-after-post.html

To my surprise the following test was not passing on my machine anymore due to part of the output being missing:
- http/tests/loading/redirect-methods.html

After browsing the bug tracker, I found:
Bug 66873 - http/tests/loading/redirect-methods.html occasionally missing chunks of output

It seems there is another unrelated bug which causes http/tests/loading/redirect-methods.html to fail sometimes to I'm keeping it in test_expectations (and updating its bug number).

[Gustavo Noronha (kov)]

Comment on attachment 140283 [details]
Patch

Good catch, thanks!

[WebKit Review Bot]

Comment on attachment 140283 [details]
Patch

Clearing flags on attachment: 140283

Committed r116160: <http://trac.webkit.org/changeset/116160>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.