Fix leaking page visits in incognito mode via bookmarked favicons

Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently
broken on mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
Review-Url: https://chromiumcodereview.appspot.com/2694333002
Cr-Commit-Position: refs/heads/master@{#500976}
Committed: https://chromium.googlesource.com/chromium/src/+/62a06efac826e30ae2683b64a6eccf628899de4c

[mastiz, 2017-03-31 14:02:55]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

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

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

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

[mastiz, 2017-03-31 14:08:59]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

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

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

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

[mastiz, 2017-03-31 14:53:14]

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

[commit-bot: I haz the power, 2017-03-31 14:53:35]

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

[mastiz, 2017-03-31 14:54:36]

mastiz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[mastiz, 2017-03-31 14:54:36]

Hey peter, I haven't tested this locally, but before bugging sky@, do you have
an idea why this was introduced in the first place?

[commit-bot: I haz the power, 2017-03-31 15:01:08]

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

[commit-bot: I haz the power, 2017-03-31 15:01:09]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pkotwicz, 2017-03-31 15:49:03]

The current code updates bookmark favicons when a user visits the bookmarked
page in incognito

With your change, if a user bookmarks a site and visits it in incognito, the
bookmark's favicon will never be updated

[mastiz, 2017-04-04 12:17:54]

On 2017/03/31 15:49:03, pkotwicz wrote:
> The current code updates bookmark favicons when a user visits the bookmarked
> page in incognito
> 
> With your change, if a user bookmarks a site and visits it in incognito, the
> bookmark's favicon will never be updated

Correct, this is precisely the intent, and I suspect privacy would consider this
desirable.

I think the original bug was mostly about caching the icon if the bookmark was
*created* in incognito, not simply visited.

Any concerns from your side? Note that UpdateFaviconMappingsAndFetch() is not
called during bookmark creation, i.e. only SetFavicons.

[pkotwicz, 2017-04-04 13:06:33]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2017-04-04 13:06:33]

Scott, can you please take a look

Punting this CL to Scott who is the owner of bookmarks

[sky, 2017-04-04 16:59:11]

Please add test coverage.

[mastiz, 2017-04-04 18:16:40]

On 2017/04/04 16:59:11, sky wrote:
> Please add test coverage.

Will do.

Meanwhile, do you think this is the right fix or, since you are the original
author of this logic (linked in the CL description), recall what the original
rationale was? Do I understand correctly that the intent was fixing bookmarks
*created* in incognito, and perhaps the *update* of mappings in incognito was
not thought through?

[sky, 2017-04-04 20:41:55]

I intended both. My thinking was that if you've bookmarked a page than it's
important and you want the favicon kept up to date, even in incognito mode.
I see the rationale you've outlined, but I think that is not the common
case. Consider if you're some one that always browses in incognito mode,
wouldn't you want the favicon kept up to date? Or more specifically,
wouldn't you file a bug that the favicon never changed?

  -Scott

On Tue, Apr 4, 2017 at 11:16 AM, <mastiz@chromium.org> wrote:

> On 2017/04/04 16:59:11, sky wrote:
> > Please add test coverage.
>
> Will do.
>
> Meanwhile, do you think this is the right fix or, since you are the
> original
> author of this logic (linked in the CL description), recall what the
> original
> rationale was? Do I understand correctly that the intent was fixing
> bookmarks
> *created* in incognito, and perhaps the *update* of mappings in incognito
> was
> not thought through?
>
> https://codereview.chromium.org/2694333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-04-05 07:44:33]

Thanks Scott.

On 2017/04/04 20:41:55, sky wrote:
> I intended both. My thinking was that if you've bookmarked a page than it's
> important and you want the favicon kept up to date, even in incognito mode.
> I see the rationale you've outlined, but I think that is not the common
> case. Consider if you're some one that always browses in incognito mode,
> wouldn't you want the favicon kept up to date? Or more specifically,
> wouldn't you file a bug that the favicon never changed?

The favicon would be updated in the UI, although not cached. This might even
lead to repeated flickering every time you visit a new page (i.e. the old
favicon would be grabbed first, and then the new one fetched), not a great user
experience.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=708447

While reproducing the issue, I realized:
1. Bookmarking pages in incognito doesn't store favicons, on Android (same
https://bugs.chromium.org/p/chromium/issues/detail?id=22670)
2. Assuming (1) is fixed, the reason why this privacy issue is exploitable is
that it's easy to bookmark a page *before* a favicon has been cached.

I'll work on these two, I assume the privacy issue will be considered of lower
severity once it's hard to exploit.

> 
>   -Scott
> 
> On Tue, Apr 4, 2017 at 11:16 AM, <mailto:mastiz@chromium.org> wrote:
> 
> > On 2017/04/04 16:59:11, sky wrote:
> > > Please add test coverage.
> >
> > Will do.
> >
> > Meanwhile, do you think this is the right fix or, since you are the
> > original
> > author of this logic (linked in the CL description), recall what the
> > original
> > rationale was? Do I understand correctly that the intent was fixing
> > bookmarks
> > *created* in incognito, and perhaps the *update* of mappings in incognito
> > was
> > not thought through?
> >
> > https://codereview.chromium.org/2694333002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-04-05 08:16:03]

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

[commit-bot: I haz the power, 2017-04-05 08:16:23]

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

[mastiz, 2017-04-05 08:17:02]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2017-04-05 08:18:21]

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

[commit-bot: I haz the power, 2017-04-05 08:18:44]

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, 2017-04-05 08:28:05]

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

[commit-bot: I haz the power, 2017-04-05 08:28:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-04-05 08:35:20]

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

[commit-bot: I haz the power, 2017-04-05 08:35:36]

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, 2017-04-05 08:50:52]

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

[commit-bot: I haz the power, 2017-04-05 08:50:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-04-05 10:24:56]

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

[commit-bot: I haz the power, 2017-04-05 10:25:07]

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, 2017-04-05 10:42:03]

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

[commit-bot: I haz the power, 2017-04-05 10:42:04]

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

[mastiz, 2017-04-05 11:16:35]

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

[commit-bot: I haz the power, 2017-04-05 11:16:49]

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, 2017-04-05 11:34:21]

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

[commit-bot: I haz the power, 2017-04-05 11:34:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mastiz, 2017-09-04 10:13:53]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

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

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

[mastiz, 2017-09-04 10:15:57]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently broken on
mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

[mastiz, 2017-09-04 17:22:16]

sky@: sorry for the long silence but I'd like to ping this thread and wrap it
up, comments below.

On 2017/04/05 07:44:33, mastiz wrote:
> Thanks Scott.
> 
> On 2017/04/04 20:41:55, sky wrote:
> > I intended both. My thinking was that if you've bookmarked a page than it's
> > important and you want the favicon kept up to date, even in incognito mode.
> > I see the rationale you've outlined, but I think that is not the common
> > case. Consider if you're some one that always browses in incognito mode,
> > wouldn't you want the favicon kept up to date? Or more specifically,
> > wouldn't you file a bug that the favicon never changed?
> 
> The favicon would be updated in the UI, although not cached. This might even
> lead to repeated flickering every time you visit a new page (i.e. the old
> favicon would be grabbed first, and then the new one fetched), not a great
user
> experience.
> 
> I filed https://bugs.chromium.org/p/chromium/issues/detail?id=708447
> 
> While reproducing the issue, I realized:
> 1. Bookmarking pages in incognito doesn't store favicons, on Android (same
> https://bugs.chromium.org/p/chromium/issues/detail?id=22670)

Filed crbug.com/761764 to track this (orthogonal) issue.

> 2. Assuming (1) is fixed, the reason why this privacy issue is exploitable is
> that it's easy to bookmark a page *before* a favicon has been cached.
> 
> I'll work on these two, I assume the privacy issue will be considered of lower
> severity once it's hard to exploit.

As per the actual bug addressed here, battre@ from privacy did speak in favor of
this change (https://bugs.chromium.org/p/chromium/issues/detail?id=708447#c3).
Are you fine with that or who can make a final call?

Thanks,
Mikel

> 
> > 
> >   -Scott
> > 
> > On Tue, Apr 4, 2017 at 11:16 AM, <mailto:mastiz@chromium.org> wrote:
> > 
> > > On 2017/04/04 16:59:11, sky wrote:
> > > > Please add test coverage.
> > >
> > > Will do.
> > >
> > > Meanwhile, do you think this is the right fix or, since you are the
> > > original
> > > author of this logic (linked in the CL description), recall what the
> > > original
> > > rationale was? Do I understand correctly that the intent was fixing
> > > bookmarks
> > > *created* in incognito, and perhaps the *update* of mappings in incognito
> > > was
> > > not thought through?
> > >
> > > https://codereview.chromium.org/2694333002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-09-04 17:22:49]

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

[commit-bot: I haz the power, 2017-09-04 17:22:55]

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, 2017-09-04 19:34:45]

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

[commit-bot: I haz the power, 2017-09-04 19:34:46]

Dry run: This issue passed the CQ dry run.

[sky, 2017-09-05 16:53:43]

battre has the final say.

  -Scott

On Mon, Sep 4, 2017 at 10:22 AM, <mastiz@chromium.org> wrote:

> sky@: sorry for the long silence but I'd like to ping this thread and
> wrap it
> up, comments below.
>
> On 2017/04/05 07:44:33, mastiz wrote:
> > Thanks Scott.
> >
> > On 2017/04/04 20:41:55, sky wrote:
> > > I intended both. My thinking was that if you've bookmarked a page than
> it's
> > > important and you want the favicon kept up to date, even in incognito
> mode.
> > > I see the rationale you've outlined, but I think that is not the common
> > > case. Consider if you're some one that always browses in incognito
> mode,
> > > wouldn't you want the favicon kept up to date? Or more specifically,
> > > wouldn't you file a bug that the favicon never changed?
> >
> > The favicon would be updated in the UI, although not cached. This might
> even
> > lead to repeated flickering every time you visit a new page (i.e. the old
> > favicon would be grabbed first, and then the new one fetched), not a
> great
> user
> > experience.
> >
> > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=708447
> >
> > While reproducing the issue, I realized:
> > 1. Bookmarking pages in incognito doesn't store favicons, on Android
> (same
> > https://bugs.chromium.org/p/chromium/issues/detail?id=22670)
>
> Filed crbug.com/761764 to track this (orthogonal) issue.
>
> > 2. Assuming (1) is fixed, the reason why this privacy issue is
> exploitable is
> > that it's easy to bookmark a page *before* a favicon has been cached.
> >
> > I'll work on these two, I assume the privacy issue will be considered of
> lower
> > severity once it's hard to exploit.
>
> As per the actual bug addressed here, battre@ from privacy did speak in
> favor of
> this change (https://bugs.chromium.org/p/chromium/issues/detail?id=
> 708447#c3).
> Are you fine with that or who can make a final call?
>
> Thanks,
> Mikel
>
> >
> > >
> > > -Scott
> > >
> > > On Tue, Apr 4, 2017 at 11:16 AM, <mailto:mastiz@chromium.org> wrote:
> > >
> > > > On 2017/04/04 16:59:11, sky wrote:
> > > > > Please add test coverage.
> > > >
> > > > Will do.
> > > >
> > > > Meanwhile, do you think this is the right fix or, since you are the
> > > > original
> > > > author of this logic (linked in the CL description), recall what the
> > > > original
> > > > rationale was? Do I understand correctly that the intent was fixing
> > > > bookmarks
> > > > *created* in incognito, and perhaps the *update* of mappings in
> incognito
> > > > was
> > > > not thought through?
> > > >
> > > > https://codereview.chromium.org/2694333002/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google
> Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send
> an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2694333002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mastiz, 2017-09-05 17:05:16]

On 2017/09/05 16:53:43, sky wrote:
> battre has the final say.

Thanks, then please proceed with the review (or reroute as appropriate).

You did earlier request more test coverage but I don't think I added any since:
please insist if you still have the same feeling and please specify what kind of
coverage you'd expect, since this patch essentially removes a feature (even the
API for it, so unit tests are not feasible).

Cheers,
Mikel

> 
>   -Scott
> 
> On Mon, Sep 4, 2017 at 10:22 AM, <mailto:mastiz@chromium.org> wrote:
> 
> > sky@: sorry for the long silence but I'd like to ping this thread and
> > wrap it
> > up, comments below.
> >
> > On 2017/04/05 07:44:33, mastiz wrote:
> > > Thanks Scott.
> > >
> > > On 2017/04/04 20:41:55, sky wrote:
> > > > I intended both. My thinking was that if you've bookmarked a page than
> > it's
> > > > important and you want the favicon kept up to date, even in incognito
> > mode.
> > > > I see the rationale you've outlined, but I think that is not the common
> > > > case. Consider if you're some one that always browses in incognito
> > mode,
> > > > wouldn't you want the favicon kept up to date? Or more specifically,
> > > > wouldn't you file a bug that the favicon never changed?
> > >
> > > The favicon would be updated in the UI, although not cached. This might
> > even
> > > lead to repeated flickering every time you visit a new page (i.e. the old
> > > favicon would be grabbed first, and then the new one fetched), not a
> > great
> > user
> > > experience.
> > >
> > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=708447
> > >
> > > While reproducing the issue, I realized:
> > > 1. Bookmarking pages in incognito doesn't store favicons, on Android
> > (same
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=22670)
> >
> > Filed crbug.com/761764 to track this (orthogonal) issue.
> >
> > > 2. Assuming (1) is fixed, the reason why this privacy issue is
> > exploitable is
> > > that it's easy to bookmark a page *before* a favicon has been cached.
> > >
> > > I'll work on these two, I assume the privacy issue will be considered of
> > lower
> > > severity once it's hard to exploit.
> >
> > As per the actual bug addressed here, battre@ from privacy did speak in
> > favor of
> > this change (https://bugs.chromium.org/p/chromium/issues/detail?id=
> > 708447#c3).
> > Are you fine with that or who can make a final call?
> >
> > Thanks,
> > Mikel
> >
> > >
> > > >
> > > > -Scott
> > > >
> > > > On Tue, Apr 4, 2017 at 11:16 AM, <mailto:mastiz@chromium.org> wrote:
> > > >
> > > > > On 2017/04/04 16:59:11, sky wrote:
> > > > > > Please add test coverage.
> > > > >
> > > > > Will do.
> > > > >
> > > > > Meanwhile, do you think this is the right fix or, since you are the
> > > > > original
> > > > > author of this logic (linked in the CL description), recall what the
> > > > > original
> > > > > rationale was? Do I understand correctly that the intent was fixing
> > > > > bookmarks
> > > > > *created* in incognito, and perhaps the *update* of mappings in
> > incognito
> > > > > was
> > > > > not thought through?
> > > > >
> > > > > https://codereview.chromium.org/2694333002/
> > > > >
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google
> > Groups
> > > > "Chromium-reviews" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send
> > an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2694333002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-09-05 17:50:57]

It would be good if you could verify SetFavicons() is not called when incognito.
Otherwise we would never know if it regressed.

[sky, 2017-09-05 17:51:16]

On 2017/09/05 17:50:57, sky wrote:
> It would be good if you could verify SetFavicons() is not called when
incognito.
> Otherwise we would never know if it regressed.

That's the only comment I have. The rest looks good.

[mastiz, 2017-09-06 08:50:15]

On 2017/09/05 17:50:57, sky wrote:
> It would be good if you could verify SetFavicons() is not called when
incognito.
> Otherwise we would never know if it regressed.

This is already verified in DownloadUnknownFaviconInIncognito().

What happens is that, since we're removing the API for FaviconHandler to know
about the bookmarked status, both cases (bookmarked or not) are now equal.

In tests for upper layers, namely ContentFaviconDriver, we have the same issue
because we no longer have a BookmarkModel.

We could think about browser tests but I don't think it's worth. WDYT?

[sky, 2017-09-06 17:24:42]

LGTM

[mastiz, 2017-09-07 10:02:54]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently broken on
mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently
broken on mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

[mastiz, 2017-09-07 10:05:03]

pkotwicz@: I'll go ahead and land this patch after a grace period of a few days
unless I hear back from you, since I don't expect much controversy in the
implementation details. Thx!

[mastiz, 2017-09-11 09:26:42]

mastiz@chromium.org changed reviewers:
- pkotwicz@chromium.org

[mastiz, 2017-09-11 09:27:11]

On 2017/09/07 10:05:03, mastiz wrote:
> pkotwicz@: I'll go ahead and land this patch after a grace period of a few
days
> unless I hear back from you, since I don't expect much controversy in the
> implementation details. Thx!

pkotwicz@: landing patch, let me know if you have further comments and I'll
address them in follow-up CLs.

[mastiz, 2017-09-11 09:27:40]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-09-11 09:27:54]

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, 2017-09-11 09:30:20]

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

[commit-bot: I haz the power, 2017-09-11 09:30:21]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2017-09-11 15:42:11]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-09-11 15:42:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2694333002/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2017-09-11 15:42:22]

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, 2017-09-11 18:14:18]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1505144531567840,
"parent_rev": "ab6406625331e73b54e0c6a70b195efb3101cfae", "commit_rev":
"62a06efac826e30ae2683b64a6eccf628899de4c"}

[commit-bot: I haz the power, 2017-09-11 18:14:32]

Description was changed from

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently
broken on mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447
==========

to

==========
Fix leaking page visits in incognito mode via bookmarked favicons

When a page is visited, the favicons are cached into HistoryService. In
incognito, this is the case only if the page is bookmarked, a
special-casing introduced long ago in
http://codereview.chromium.org/5753007.

The exception doesn't seem necessary because bookmark creation 
explicitly saves the favicon (calls SetFavicon), although currently
broken on mobile (crbug.com/761764).

This exception seems to go against the general promise behind incognito
mode and can be exploited by users that share devices with other users.
E.g. if one user wants to know if another user visits a certain page,
it's sufficient to bookmark it and clear the local cache.

BUG=708447

Review-Url: https://chromiumcodereview.appspot.com/2694333002
Cr-Commit-Position: refs/heads/master@{#500976}
Committed:
https://chromium.googlesource.com/chromium/src/+/62a06efac826e30ae2683b64a6ec...
==========

[commit-bot: I haz the power, 2017-09-11 18:14:34]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/62a06efac826e30ae2683b64a6ec...