Bug 39418 – history.pushState doesn't work for the first page in a window.

• Component: History
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

history.pushState doesn't do enything for the first page in a window. Bug was introduced by r59815 (bug 38840).

[Jędrzej Nowacki]

Created attachment 56585 [details]
Fix v1

Apparently it fix the bug 38754 too at least on my Linux machine.

[WebKit Commit Bot]

Comment on attachment 56585 [details]
Fix v1

Clearing flags on attachment: 56585

Committed r59933: <http://trac.webkit.org/changeset/59933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

This would've been easy to add a layouttest for, wouldn't it have?

[Jędrzej Nowacki]

(In reply to comment #4)
> This would've been easy to add a layouttest for, wouldn't it have?
It will be a bit redundant, but easy :-)

[Jędrzej Nowacki]

Created attachment 57020 [details]
layout test v1

New test, after load event, calls the pushState a few times and check if it change the history length property.

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #4)
> > This would've been easy to add a layouttest for, wouldn't it have?
> It will be a bit redundant, but easy :-)

I don't understand this comment.

There was a bug - you couldn't call pushState after the initial load in a new window.

If a layouttest was catching this case, then that layout test should've been failing.
If a layouttest was NOT catching this case, then adding one would not be redundant.

[Brady Eidson]

Comment on attachment 57020 [details]
layout test v1

I'm r-'ing because I'm missing how this test actually works under the run-webkit-tests harness.

Yes, if I open it in a new Safari window, for example, it would be exercising the code path that used to have this problem.

But run-webkit-tests reuses the same WebView for a series of tests.  Therefore - unless it goes first - this test won't actually excercise the buggy code path.

[Brady Eidson]

There is a fairly simple invariant for layouttests.

They need to fail before the WebKit code change is applied, and pass after the WebKit code change is applied.

Unless I'm missing something, I don't see how this test would fail before r59933 was applied.

Note that by failure, I mean in the context of run-webkit-tests.

There is a way to get DRT to run the test in a fresh WebView *even* in the context of run-webkit-tests, and that is to run the test in a new window.

[Jędrzej Nowacki]

Created attachment 57091 [details]
layout test v2

(In reply to comment #8)
> (From update of attachment 57020 [details])
> (...) But run-webkit-tests reuses the same WebView for a series of tests.  Therefore - unless it goes first - this test won't actually excercise the buggy code path.
Wow, I didn't know, it is evil :-).

(In reply to comment #7)
> If a layouttest was catching this case, then that layout test should've been failing.
> If a layouttest was NOT catching this case, then adding one would not be redundant.
I thought that bug was exposed after some other changes. For me document-destroyed-navigate-back-with-fragment-scroll.html is failing without the patch, but only when it is run as a first test, I haven't noticed ordering issue. It is why I wrote that a new test would be a bit redundant.

(In reply to comment #9)
> (...) There is a way to get DRT to run the test in a fresh WebView *even* in the context of run-webkit-tests, and that is to run the test in a new window.
New patch use this trick. Shouldn't we fix DRT? The bug wasn't caught only because of this "feature", for example document-destroyed-navigate-back-with-fragment-scroll.html covers similar use case.

[Darin Adler]

Running all the tests in the same WebView isn't a "trick", it's just how the tests are run, and it’s closer to the normal model of web browsing. Making a new WebView every time would both be slower and would hide other kinds of bugs. Most tests aren’t sensitively dependent on whether the WebView is new.

[Brady Eidson]

Comment on attachment 57091 [details]
layout test v2

As Darin mentioned, this isn't a bug in DRT, but a conscious decision.  The type of test that relies on having a fresh WebView is actually quite the rare exception.

Thanks for adding this test.

[Jędrzej Nowacki]

Thank you for explaination and review.

[WebKit Commit Bot]

Comment on attachment 57091 [details]
layout test v2

Clearing flags on attachment: 57091

Committed r60295: <http://trac.webkit.org/changeset/60295>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.