Bug 182678

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:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

All

OS:

All

URL:

https://html.spec.whatwg.org/#dom-history-replacestate

Attachments:

Description	Flags
Fixed the ReplaceState bug in history.cpp and added test.	ews-watchlist: commit-queue-
Archive of layout-test-results from ews101 for mac-sierra	none
Archive of layout-test-results from ews107 for mac-sierra-wk2	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Archive of layout-test-results from ews116 for mac-sierra	none
Archive of layout-test-results from ews204 for win-future	none
Add js-test for test & Empty string check	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

huocp

Reported 2018-02-11 14:54:32 PST

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.

Attachments
Fixed the ReplaceState bug in history.cpp and added test. (2.46 KB, patch) 2018-03-05 14:57 PST, Sihui Liu	ews-watchlist: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-sierra (2.31 MB, application/zip) 2018-03-05 15:44 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.56 MB, application/zip) 2018-03-05 16:04 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.15 MB, application/zip) 2018-03-05 16:26 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews116 for mac-sierra (3.05 MB, application/zip) 2018-03-05 16:37 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews204 for win-future (12.02 MB, application/zip) 2018-03-05 17:21 PST, EWS Watchlist	no flags	Details
Add js-test for test & Empty string check (5.23 KB, patch) 2018-03-06 15:05 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (4.94 KB, patch) 2018-03-06 15:17 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (4.90 KB, patch) 2018-03-06 15:53 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (6.60 KB, patch) 2018-03-07 11:17 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (6.63 KB, patch) 2018-03-07 12:50 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (6.62 KB, patch) 2018-03-07 13:20 PST, Sihui Liu	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-02-13 16:11:26 PST

<rdar://problem/37517821>

Sihui Liu

Comment 2 2018-03-05 14:57:05 PST

Created attachment 335032 [details] Fixed the ReplaceState bug in history.cpp and added test.

Chris Dumez

Comment 3 2018-03-05 15:19:37 PST

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.

Chris Dumez

Comment 4 2018-03-05 15:33:55 PST

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.

Brady Eidson

Comment 5 2018-03-05 15:43:43 PST

(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)

EWS Watchlist

Comment 6 2018-03-05 15:44:08 PST

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

EWS Watchlist

Comment 7 2018-03-05 15:44:09 PST

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

Chris Dumez

Comment 8 2018-03-05 15:45:31 PST

Ryosuke Niwa

Comment 9 2018-03-05 15:52:03 PST

(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.

Brady Eidson

Comment 10 2018-03-05 16:01:02 PST

EWS Watchlist

Comment 11 2018-03-05 16:04:30 PST

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

EWS Watchlist

Comment 12 2018-03-05 16:04:32 PST

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

EWS Watchlist

Comment 13 2018-03-05 16:26:46 PST

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

EWS Watchlist

Comment 14 2018-03-05 16:26:55 PST

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

EWS Watchlist

Comment 15 2018-03-05 16:37:20 PST

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

EWS Watchlist

Comment 16 2018-03-05 16:37:22 PST

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

EWS Watchlist

Comment 17 2018-03-05 17:21:43 PST

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

EWS Watchlist

Comment 18 2018-03-05 17:21:54 PST

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

Sihui Liu

Comment 19 2018-03-06 15:05:08 PST

Created attachment 335144 [details] Add js-test for test & Empty string check

Chris Dumez

Comment 20 2018-03-06 15:11:22 PST

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.

Sihui Liu

Comment 21 2018-03-06 15:17:48 PST

Created attachment 335147 [details] Patch

Chris Dumez

Comment 22 2018-03-06 15:21:47 PST

Comment on attachment 335147 [details] Patch Please see review comments on previous patch iteration.

Sihui Liu

Comment 23 2018-03-06 15:53:09 PST

Created attachment 335154 [details] Patch

Chris Dumez

Comment 24 2018-03-06 15:55:58 PST

Comment on attachment 335154 [details] Patch r=me. Will wait for EWS to be green before triggering the commit queue.

Chris Dumez

Comment 25 2018-03-06 16:36:59 PST

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.

Brady Eidson

Comment 26 2018-03-06 16:57:30 PST

(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)

Sihui Liu

Comment 27 2018-03-07 11:17:11 PST

Created attachment 335201 [details] Patch

Chris Dumez

Comment 28 2018-03-07 11:23:20 PST

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.

Sihui Liu

Comment 29 2018-03-07 12:50:25 PST

Created attachment 335207 [details] Patch

Chris Dumez

Comment 30 2018-03-07 12:57:32 PST

Comment on attachment 335207 [details] Patch r=me

Sihui Liu

Comment 31 2018-03-07 13:20:34 PST

Created attachment 335209 [details] Patch

WebKit Commit Bot

Comment 32 2018-03-07 14:09:53 PST

Comment on attachment 335209 [details] Patch Clearing flags on attachment: 335209 Committed r229375: <https://trac.webkit.org/changeset/229375>

WebKit Commit Bot

Comment 33 2018-03-07 14:09:55 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.