Add WebContents source to methods in WebContentsDelegate

Add WebContents source to methods in WebContentsDelegate

The added parameter helps us avoid a potential timing-related security issue. The UI can be instructed to open a new tab and show a dialog roughly at the same time, and since it's handled asynchronously the dialog may end up in front of the new tab. Knowing what web contents issued the dialog would allow us to prevent that from happening.

BUG=428793
Committed: https://crrev.com/72a5e46303c13d806475cd01aedfb1305fcde5d7
Cr-Commit-Position: refs/heads/master@{#304781}

[Mathias Hällman, 2014-10-30 15:53:06]

mathiash@opera.com changed reviewers:
+ avi@chromium.org, hans@chromium.org, mkwst@chromium.org, sky@chromium.org,
tburkard@chromium.org, tedchoc@chromium.org, torne@chromium.org,
yoz@chromium.org, yurys@chromium.org

[Mike West, 2014-10-31 14:45:11]

Can you give a little more explanation as to what you need this for? Both in the
CL description and also in the bug.

(It would also be helpful to suggest to your chosen reviewers what files you'd
like them to take a look at)

[Mathias Hällman, 2014-10-31 15:03:01]

On 2014/10/31 14:45:11, Mike West (OOO until 3rd) wrote:
> Can you give a little more explanation as to what you need this for? Both in
the
> CL description and also in the bug.
> 
> (It would also be helpful to suggest to your chosen reviewers what files you'd
> like them to take a look at)

First, my apologies for the two handfuls of reviewers. If I'm not mistaken, each
file needs an owner to review it, and I assumed each owner would know and/or see
in the interface which files are for them to review.

If it's alright I'll update the CL description after discussing it briefly here
first. As I understand it, we want this parameter to avoid a potential
timing-related security issue. The problem as stated to me was that the UI can
be instructed to open a new tab and show a dialog roughly at the same time, and
since it's handled asynchronously the dialog may end up in front of the new tab.
Knowing what web contents issued the dialog would allow us to prevent that from
happening. Perhaps this is a non-issue, or there's a different way around this
problem?

[Mathias Hällman, 2014-11-04 15:29:59]

On 2014/10/31 15:03:01, Mathias Hällman wrote:
> On 2014/10/31 14:45:11, Mike West (OOO until 3rd) wrote:
> > Can you give a little more explanation as to what you need this for? Both in
> the
> > CL description and also in the bug.
> > 
> > (It would also be helpful to suggest to your chosen reviewers what files
you'd
> > like them to take a look at)
> 
> First, my apologies for the two handfuls of reviewers. If I'm not mistaken,
each
> file needs an owner to review it, and I assumed each owner would know and/or
see
> in the interface which files are for them to review.
> 
> If it's alright I'll update the CL description after discussing it briefly
here
> first. As I understand it, we want this parameter to avoid a potential
> timing-related security issue. The problem as stated to me was that the UI can
> be instructed to open a new tab and show a dialog roughly at the same time,
and
> since it's handled asynchronously the dialog may end up in front of the new
tab.
> Knowing what web contents issued the dialog would allow us to prevent that
from
> happening. Perhaps this is a non-issue, or there's a different way around this
> problem?

Any comments on the potential issue mentioned in my previous message?

[Avi (use Gerrit), 2014-11-04 16:50:35]

BTW, philosophically, I don't have a problem adding the parameters.

[Mike West, 2014-11-05 08:29:20]

On 2014/10/31 15:03:01, Mathias Hällman wrote:
> First, my apologies for the two handfuls of reviewers. If I'm not mistaken,
each
> file needs an owner to review it, and I assumed each owner would know and/or
see
> in the interface which files are for them to review.

For some folks, the files to review are pretty clear (e.g. I can only stamp the
content/shell changes). Other folks are in higher-level OWNERS files, and could
theoretically stamp large swaths of the patch (avi@ for instance, is in
//content/OWNERS). In both cases, being explicit about which files you'd like
specific folks to stamp makes the process simpler for everyone. The code review
tool doesn't give any explicit hints on its own. :)

> If it's alright I'll update the CL description after discussing it briefly
here
> first. As I understand it, we want this parameter to avoid a potential
> timing-related security issue. The problem as stated to me was that the UI can
> be instructed to open a new tab and show a dialog roughly at the same time,
and
> since it's handled asynchronously the dialog may end up in front of the new
tab.
> Knowing what web contents issued the dialog would allow us to prevent that
from
> happening. Perhaps this is a non-issue, or there's a different way around this
> problem?

This seems reasonable, and is in-line with many of the other methods in
WebContentsDelegate which accept a source WebContents pointer.

content/shell LGTM if you update the CL description and bug accordingly.

[Mathias Hällman, 2014-11-05 09:39:43]

On 2014/11/05 08:29:20, Mike West wrote:
> On 2014/10/31 15:03:01, Mathias Hällman wrote:
> > First, my apologies for the two handfuls of reviewers. If I'm not mistaken,
> each
> > file needs an owner to review it, and I assumed each owner would know and/or
> see
> > in the interface which files are for them to review.
> 
> For some folks, the files to review are pretty clear (e.g. I can only stamp
the
> content/shell changes). Other folks are in higher-level OWNERS files, and
could
> theoretically stamp large swaths of the patch (avi@ for instance, is in
> //content/OWNERS). In both cases, being explicit about which files you'd like
> specific folks to stamp makes the process simpler for everyone. The code
review
> tool doesn't give any explicit hints on its own. :)
> 
> > If it's alright I'll update the CL description after discussing it briefly
> here
> > first. As I understand it, we want this parameter to avoid a potential
> > timing-related security issue. The problem as stated to me was that the UI
can
> > be instructed to open a new tab and show a dialog roughly at the same time,
> and
> > since it's handled asynchronously the dialog may end up in front of the new
> tab.
> > Knowing what web contents issued the dialog would allow us to prevent that
> from
> > happening. Perhaps this is a non-issue, or there's a different way around
this
> > problem?
> 
> This seems reasonable, and is in-line with many of the other methods in
> WebContentsDelegate which accept a source WebContents pointer.
> 
> content/shell LGTM if you update the CL description and bug accordingly.

Thanks. I think this is roughly what I was thinking:

sky
chrome/browser/ui/
content/public/test/

yurys
chrome/browser/devtools/
content/browser/devtools/

yoz
apps/
extensions/

hans
content/browser/device_sensors/

tburkard
chrome/browser/prerender/

torne
android_webview/

avi
chrome/browser/tab_contents/
content/browser/web_contents/
content/public/browser/

mkwst
content/shell

[Mathias Hällman, 2014-11-05 09:40:00]

mathiash@opera.com changed reviewers:
- tedchoc@chromium.org

[Torne, 2014-11-05 11:03:55]

android_webview LGTM

[hans, 2014-11-05 11:23:22]

content/browser/device_sensors/ lgtm

[yurys, 2014-11-05 13:11:00]

devtools - lgtm

[Avi (use Gerrit), 2014-11-05 16:43:54]

All of the files you asked me to review LGTM

[Yoyo Zhou, 2014-11-05 19:40:04]

apps and extensions LGTM

[Mathias Hällman, 2014-11-07 11:38:28]

mathiash@opera.com changed reviewers:
+ gavinp@chromium.org, phajdan.jr@chromium.org, pkasting@chromium.org
- sky@chromium.org, tburkard@chromium.org

[Mathias Hällman, 2014-11-07 11:40:42]

On 2014/11/05 19:40:04, Yoyo Zhou wrote:
> apps and extensions LGTM

Trying some different reviewers for the remaining files,

pkasting: chrome/browser/ui/
phajdan.jr: content/public/test/
gavinp: chrome/browser/prerender/

Can you three have a look at these folders?

[Peter Kasting, 2014-11-07 18:18:25]

LGTM

[davidben, 2014-11-07 19:08:25]

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

[davidben, 2014-11-07 19:08:26]

chrome/browser/prerender/ lgtm

https://codereview.chromium.org/691823002/diff/40001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/691823002/diff/40001/chrome/browser/prerender...
chrome/browser/prerender/prerender_manager.cc:194: bool
ShouldSuppressDialogs(WebContents* source) override {
Nit: May as well add a
  DCHECK_EQ(tab_, source);

as with the others.

[Paweł Hajdan Jr., 2014-11-13 13:10:15]

content/public/test LGTM

[Mathias Hällman, 2014-11-13 13:19:27]

mathiash@opera.com changed reviewers:
- gavinp@chromium.org

[Mathias Hällman, 2014-11-13 13:21:25]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-11-13 13:21:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691823002/60001

[commit-bot: I haz the power, 2014-11-13 13:38:32]

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

[commit-bot: I haz the power, 2014-11-13 13:38:34]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Mathias Hällman, 2014-11-18 07:49:32]

mathiash@opera.com changed reviewers:
+ oshima@chromium.org, tedchoc@chromium.org

[Mathias Hällman, 2014-11-18 07:51:51]

On 2014/11/13 13:38:34, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_dbg_tests_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

Missed some bits,

tedchoc, could you have a look at chrome/browser/android/?
oshima, athena/content/ please?

[Ted C, 2014-11-18 18:06:07]

On 2014/11/18 07:51:51, Mathias Hällman wrote:
> On 2014/11/13 13:38:34, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   android_dbg_tests_recipe on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
> 
> Missed some bits,
> 
> tedchoc, could you have a look at chrome/browser/android/?
> oshima, athena/content/ please?

chrome/browser/android/ - lgtm

[oshima, 2014-11-18 21:32:18]

athena lgtm

[Mathias Hällman, 2014-11-19 07:23:11]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-11-19 07:23:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691823002/80001

[Mathias Hällman, 2014-11-19 07:26:03]

The CQ bit was unchecked by mathiash@opera.com

[Mathias Hällman, 2014-11-19 07:26:51]

The CQ bit was checked by mathiash@opera.com

[commit-bot: I haz the power, 2014-11-19 07:28:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691823002/100001

[commit-bot: I haz the power, 2014-11-19 08:19:09]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-19 08:19:58]

Patchset 6 (id:??) landed as
https://crrev.com/72a5e46303c13d806475cd01aedfb1305fcde5d7
Cr-Commit-Position: refs/heads/master@{#304781}