Don't show a modal permission prompt if the requesting WebContents has no tab.

Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
Committed: https://crrev.com/c5c663618e3b8aa61bc71b1e39f4313ef6b41567
Cr-Commit-Position: refs/heads/master@{#435556}

[dominickn, 2016-11-30 23:08:36]

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

[dominickn, 2016-11-30 23:09:10]

Description was changed from

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
==========

to

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
==========

[dominickn, 2016-11-30 23:09:10]

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

[commit-bot: I haz the power, 2016-11-30 23:09:13]

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

[dominickn, 2016-11-30 23:24:39]

PTAL, thanks!

[commit-bot: I haz the power, 2016-11-30 23:40:33]

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

[commit-bot: I haz the power, 2016-11-30 23:40:34]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-12-01 00:31:08]

Do you know if things also act like they are dismissed in the non-modal
(infobar) case? I'm curious what codepath gets hit from contextual search in
that case, in case we can hook into the same one? Thanks!

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_dialog_delegate.cc:167: TabAndroid* tab,
Do you know how the TabAndroid lifetime works? It seems like it might be safer
to get the TabAndroid inside this class from the WebContents? We know it will
always be valid in ::CreateJavaDelegate but I'm not so sure about LinkClicked

[dominickn, 2016-12-01 01:11:54]

On 2016/12/01 00:31:08, raymes wrote:
> Do you know if things also act like they are dismissed in the non-modal
> (infobar) case? I'm curious what codepath gets hit from contextual search in
> that case, in case we can hook into the same one? Thanks!

The infobar case is fundamentally different because they don't have a tab
dependency. I'm fairly sure that in there, they simply return a nullptr instead
of the infobar object, and the API call is actually left hanging because the
callback is never run.

For dialogs, we don't return anything back to the native side, so there's no
conduit to tell C++ at creation time that creation failed.

For infobars, PermissionQueueController never actually checks whether the
infobar created by a permission prompt actually exists. If creation fails, it
basically acts like the permission request is still pending and never runs the
PermissionSetCallback. So it doesn't crash, but it doesn't actually do anything
either.

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_dialog_delegate.cc:167: TabAndroid* tab,
On 2016/12/01 00:31:08, raymes wrote:
> Do you know how the TabAndroid lifetime works? It seems like it might be safer
> to get the TabAndroid inside this class from the WebContents? We know it will
> always be valid in ::CreateJavaDelegate but I'm not so sure about LinkClicked

We already require the tab to live as long as LinkClicked; we may need to
request system permissions when the user responds to the prompt, and that
requires the Tab (see PermissionDialogController.java).

[raymes, 2016-12-01 03:22:25]

lgtm - but it would be good to understand how the infobar case fails, if we can.
Thanks!

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_dialog_delegate.cc (right):

https://codereview.chromium.org/2538253002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_dialog_delegate.cc:167: TabAndroid* tab,
On 2016/12/01 01:11:54, dominickn wrote:
> On 2016/12/01 00:31:08, raymes wrote:
> > Do you know how the TabAndroid lifetime works? It seems like it might be
safer
> > to get the TabAndroid inside this class from the WebContents? We know it
will
> > always be valid in ::CreateJavaDelegate but I'm not so sure about
LinkClicked
> 
> We already require the tab to live as long as LinkClicked; we may need to
> request system permissions when the user responds to the prompt, and that
> requires the Tab (see PermissionDialogController.java).

I see - a reference to the TabAndroid will get passed into the Java object which
owns this object. That makes sense.

[dominickn, 2016-12-01 05:00:01]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-12-01 05:00:10]

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-01 05:04:07]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480568401003100, "parent_rev":
"f13196fdbc2836e5899511d1260d8b2663228a02", "commit_rev":
"b1de0fda100c5be08c188bf0734d2bbaff373948"}

[commit-bot: I haz the power, 2016-12-01 05:04:49]

Description was changed from

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
==========

to

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
==========

[commit-bot: I haz the power, 2016-12-01 05:04:50]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-12-01 05:08:09]

Description was changed from

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640
==========

to

==========
Don't show a modal permission prompt if the requesting WebContents has no tab.

Contextual search on Android spins up a WebContents which talks to
Google search. If Google search chooses to request a permission inside
this WebContents, the modal prompt crashes due to a lack of a containing
tab. This is not an issue with the standard infobar permission prompts
as they do not have a dependency on a tab for display.

This CL changes a DCHECK for a TabAndroid to an explicit conditional,
which acts as though the permission request was dismissed if there is no
active Tab for the requesting WebContents.

BUG=668640

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

[commit-bot: I haz the power, 2016-12-01 05:08:10]

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