Bug 45369 - Need to check the target frame and restore the right gesture state during the asynchronous history navigation
Summary: Need to check the target frame and restore the right gesture state during the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-07 23:21 PDT by Johnny(Jianning) Ding
Modified: 2010-09-17 09:37 PDT (History)
4 users (show)

See Also:


Attachments
fix patch v1 (10.90 KB, patch)
2010-09-08 05:52 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
fix patch v1 (11.20 KB, patch)
2010-09-08 06:15 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 (11.10 KB, patch)
2010-09-13 04:19 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v3, only allow the history reload to navigate in the self frame. (10.51 KB, patch)
2010-09-16 05:07 PDT, Johnny(Jianning) Ding
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 2010-09-07 23:21:41 PDT
The following html can bypass popup blocker in both chromium and safari. (You can also run the attached test case)
<base target="some"> 
<script>
history.go()
</script>

This bugs is to opening a page in a new frame by using history.go(0),  there are two problems in this bug.
1. WebKit needs to check the target frame to see whether it is a new frame and is allowed to open.
2. WebKit needs to restore the correct gesture state during the asynchronous history navigation. The cause of this problem is same as bug 44969, so again a Chromium test will also be added to address the popup blocker issue in Chromium side.

A patch will be sent soon.
Comment 1 Johnny(Jianning) Ding 2010-09-08 05:52:33 PDT
Created attachment 66886 [details]
fix patch v1
Comment 2 WebKit Review Bot 2010-09-08 05:54:11 PDT
Attachment 66886 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/loader/RedirectScheduler.cpp:85:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Johnny(Jianning) Ding 2010-09-08 06:15:10 PDT
Created attachment 66889 [details]
fix patch v1

fix style warning.
Comment 4 Abhishek Arya 2010-09-08 07:10:50 PDT
Thanks a lot Johnny for neutralizing yet another popup blocker bypass.
Comment 5 Adam Barth 2010-09-09 21:19:07 PDT
Comment on attachment 66889 [details]
fix patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=66889&action=prettypatch

> WebCore/loader/FrameLoader.cpp:296
> +    // Check whether a new window is allowed to create for navigation.
> +    Frame* targetFrame = m_frame->tree()->find(target);
> +    if (!targetFrame && !DOMWindow::allowPopUp(m_frame) && !isProcessingUserGesture())
> +        return;
It's lame that we need to find the targetFrame again.  What if we get a different answer the second time?  Can we move this check closing to where we actually know the targetFrame?
Comment 6 Johnny(Jianning) Ding 2010-09-13 04:19:42 PDT
Created attachment 67396 [details]
patch v2

(In reply to comment #5)

> It's lame that we need to find the targetFrame again.  What if we get a different answer the second time?  Can we move this check closing to where we actually know the targetFrame?

Thanks, Adam. I agree with you that we need to put the the check in right place.
So the way is to check the name of targetFrame in where we actually know the targetFrame as early as possible, like currently WebKit  puts block popup logic in the following situations,

a). window.open,  WebKit checks the name of targetFrame immediately in DOMWindow:open before starting creating the window and loading the URL.
b). form.submit, WebKit checks the name of targetFrame in FrameLoader::submitForm before adding it to RedirectScheduler
c). history.go(0) (what this patch wants to do),  so I guess I should put the logic in RedirectScheduler::scheduleHistoryNavigation before starting the RedirectScheduler.

Please take a look at the new patch.
Comment 7 Adam Barth 2010-09-15 17:57:03 PDT
Comment on attachment 67396 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=67396&action=prettypatch

> WebCore/loader/RedirectScheduler.cpp:355
> +        // History reload uses the base target as target frame.
> +        Frame* targetFrame = m_frame->tree()->find(m_frame->document()->baseTarget());
> +        if (!targetFrame && !DOMWindow::allowPopUp(m_frame) && !isUserGesture)
> +            return;

Is there a race condition where the target frame changes between when the navigation is scheduled and when it fires?
Comment 8 Johnny(Jianning) Ding 2010-09-16 04:54:04 PDT
(In reply to comment #7)
> Is there a race condition where the target frame changes between when the navigation is scheduled and when it fires?

Adam, you are right, there could be a race condition.
So I am interested in how IE and Firefox handle this situation. The investigation result is that they didn't allow history reload to navigate to another frame. The history reload can only be navigate to the self frame.
I think we should follow the main-stream browsers' behavior, so I drop the old patch and wrote the patch V3, please take a look.

Thanks!
Comment 9 Johnny(Jianning) Ding 2010-09-16 05:07:06 PDT
Created attachment 67782 [details]
patch v3,  only allow the history reload to navigate in the self frame.
Comment 10 Eric Seidel (no email) 2010-09-16 12:35:51 PDT
Comment on attachment 67396 [details]
patch v2

Cleared Adam Barth's review+ from obsolete attachment 67396 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Adam Barth 2010-09-16 23:08:30 PDT
Comment on attachment 67782 [details]
patch v3,  only allow the history reload to navigate in the self frame.

View in context: https://bugs.webkit.org/attachment.cgi?id=67782&action=prettypatch

> WebCore/loader/RedirectScheduler.cpp:164
> +            // To follow Firefox and IE's behavior, history reload can only be navigated to the self frame.

I'd say "can only navigate the self frame"
Comment 12 Johnny(Jianning) Ding 2010-09-16 23:54:19 PDT
(In reply to comment #11)
> (From update of attachment 67782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67782&action=prettypatch
> 
> > WebCore/loader/RedirectScheduler.cpp:164
> > +            // To follow Firefox and IE's behavior, history reload can only be navigated to the self frame.
> 
> I'd say "can only navigate the self frame"

Thanks Adam! Abhishek will help on landing it with taking your comment soon.
Comment 13 Abhishek Arya 2010-09-17 09:37:56 PDT
Committed r67716: <http://trac.webkit.org/changeset/67716>