Bug 44086 – Back button after Google search result click broken

• Component: History
• Status: RESOLVED FIXED

[Mihai Parparita]

To reproduce:

1. Go to http://www.google.com/
2. Search for [test]
3. URL changes to http://www.google.com/#hl=en&source=hp&q=test&aq=f&aqi=g10&aql=&oq=&gs_rfai=CjppO1OtpTIzsAYmgjQOpw8H5CAAAAKoEBU_QuYKi&fp=57531420c0ae60ee (this is Google search results page that doesn't load a new page)
4. Click on the first result (test.com)
5. Press back

Expected result:
1. Back at the search results for [test]

Actual result:
1. Nothing happens, the back button is disabled

I'm fairly certain this is caused by r65340 (which was to fix bug 42861). I can reproduce in today's nightly WebKit build (r65398), but not with Thursday's (r65222).

[Mihai Parparita]

Test case:

http://persistent.info/webkit/test-cases/google-result-click/results.html
(click on the "result" link, the page will print PASS or FAIL)

Background:

For the search results pages that don't trigger a page reload, Google still wants to send a referrer with the query that was done. Therefore on result click, they create an iframe with a src of the form "http://www.google.com/url?sa=t&source=web&cd=1&ved=0CCMQFjAA&url=http%3A%2F%2Fwww.test.com%2F&rct=j&q=test&ei=YyJqTMbNNIOB8gaiqsmyBA&usg=AFQjCNH21KLjC0CBkjon2DwD_CZ0HApLMw&sig2=n3-oadI9v06M390S4tA7jw"

(that's for a click on the first result of [test], which goes to www.test.com)

That serves a a small Javascript snippet:

<script>var a=parent,b=parent.google,c=location;
if(a!=window&&b){if(b.r){b.r=0;a.location.href="http://www.test.com/";c.replace("about:blank");}}else{c.replace("http://www.test.com/");};</script><noscript><META http-equiv="refresh" content="0;URL='http://www.test.com/'"></noscript>

parent.google.r is set to 1, so this boils down to parent.location.href = "http://www.test.com/"

The reason why r65340 causes by this bug is because the parent.location.href happens inline (before onload), and is not (directly) in response to a user gesture, therefore the back/forward list is locked. Though I'm actually surprised that this is considered to be before onload, since we check targetFrame->loader()->documentLoader()->isLoadingInAPISense(), and the target frame should be the parent, which should have finished loading long ago.

[Mihai Parparita]

(In reply to comment #1)
> Though I'm actually surprised that this is considered to be before onload, since we check targetFrame->loader()->documentLoader()->isLoadingInAPISense(), and the target frame should be the parent, which should have finished loading long ago.

Ah, I should have looked at isLoadingInAPISense more carefully:

bool DocumentLoader::isLoadingInAPISense() const
{
    // Once a frame has loaded, we no longer need to consider subresources,
    // but we still need to consider subframes.
    if (frameLoader()->state() != FrameStateComplete) {
        ...
    }
    return frameLoader()->subframeIsLoading();
}

[Mihai Parparita]

Created attachment 64595 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 64595 [details]
Patch

As discussed in person, I don't think this quite does what we want.  I think
we want to know if the 'load' event fired, but isMainFrameLoading could return
true before that since it only inspects the state of the current frame.

[Mihai Parparita]

Comment on attachment 64595 [details]
Patch

Nevermind about reviewing this, we discussed this in person and the isMainFrameLoading method isn't quite the right way to go.

[Mihai Parparita]

Created attachment 64607 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 64607 [details]
Patch

Rejecting patch 64607 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20854 test cases.
transitions/change-values-during-transition.html -> failed

Exiting early after 1 failures. 19757 tests run.
599.78s total testing time

19756 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
281 test cases (1%) had stderr output

Full output: http://queues.webkit.org/results/3755295

[Mihai Parparita]

(In reply to comment #7)
> transitions/change-values-during-transition.html -> failed

I can't reproduce this locally (ran all the transitions tests 10 times), and per bug 28461, bug 27638 and bug 32946, that test is flaky.

Darin, do you mind giving this a cq+ again?

[WebKit Commit Bot]

Comment on attachment 64607 [details]
Patch

Clearing flags on attachment: 64607

Committed r65573: <http://trac.webkit.org/changeset/65573>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65573 might have broken Qt Windows 32-bit Release

[Eric Seidel (no email)]

Yes, it's a Leopard + CoreVideo problem.  I should just skip all media and composition tests for the commit-queue. :(

[Mihai Parparita]

(In reply to comment #11)
> http://trac.webkit.org/changeset/65573 might have broken Qt Windows 32-bit Release

Filed bug 44145 about that.

[Mihai Parparita]

Created attachment 65291 [details]
Patch

[Mihai Parparita]

Comment on attachment 65291 [details]
Patch

Oops, attached to the wrong bug.