Bug 85072

Summary: [soup] URL of the ResourceResponse passed to willSendRequest is incorrect
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: danw, d-r, gns, mrobinson, rakuco, svillar, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 82704, 85173    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
mrobinson: review-
Patch
none
Patch
none
Patch
gns: review-, gns: commit-queue-
Patch
gns: review+, gns: commit-queue-
Patch none

Description Chris Dumez 2012-04-27 09:12:55 PDT
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
Comment 1 Chris Dumez 2012-04-27 09:23:48 PDT
Created attachment 139218 [details]
Proposed patch
Comment 2 Martin Robinson 2012-04-27 09:32:58 PDT
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.
Comment 3 Chris Dumez 2012-04-27 09:39:57 PDT
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.
Comment 4 Martin Robinson 2012-04-27 09:44:10 PDT
(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.
Comment 5 Chris Dumez 2012-04-30 03:54:56 PDT
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.
Comment 6 Martin Robinson 2012-04-30 03:57:53 PDT
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.
Comment 7 Chris Dumez 2012-04-30 04:02:33 PDT
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.
Comment 8 Chris Dumez 2012-04-30 04:30:52 PDT
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.
Comment 9 Chris Dumez 2012-04-30 04:59:45 PDT
(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)
Comment 10 Gustavo Noronha (kov) 2012-04-30 06:44:46 PDT
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.
Comment 11 Chris Dumez 2012-05-02 00:41:40 PDT
Created attachment 139760 [details]
Patch

Proper fix proposal based on Gustavo Noronha's input.
Comment 12 Chris Dumez 2012-05-02 00:48:08 PDT
Comment on attachment 139760 [details]
Patch

Clearing flags as the patch is wrong (Response storing code)
Comment 13 Chris Dumez 2012-05-02 01:42:52 PDT
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?
Comment 14 Sergio Villar Senin 2012-05-02 02:52:14 PDT
(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 :)
Comment 15 Chris Dumez 2012-05-02 03:13:26 PDT
Created attachment 139782 [details]
Patch

Move WEB_TIMING receiveHeadersEnd update code from sendRequestCallback() to new gotHeadersCallback() callback based on Sergio Villar Senin's feedback.
Comment 16 Gustavo Noronha (kov) 2012-05-02 08:45:17 PDT
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 =)
Comment 17 Chris Dumez 2012-05-02 23:04:35 PDT
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).
Comment 18 Gustavo Noronha (kov) 2012-05-04 10:56:10 PDT
Comment on attachment 139959 [details]
Patch

OK, looks like we can now unskip those tests (thus cq-), rest of the patch looks good, though.
Comment 19 Chris Dumez 2012-05-04 11:28:22 PDT
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).
Comment 20 Gustavo Noronha (kov) 2012-05-04 13:18:33 PDT
Comment on attachment 140283 [details]
Patch

Good catch, thanks!
Comment 21 WebKit Review Bot 2012-05-04 13:27:30 PDT
Comment on attachment 140283 [details]
Patch

Clearing flags on attachment: 140283

Committed r116160: <http://trac.webkit.org/changeset/116160>
Comment 22 WebKit Review Bot 2012-05-04 13:27:36 PDT
All reviewed patches have been landed.  Closing bug.