Bug 35300 – (GTK) HTTP 307 after a 303 after a POST re-sends POST data from the original request

• Component: Page Loading
• Status: RESOLVED WONTFIX

[Brady Eidson]

HTTP 307 after a 303 after a POST re-sends POST data from the original request.

See https://bugs.webkit.org/show_bug.cgi?id=31410 for more discussion and the fix that went in for Mac and Windows.

GTK needs to do the same.

[Alberto Garcia]

Looks like in GTK the second redirection doesn't even work (I get a timeout).

I'm taking a look at this.

[Dan Winship]

So with the changes from bug 88961, we now make the correct series of requests. However, the test still fails because DRT doesn't print the right strings on redirect.

Eg, -expected has:

http://127.0.0.1:8000/loading/resources/post-to-303-target.php - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, main document URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, http method GET> redirectResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/post-to-303-target.php, http status code 303>

but we output:

http://127.0.0.1:8000/loading/resources/303-to-307-target.php - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, main document URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, http method GET> redirectResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/post-to-303-target.php, http status code 303>

(The first URL is wrong.)

[youenn fablet]

EFL has a similar bug (https://bugs.webkit.org/show_bug.cgi?id=93214) for which I submitted a patch.
As the code is shared with Gtk (libsoup backend), this patch may be of interest.
I actually ran DRT GTK with and without the patch.
Without the patch, the last request is a POST (possible regression?). With the patch, the last request is a GET, as expected.

As of the DRT URL bug, the WebKitWebResource private uri field is changed in FrameLoaderClient::dispatchWillSendRequest.
I was wondering whether it would be a more appropriate model to keep this field unchanged accross redirections? 
If so, with a small change to willSendRequestCallback, this bug would probably be fixed.
I can provide a patch along those lines.

[Alberto Garcia]

Created attachment 196679 [details]
Patch

(In reply to comment #3)
> As of the DRT URL bug, the WebKitWebResource private uri field is
> changed in FrameLoaderClient::dispatchWillSendRequest.

> I was wondering whether it would be a more appropriate model to keep
> this field unchanged accross redirections?

Yes, this actually seems to be the problem.

Do we need to change that field? The identifier is supposed to be
associated to the initial request.

It works fine if we do it like that, here's the patch, I tried all
http/tests and it doesn't seem to break anything.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk-wk1/TestExpectations', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp']" exit_code: 1
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1284:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


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

[Alberto Garcia]

One other possibility if we don't want to change the current behavior
is to add a new 'original-uri' property to the WebKitWebResource
class.

I can easily prepare a patch.

(In reply to comment #4)
> Do we need to change that field? The identifier is supposed to be
> associated to the initial request.

One thing to note, though, is that we're currently updating the uri
property (which is read-only) by directly accessing the private
structure from a different class...

Gustavo wrote the original code (bug 29134), adding him to Cc.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk-wk1/TestExpectations', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp']" exit_code: 1
Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1284:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


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

[youenn fablet]

(In reply to comment #6)
> One other possibility if we don't want to change the current behavior
> is to add a new 'original-uri' property to the WebKitWebResource
> class.

Does that new property be useful outside this test?
If created for the purpose of the test, I would avoid doing that.
A temporary workaround may be to create an expected file for that test.
We would at least check that there is no regression in the POST-303-307 behavior.

> One thing to note, though, is that we're currently updating the uri
> property (which is read-only) by directly accessing the private
> structure from a different class...

Yes, when reading that code, I thought it might be better to encapsulate this as a function call, if this code is to remain.

[Gustavo Noronha (kov)]

Comment on attachment 196679 [details]
Patch

Yeah, the URI is to remain read-only for API users, it should only be updated by internal code, that is why it's done the way it is. I'm OK with adding an internal call to update it instead of poking the private structure, though. I think adding a new property for original-uri is a good idea, since it maintains the API expectations while still providing complete information.

All that said, wk1 is in maintenance mode and there was a decision not to add new API to it unless strictly necessary, so I wonder, do we really want to fix this test at this point?

r- for the change in API expectation, adding a new property sounds like a better solution.

[Alberto Garcia]

(In reply to comment #9)
> All that said, wk1 is in maintenance mode and there was a decision
> not to add new API to it unless strictly necessary, so I wonder, do
> we really want to fix this test at this point?

I think you're right, actually.

[Michael Catanzaro]

Closing since this looks like an old WK1 bug.