| Summary: | replaceState cause back/forward malfunction on html page with <base href="/"> tag | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | huocp | ||||||||||||||||||||||||||
| Component: | History | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | beidson, cdumez, commit-queue, ews-watchlist, rniwa, sihui_liu, webkit-bug-importer | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
| Version: | Safari Technology Preview | ||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||
| URL: | https://html.spec.whatwg.org/#dom-history-replacestate | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
Created attachment 335032 [details]
Fixed the ReplaceState bug in history.cpp and added test.
Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. View in context: https://bugs.webkit.org/attachment.cgi?id=335032&action=review > LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 > +ALERT: Current location is http://127.0.0.1:8000/one This does not tell me if this is a PASS or a failure. Please use js-test.js and the shouldBeEqualtoString() helper. > LayoutTests/http/tests/history/replacestate-no-url.html:12 > + history.pushState({}, 'page 1', '/one'); indentation looks weird. No tabs. We should only use spaces (4). > LayoutTests/http/tests/history/replacestate-no-url.html:20 > + if(window.testRunner) { As per coding style, no curly brackets for one liners. > LayoutTests/http/tests/history/replacestate-no-url.html:21 > + testRunner.notifyDone(); Indentation problem. > Source/WebCore/ChangeLog:3 > + replaceState cause back/forward malfunction on html page with <base href="/"> tag You're missing a changeLog for the layout test. > Source/WebCore/ChangeLog:9 > + Test: http/tests/history/replacestate-no-url.html This line should come *after* the description. > Source/WebCore/ChangeLog:11 > + URL should not be changed after ReplaceState if the optional URL argument is null. This line should be before the "Test:" line. > Source/WebCore/page/History.cpp:159 > URL baseURL = m_frame->document()->baseURL(); The change looks good but I think we should rewrite this to rely on Document::completeURL(). completeURL() already has the logic to resolve the base URL. Comment on attachment 335032 [details]
Fixed the ReplaceState bug in history.cpp and added test.
Your new test seems to be failing on Mac-wk1 EWS bot.
(In reply to Chris Dumez from comment #3) > Comment on attachment 335032 [details] > Fixed the ReplaceState bug in history.cpp and added test. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335032&action=review > > > LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 > > +ALERT: Current location is http://127.0.0.1:8000/one > > This does not tell me if this is a PASS or a failure. Please use js-test.js > and the shouldBeEqualtoString() helper. I disagree with the necessity in doing this (But it's a good exercise for someone picking up coding, regardless) Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. Attachment 335032 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6817024 New failing tests: http/tests/history/replacestate-no-url.html Created attachment 335042 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. View in context: https://bugs.webkit.org/attachment.cgi?id=335032&action=review >>> LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 >>> +ALERT: Current location is http://127.0.0.1:8000/one >> >> This does not tell me if this is a PASS or a failure. Please use js-test.js and the shouldBeEqualtoString() helper. > > I disagree with the necessity in doing this (But it's a good exercise for someone picking up coding, regardless) Wether you agree with using js-test.js or not. The message should still make it clear if it is a PASS or a FAIL. Otherwise, someone may just rebaseline the test if it starts printing out a different URL. (In reply to Chris Dumez from comment #8) > Comment on attachment 335032 [details] > Fixed the ReplaceState bug in history.cpp and added test. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335032&action=review > > >>> LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 > >>> +ALERT: Current location is http://127.0.0.1:8000/one > >> > >> This does not tell me if this is a PASS or a failure. Please use js-test.js and the shouldBeEqualtoString() helper. > > > > I disagree with the necessity in doing this (But it's a good exercise for someone picking up coding, regardless) > > Wether you agree with using js-test.js or not. The message should still make > it clear if it is a PASS or a FAIL. Otherwise, someone may just rebaseline > the test if it starts printing out a different URL. Indeed. It's very important for each test to make it clear whether something is passing or failing. I wish we were more disciplined about this throughout our project. (In reply to Chris Dumez from comment #8) > Comment on attachment 335032 [details] > Fixed the ReplaceState bug in history.cpp and added test. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335032&action=review > > >>> LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 > >>> +ALERT: Current location is http://127.0.0.1:8000/one > >> > >> This does not tell me if this is a PASS or a failure. Please use js-test.js and the shouldBeEqualtoString() helper. > > > > I disagree with the necessity in doing this (But it's a good exercise for someone picking up coding, regardless) > > Wether you agree with using js-test.js or not. The message should still make > it clear if it is a PASS or a FAIL. Otherwise, someone may just rebaseline > the test if it starts printing out a different URL. Sorry, yes, that's fine - That's all I meant. Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. Attachment 335032 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6817231 New failing tests: http/tests/history/replacestate-no-url.html Created attachment 335044 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. Attachment 335032 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6817250 New failing tests: http/tests/history/replacestate-no-url.html Created attachment 335047 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. Attachment 335032 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6817413 New failing tests: http/tests/history/replacestate-no-url.html Created attachment 335049 [details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test. Attachment 335032 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6817769 New failing tests: http/tests/history/replacestate-no-url.html Created attachment 335057 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 335144 [details]
Add js-test for test & Empty string check
Comment on attachment 335144 [details] Add js-test for test & Empty string check View in context: https://bugs.webkit.org/attachment.cgi?id=335144&action=review > LayoutTests/http/tests/history/replacestate-no-url-expected.txt:1 > \ No newline at end of file We'll see if the bots are happy but this line seems surprising. > LayoutTests/http/tests/history/replacestate-no-url.html:5 > + <script src="../../js-test-resources/js-test.js"></script> <script src="/js-test-resources/js-test.js"></script> since your test is in the http folder. > LayoutTests/http/tests/history/replacestate-no-url.html:7 > + window.jsTestIsAsync = true; No need for "window." > LayoutTests/http/tests/history/replacestate-no-url.html:33 > + shouldBeEqualToString(JSON.stringify(document.location.href), "http://127.0.0.1:8000/one"); shouldBeEqualToString("document.location.href", "http://127.0.0.1:8000/one"); > LayoutTests/http/tests/history/replacestate-no-url.html:37 > + shouldBeEqualToString(JSON.stringify(document.location.href), "http://127.0.0.1:8000/"); shouldBeEqualToString("document.location.href", "http://127.0.0.1:8000/"); > Source/WebCore/page/History.cpp:161 > + else return m_frame->document()->completeURL(urlString); return statement should be on the next line as per coding style. Also, since the if case returns, you don't need an else. if (urlString.isNull()) return m_frame->document()->url(); return m_frame->document()->completeURL(urlString); or maybe even a ternary on a single line. Created attachment 335147 [details]
Patch
Comment on attachment 335147 [details]
Patch
Please see review comments on previous patch iteration.
Created attachment 335154 [details]
Patch
Comment on attachment 335154 [details]
Patch
r=me. Will wait for EWS to be green before triggering the commit queue.
Comment on attachment 335154 [details]
Patch
Looks like this is causing fast/loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html to fail on EWS. Please take a look.
(In reply to Chris Dumez from comment #25) > Comment on attachment 335154 [details] > Patch > > Looks like this is causing > fast/loader/stateobjects/pushstate-with-fragment-urls-and-hashchange.html to > fail on EWS. Please take a look. Note that the answer might be that the test was actually broken, we didn't realize it, and this patch fixes it. In which case we just need new expectations. (I can't verify this theory myself because I can't see what the failure is/was) Created attachment 335201 [details]
Patch
Comment on attachment 335201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335201&action=review > Source/WebCore/ChangeLog:7 > + Unreviewed. See comment below. > LayoutTests/ChangeLog:7 > + Unreviewed. No, you need to keep the original reviewed by nobody line or the bot won’t be able to update this line before landing. > LayoutTests/ChangeLog:9 > + Add layout test coverage. Please move this under the entries for your new test. > LayoutTests/ChangeLog:14 > + Rebaseline a layout test as empty string for URL is handled differently. This should be under the file you changed, not over. Created attachment 335207 [details]
Patch
Comment on attachment 335207 [details]
Patch
r=me
Created attachment 335209 [details]
Patch
Comment on attachment 335209 [details] Patch Clearing flags on attachment: 335209 Committed r229375: <https://trac.webkit.org/changeset/229375> All reviewed patches have been landed. Closing bug. |
To reproduce the issue. 1. create an static index.html file with following content <!doctype html> <html> <head> <title>test</title> <base href="/"> </head> <body> </body> 2. serve the file with same http server, for instance use nodejs http-server (npm install -g http-server) 3. use Safari to open the local site. (in Safari preference, turn on "Show full website address" in advanced tab) 4. in Safari dev console, do following line by line history.pushState({}, "", "/one"); // browser shows url "/one", as expected. history.replaceState({}, ""); // 3rd param is optional new url, we leave it out to not override it. history.pushState({}, "", "/two"); // browser shows url "/two", as expected. history.back(); // browser shows url "" instead of "/one". This is the bug! This bug only appears on webpage with `<base href="/some_base">` tag. This bug appears on latest macOS Safari 11.0.3, latest iOS Safari 11, latest macOS Safari Technology Preview release 49. I did not test on any older version of Safari. As a reference, this bug doesn't affect Chrome or Firefox. To bypass this bug, I need to feed current url back to replaceState. history.replaceState({}, "", "/one"); // bypass the bug.