Bug 182678 - replaceState cause back/forward malfunction on html page with <base href="/"> tag
Summary: replaceState cause back/forward malfunction on html page with <base href="/">...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Normal
Assignee: Sihui Liu
URL: https://html.spec.whatwg.org/#dom-his...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-11 14:54 PST by huocp
Modified: 2018-03-07 14:09 PST (History)
7 users (show)

See Also:


Attachments
Fixed the ReplaceState bug in history.cpp and added test. (2.46 KB, patch)
2018-03-05 14:57 PST, Sihui Liu
ews: 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, Build Bot
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, Build Bot
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, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.05 MB, application/zip)
2018-03-05 16:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews204 for win-future (12.02 MB, application/zip)
2018-03-05 17:21 PST, Build Bot
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

Note You need to log in before you can comment on or make changes to this bug.
Description huocp 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.
Comment 1 Radar WebKit Bug Importer 2018-02-13 16:11:26 PST
<rdar://problem/37517821>
Comment 2 Sihui Liu 2018-03-05 14:57:05 PST
Created attachment 335032 [details]
Fixed the ReplaceState bug in history.cpp and added test.
Comment 3 Chris Dumez 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.
Comment 4 Chris Dumez 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.
Comment 5 Brady Eidson 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)
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Chris Dumez 2018-03-05 15:45:31 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.
> 
> 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.
Comment 9 Ryosuke Niwa 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.
Comment 10 Brady Eidson 2018-03-05 16:01:02 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.

Sorry, yes, that's fine - That's all I meant.
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Build Bot 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
Comment 18 Build Bot 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
Comment 19 Sihui Liu 2018-03-06 15:05:08 PST
Created attachment 335144 [details]
Add js-test for test & Empty string check
Comment 20 Chris Dumez 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.
Comment 21 Sihui Liu 2018-03-06 15:17:48 PST
Created attachment 335147 [details]
Patch
Comment 22 Chris Dumez 2018-03-06 15:21:47 PST
Comment on attachment 335147 [details]
Patch

Please see review comments on previous patch iteration.
Comment 23 Sihui Liu 2018-03-06 15:53:09 PST
Created attachment 335154 [details]
Patch
Comment 24 Chris Dumez 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.
Comment 25 Chris Dumez 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.
Comment 26 Brady Eidson 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)
Comment 27 Sihui Liu 2018-03-07 11:17:11 PST
Created attachment 335201 [details]
Patch
Comment 28 Chris Dumez 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.
Comment 29 Sihui Liu 2018-03-07 12:50:25 PST
Created attachment 335207 [details]
Patch
Comment 30 Chris Dumez 2018-03-07 12:57:32 PST
Comment on attachment 335207 [details]
Patch

r=me
Comment 31 Sihui Liu 2018-03-07 13:20:34 PST
Created attachment 335209 [details]
Patch
Comment 32 WebKit Commit Bot 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>
Comment 33 WebKit Commit Bot 2018-03-07 14:09:55 PST
All reviewed patches have been landed.  Closing bug.