Bug 39418

Created attachment 56585 [details] Fix v1 Apparently it fix the bug 38754 too at least on my Linux machine.

Comment on attachment 56585 [details] Fix v1 Clearing flags on attachment: 56585 Committed r59933: <http://trac.webkit.org/changeset/59933>

All reviewed patches have been landed. Closing bug.

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

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

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.

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

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.

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.

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.

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.

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.

Thank you for explaination and review.

Comment on attachment 57091 [details] layout test v2 Clearing flags on attachment: 57091 Committed r60295: <http://trac.webkit.org/changeset/60295>

All reviewed patches have been landed. Closing bug.