Bug 44086 - Back button after Google search result click broken
Summary: Back button after Google search result click broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-16 19:02 PDT by Mihai Parparita
Modified: 2010-08-24 10:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.42 KB, patch)
2010-08-17 08:11 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (11.44 KB, patch)
2010-08-17 10:39 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (15.36 KB, text/plain)
2010-08-24 10:23 PDT, Mihai Parparita
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-08-16 19:02:00 PDT
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).
Comment 1 Mihai Parparita 2010-08-17 07:23:43 PDT
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.
Comment 2 Mihai Parparita 2010-08-17 07:26:08 PDT
(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();
}
Comment 3 Mihai Parparita 2010-08-17 08:11:25 PDT
Created attachment 64595 [details]
Patch
Comment 4 Darin Fisher (:fishd, Google) 2010-08-17 09:21:01 PDT
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.
Comment 5 Mihai Parparita 2010-08-17 09:22:17 PDT
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.
Comment 6 Mihai Parparita 2010-08-17 10:39:05 PDT
Created attachment 64607 [details]
Patch
Comment 7 WebKit Commit Bot 2010-08-17 12:25:20 PDT
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
Comment 8 Mihai Parparita 2010-08-17 12:40:15 PDT
(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?
Comment 9 WebKit Commit Bot 2010-08-17 16:40:29 PDT
Comment on attachment 64607 [details]
Patch

Clearing flags on attachment: 64607

Committed r65573: <http://trac.webkit.org/changeset/65573>
Comment 10 WebKit Commit Bot 2010-08-17 16:40:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Review Bot 2010-08-17 16:59:47 PDT
http://trac.webkit.org/changeset/65573 might have broken Qt Windows 32-bit Release
Comment 12 Eric Seidel (no email) 2010-08-17 17:13:21 PDT
Yes, it's a Leopard + CoreVideo problem.  I should just skip all media and composition tests for the commit-queue. :(
Comment 13 Mihai Parparita 2010-08-17 17:36:58 PDT
(In reply to comment #11)
> http://trac.webkit.org/changeset/65573 might have broken Qt Windows 32-bit Release

Filed bug 44145 about that.
Comment 14 Mihai Parparita 2010-08-24 10:23:50 PDT
Created attachment 65291 [details]
Patch
Comment 15 Mihai Parparita 2010-08-24 10:24:47 PDT
Comment on attachment 65291 [details]
Patch

Oops, attached to the wrong bug.