|Summary:||[soup] URL of the ResourceResponse passed to willSendRequest is incorrect|
|Product:||WebKit||Reporter:||Chris Dumez <cdumez>|
|Component:||WebCore Misc.||Assignee:||Chris Dumez <cdumez>|
|Severity:||Normal||CC:||danw, d-r, gustavo, mrobinson, rakuco, svillar, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||82704, 85173|
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 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.