Bug 45369 – Need to check the target frame and restore the right gesture state during the asynchronous history navigation

• Component: Page Loading
• Status: RESOLVED FIXED

[Johnny(Jianning) Ding]

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.

[Johnny(Jianning) Ding]

Created attachment 66886 [details]
fix patch v1

[WebKit Review Bot]

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.

[Johnny(Jianning) Ding]

Created attachment 66889 [details]
fix patch v1

fix style warning.

[Abhishek Arya]

Thanks a lot Johnny for neutralizing yet another popup blocker bypass.

[Adam Barth]

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?

[Johnny(Jianning) Ding]

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.

[Adam Barth]

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?

[Johnny(Jianning) Ding]

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

[Johnny(Jianning) Ding]

Created attachment 67782 [details]
patch v3,  only allow the history reload to navigate in the self frame.

[Eric Seidel (no email)]

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.

[Adam Barth]

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"

[Johnny(Jianning) Ding]

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

[Abhishek Arya]

Committed r67716: <http://trac.webkit.org/changeset/67716>