Bug 170058

Summary: [SOUP] URI Fragment is lost after redirect
Product: WebKit Reporter: Paul Ogris <pogris>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bugs-noreply, cgarcia, mcatanzaro, svillar
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mcatanzaro: review+

Paul Ogris
Reported 2017-03-24 10:36:27 PDT
When using webkit_web_view_load_uri with the URI "https://hackage.haskell.org/package/bytestring/docs/Data-ByteString.html#v:map", the jump to the specified anchor is never performed. The page itself redirects to the current version of the documentation hosted there, currently https://hackage.haskell.org/package/bytestring-0.10.8.1/docs/Data-ByteString.html#v:map. Loading this URI directly works as intended. The problem also exists when testing with MiniBrowser. How to reproduce: ./MiniBrowser "https://hackage.haskell.org/package/bytestring/docs/Data-ByteString.html#v:map" Actual Results: The page is displayed, but not at the anchor point specified Expected Results: The page should be scrolled down to the specified anchor. E.g. Firefox preserves the fragment through the redirect. Tested on Fedora 25, the WebkitGtk version used is 2.14.1.
Attachments
Patch (15.96 KB, patch)
2017-04-03 03:03 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
Patch (16.63 KB, patch)
2017-04-03 03:59 PDT, Carlos Garcia Campos 		mcatanzaro: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2017-03-24 12:59:20 PDT
*** This bug has been marked as a duplicate of bug 158420 ***
Brady Eidson
Comment 2 2017-03-24 16:56:25 PDT
(In reply to Alexey Proskuryakov from comment #1) > > *** This bug has been marked as a duplicate of bug 158420 *** I don't think this is a duplicate of 158420, since 158420 is about a very specific case that occurs on Apple's platforms, whereas this bug is about the original use case mentioned in https://bugs.webkit.org/show_bug.cgi?id=24175 and on non-Apple platforms. WebKit itself makes no effort to preserve these anchors - It relies on the platform networking library beneath it. It seems that the curl networking port doesn't do this correctly.
Michael Catanzaro
Comment 3 2017-03-25 07:58:28 PDT
We use the soup backend, not curl.
Brady Eidson
Comment 4 2017-03-26 23:22:22 PDT
(In reply to Michael Catanzaro from comment #3) > We use the soup backend, not curl. Yup sorry, misremembered that.
Carlos Garcia Campos
Comment 5 2017-04-03 03:03:54 PDT
Created attachment 306068 [details] Patch
Carlos Garcia Campos
Comment 6 2017-04-03 03:59:33 PDT
Created attachment 306070 [details] Patch Update test expectations of iOS too.
Michael Catanzaro
Comment 7 2017-04-03 07:29:59 PDT
Comment on attachment 306070 [details] Patch LGTM, thanks! I'll let Sergio do final review. My only comment is to please file bugs for Mac WK1 and iOS.
Michael Catanzaro
Comment 8 2017-04-03 07:30:37 PDT
Comment on attachment 306070 [details] Patch Oh, I thought Sergio had commented in this bug, but I had it mixed up with another. r+ then.
Carlos Garcia Campos
Comment 9 2017-04-03 08:35:40 PDT
See the TestExpectations changes in mac an iOS, bug already exists.
Carlos Garcia Campos
Comment 10 2017-04-03 10:08:26 PDT
Committed r214807: <http://trac.webkit.org/changeset/214807>
Sergio Villar Senin
Comment 11 2017-04-04 01:00:05 PDT
Comment on attachment 306070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306070&action=review > LayoutTests/http/tests/navigation/redirect-preserves-fragment.html:3 > +<head> No need for <html>, <head> and <body> tags when specifying the DOCTYPE. Same for the other tests. > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:107 > + m_currentRequest = WTFMove(request); Not a big fan of adding another element to the internal state of the object. Also we're doing additional changes of behaviour here in case of multiple redirections because you are no longer using the first request for several checks.
Carlos Garcia Campos
Comment 12 2017-04-04 01:05:17 PDT
(In reply to Sergio Villar Senin from comment #11) > Comment on attachment 306070 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=306070&action=review > > > LayoutTests/http/tests/navigation/redirect-preserves-fragment.html:3 > > +<head> > > No need for <html>, <head> and <body> tags when specifying the DOCTYPE. Same > for the other tests. I copy-pasted it from existing tests, that what I usually do, since I don't know html, css, js :-P > > Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp:107 > > + m_currentRequest = WTFMove(request); > > Not a big fan of adding another element to the internal state of the object. Do you have a better proposal? This is done by NetworkLoad, which could be our client, we could add currentRequest() to the client interface, but I'm not sure it's worth it, TBH. > Also we're doing additional changes of behaviour here in case of multiple > redirections because you are no longer using the first request for several > checks. That's what fixes bug #158420 for us.
Brady Eidson
Comment 13 2017-04-07 10:22:05 PDT
As I commented over in https://bugs.webkit.org/show_bug.cgi?id=158420#c8, I'm not sure that *always* applying this behavior is correct. If the investigation in 158420 shows that there's some cases we shouldn't do this, the lib soup implementation should adapt to what we learn.
Note You need to log in before you can comment on or make changes to this bug.