[HBD] Intercept Flash navigations in popup windows.

[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096
Committed: https://crrev.com/d60fef6f171bacc82ecaf3f1d4b70450d49ad1cc
Cr-Commit-Position: refs/heads/master@{#440518}

[tommycli, 2016-12-20 20:46:15]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 20:47:06]

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

[tommycli, 2016-12-20 21:04:04]

Description was changed from

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=
==========

to

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096
==========

[tommycli, 2016-12-20 21:04:04]

tommycli@chromium.org changed reviewers:
+ raymes@chromium.org

[tommycli, 2016-12-20 21:05:16]

raymes: PTAL and refer to corresponding email and bug.

I remember we discussed this and decided it was best to not intercept navs in
popup windows.

I think this is harmless (since we already intercept new windows that directly
go to the download site) -- but can you remember any reason why it might not be?

[raymes, 2016-12-20 22:33:01]

Thanks!

I don't really remember why we didn't do this. Will this get intercepted before
or after the new tab is actually created? Specifically, will the decision get
tied to the right origin and WebContents? If the origin of the popup is used
that seems like it might not work? Similarly if the popup WebContents is used it
might not work in the enterprise case?

Also, would it be hard to add a test to flash_permission_browsertest.cc?

[tommycli, 2016-12-20 22:38:02]

On 2016/12/20 22:33:01, raymes wrote:
> Thanks!
> 
> I don't really remember why we didn't do this. Will this get intercepted
before
> or after the new tab is actually created? Specifically, will the decision get
> tied to the right origin and WebContents? If the origin of the popup is used
> that seems like it might not work? Similarly if the popup WebContents is used
it
> might not work in the enterprise case?

Hey - the decision to spawn a new window or not is in a separate call within
CanCreateWindow here:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...

The navigation occurs AFTER the window has been created (only if the
CanCreateWindow test passes). So theoretically, any new windows that go directly
to Flash should have already been suppressed.

Yes - the decision is tied to the correct WebContents, the popup's webcontents,
not the opener's. Can you elaborate a bit more on why using the popup
WebContents might fail in the enterprise case?

Tommy

> 
> Also, would it be hard to add a test to flash_permission_browsertest.cc?

[commit-bot: I haz the power, 2016-12-20 23:11:43]

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

[commit-bot: I haz the power, 2016-12-20 23:11:44]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-12-21 00:20:45]

raymes: PTAL again. I added a test, but the test could use a review. Thanks!

https://codereview.chromium.org/2590363003/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permissions_browsertest.cc (left):

https://codereview.chromium.org/2590363003/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permissions_browsertest.cc:38:
prompt_factory_.reset();
I had to remove this, because otherwise I got the below crash when the new test
ends. Another solution might be restoring the view_factory_ to its original
state?

#0 0x7f610197bc8e base::debug::StackTrace::StackTrace()
#1 0x7f61019a021b logging::LogMessage::~LogMessage()
#2 0x00000118207c MockPermissionPromptFactory::DoNotCreate()
#3 0x0000011824cd
_ZN4base8internal7InvokerINS0_9BindStateIPFSt10unique_ptrI16PermissionPromptSt14default_deleteIS4_EEPN7content11WebContentsEEJEEESB_E3RunEPNS0_13BindStateBaseEOSA_
#4 0x000001a1a866 PermissionRequestManager::DisplayPendingRequests()
#5 0x000002a9df65 BrowserView::OnActiveTabChanged()
#6 0x0000029b293e Browser::ActiveTabChanged()
#7 0x0000029e6fc1 TabStripModel::NotifyIfActiveTabChanged()
#8 0x0000029e5e5b TabStripModel::DetachWebContentsAt()
#9 0x0000029e5966 TabStripModel::WebContentsData::WebContentsDestroyed()
#10 0x7f60fe30e3f4 content::WebContentsImpl::~WebContentsImpl()
#11 0x7f60fe30efc9 content::WebContentsImpl::~WebContentsImpl()
#12 0x0000029eacf0 TabStripModel::InternalCloseTab()
#13 0x0000029e81dd TabStripModel::InternalCloseTabs()
#14 0x0000029e7b5c TabStripModel::CloseAllTabs()
#15 0x0000029b138e Browser::OnWindowClosing()
#16 0x000002aa0d67 BrowserView::CanClose()
#17 0x7f60ffa61a7b views::Widget::Close()
#18 0x000001bb3612 BrowserCloseManager::CloseBrowsers()
#19 0x000001bb38d5 BrowserCloseManager::TryToCloseBrowsers()
#20 0x0000019cc1b6 chrome::CloseAllBrowsers()
#21 0x0000019cc5e3 chrome::AttemptExit()

[tommycli, 2016-12-21 18:00:20]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 18:00:44]

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

[tommycli, 2016-12-21 18:08:00]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 18:08:23]

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

[tommycli, 2016-12-21 19:06:58]

raymes: PTAL, fixed the test per our chat. thanks!

[commit-bot: I haz the power, 2016-12-21 19:33:19]

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

[commit-bot: I haz the power, 2016-12-21 19:33:20]

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

[tommycli, 2016-12-21 23:25:16]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 23:25:47]

Dry run: 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-12-22 00:45:25]

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

[commit-bot: I haz the power, 2016-12-22 00:45:25]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-12-22 22:06:09]

lgtm thanks

[tommycli, 2016-12-22 22:06:54]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-12-22 22:07:15]

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-12-22 22:12:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482444414426810,
"parent_rev": "f7479b0313898ced9f9eeae770c8abded79f0006", "commit_rev":
"1fad9c66dbc419a624f7bc548cfc6e4a20238554"}

[commit-bot: I haz the power, 2016-12-22 22:12:37]

Description was changed from

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096
==========

to

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096

Review-Url: https://codereview.chromium.org/2590363003
==========

[commit-bot: I haz the power, 2016-12-22 22:12:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-22 22:21:01]

Description was changed from

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096

Review-Url: https://codereview.chromium.org/2590363003
==========

to

==========
[HBD] Intercept Flash navigations in popup windows.

This CL turns up the Flash download interception aggression - by making
it apply to popup windows as well.

This should be fairly harmless.

BUG=676096

Committed: https://crrev.com/d60fef6f171bacc82ecaf3f1d4b70450d49ad1cc
Cr-Commit-Position: refs/heads/master@{#440518}
==========

[commit-bot: I haz the power, 2016-12-22 22:21:02]

Patchset 5 (id:??) landed as
https://crrev.com/d60fef6f171bacc82ecaf3f1d4b70450d49ad1cc
Cr-Commit-Position: refs/heads/master@{#440518}