Don't allow deferred frames to create new windows.

Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG=616907
Committed: https://crrev.com/25a8ae78165cd8725465d26e02a342d7121c42f4
Cr-Commit-Position: refs/heads/master@{#403640}

[Nate Chapin, 2016-06-09 18:29:40]

japhet@chromium.org changed reviewers:
+ japhet@chromium.org

[Nate Chapin, 2016-06-09 18:29:41]

Cod change LGTM. Is your repro in bug 616907 viable as a layout test?

[Mariusz Mlynski, 2016-06-09 18:54:11]

On 2016/06/09 18:29:41, Nate Chapin wrote:
> Cod change LGTM. Is your repro in bug 616907 viable as a layout test?

Thanks for the review. It'd be difficult to make a layout test from it, it's
necessary to spin a nested message loop to defer loads.

[Nate Chapin, 2016-06-09 19:05:46]

That's what I was afraid of. It might be worth taking a quick look as to whether
ChromeClientImplTest can tickle it, but if that's not obviously feasible, this
is fine to land as-is.

[Mariusz Mlynski, 2016-06-09 19:54:35]

On 2016/06/09 19:05:46, Nate Chapin wrote:
> That's what I was afraid of. It might be worth taking a quick look as to
whether
> ChromeClientImplTest can tickle it, but if that's not obviously feasible, this
> is fine to land as-is.

At a glance, it should be feasible, but I have zero experience writing unit
tests, so it might take a little time and possibly some guidance along the way.
Should I hook up to GetNavigationPolicyTest or create a separate class for this
one test? What I guess needs to be done is:

1) Defer loads on the page: m_webView->page()->setDefersLoading(true);
2) Verify that createWindow returns null: EXPECT_EQ(nullptr,
m_chromeClientImpl->createWindow(m_mainFrame, someFrameLoadRequest,
someWindowFeatures, ?WebNavigationPolicyNewWindow?, MaybeSetOpener));
3) Maybe m_webView->page()->setDefersLoading(false); to clean up.
4) Some more #includes to handle createWindow arguments in #2.

[Nate Chapin, 2016-06-10 21:28:01]

On 2016/06/09 19:54:35, Mariusz Mlynski wrote:
> On 2016/06/09 19:05:46, Nate Chapin wrote:
> > That's what I was afraid of. It might be worth taking a quick look as to
> whether
> > ChromeClientImplTest can tickle it, but if that's not obviously feasible,
this
> > is fine to land as-is.
> 
> At a glance, it should be feasible, but I have zero experience writing unit
> tests, so it might take a little time and possibly some guidance along the
way.
> Should I hook up to GetNavigationPolicyTest or create a separate class for
this
> one test? What I guess needs to be done is:

I'd lean toward creating a new class in ChromeClientImplTest.cpp, but if you
need some of the helpers in GetNavigationPolicyTest, I don't think it's much of
a problem to use it instead.

> 
> 1) Defer loads on the page: m_webView->page()->setDefersLoading(true);
> 2) Verify that createWindow returns null: EXPECT_EQ(nullptr,
> m_chromeClientImpl->createWindow(m_mainFrame, someFrameLoadRequest,
> someWindowFeatures, ?WebNavigationPolicyNewWindow?, MaybeSetOpener));
> 3) Maybe m_webView->page()->setDefersLoading(false); to clean up.
> 4) Some more #includes to handle createWindow arguments in #2.

That sounds sufficient to me.

[Mariusz Mlynski, 2016-06-14 05:58:12]

Nate, could you PTAL at the test?

[Nate Chapin, 2016-06-14 21:42:13]

LGTM, thanks!

[kinuko, 2016-07-04 03:11:07]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2016-07-04 03:11:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2016-07-04 03:13:27]

I think we want this patch in, let me try CQ-ing now (in case you've been
waiting one more stamp-- lgtm/2 and sorry for now following up)

[commit-bot: I haz the power, 2016-07-04 03:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-04 03:24:11]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mariusz Mlynski, 2016-07-04 04:21:30]

WebView::create was modified a few days ago, please try again.

[kinuko, 2016-07-04 04:58:49]

On 2016/07/04 04:21:30, Mariusz Mlynski wrote:
> WebView::create was modified a few days ago, please try again.

Yup-- I can rebase and retry.

[kinuko, 2016-07-04 04:59:08]

Ah ok, you already updated.. thanks!

[kinuko, 2016-07-04 04:59:19]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2016-07-04 04:59:19]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2035973002/#ps40001
(title: "WebView::create update")

[commit-bot: I haz the power, 2016-07-04 04:59:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 06:25:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-04 06:28:23]

Description was changed from

==========
Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG=616907
==========

to

==========
Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG=616907

Committed: https://crrev.com/25a8ae78165cd8725465d26e02a342d7121c42f4
Cr-Commit-Position: refs/heads/master@{#403640}
==========

[commit-bot: I haz the power, 2016-07-04 06:28:25]

Patchset 3 (id:??) landed as
https://crrev.com/25a8ae78165cd8725465d26e02a342d7121c42f4
Cr-Commit-Position: refs/heads/master@{#403640}