Fixes Permission Bubbles never responding to duplicate requests

Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
Committed: https://crrev.com/7fa91f74858e1a47646814919966b5c5cbf05e92
Cr-Commit-Position: refs/heads/master@{#372435}

[johnme, 2016-01-20 19:29:04]

johnme@chromium.org changed reviewers:
+ felt@chromium.org

[johnme, 2016-01-20 19:29:04]

Hi Adrienne, can I get your initial feedback on this? This improves the handling
of duplicate permission requests (aligning the behavior with infobars), though
it does make the code a little more complex.

I haven't yet updated PermissionBubbleManagerTest; I'll do that next if you're
happy with the general approach.

https://codereview.chromium.org/1610753002/diff/1/chrome/browser/ui/website_s...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:90: for
(PermissionBubbleRequest* request : queued_frame_requests_)
n.b. queued_frame_requests_ was missing here before. Adding it is orthogonal to
this patch, but seemed useful.

https://codereview.chromium.org/1610753002/diff/1/chrome/browser/ui/website_s...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:164: for
(requests_iter = queued_frame_requests_.begin();
n.b. queued_frame_requests_ was missing here before. Adding it is orthogonal to
this patch, but seemed useful.

[felt, 2016-01-23 15:19:56]

Description was changed from

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
==========

to

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
==========

[felt, 2016-01-23 15:20:02]

+cc hcarmona

[felt, 2016-01-23 16:51:43]

looks good at a high level, i don't think the extra complexity is too bad since
you've kept the code fairly organized and the number of requests to loop over
should always be quite small.

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:205:
PermissionBubbleRequest* existing_request = GetExistingRequest(request);
If RequestFinishedIncludingDuplicates removes the duplicates do you need to do
it here too?

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:422: bool
MessageTextIsEqual(PermissionBubbleRequest* a,
nit: IsMessageTextEqual

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:493:
duplicate_requests_.erase(request);
Should you remove the duplicates in the other XIncludingDuplicates as well?

[felt, 2016-01-23 16:57:28]

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:427:
a->GetRequestingHostname() == b->GetRequestingHostname()) {
This equality check is bugging me a little bit. I know you didn't introduce it,
but before we start using it for more things it would be good to make sure it's
correct.

My concern is that each permission request implementation defines the requesting
hostname in their own way. So it might be equivalent to the security origin, or
it might not be.

If it is equivalent to the precise security origin, then I suppose comparing the
two GURLs does what we want.

If it is different... for example, if some portion of the URL has been removed
for the sake of display... then could that lead to an inaccurate comparison?

Perhaps the root cause of the issue is that the code around the hostnames here
should be more clear about whether it's about a hostname for display, or a
security origin for meaningful security boundaries. (And whether we care about
the distinction. Maybe it's reasonable for the hostname for display to map to
multiple different origins.)

Have you given this any thought?

[johnme, 2016-01-26 15:57:55]

Addressed review comments - thanks :)

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:205:
PermissionBubbleRequest* existing_request = GetExistingRequest(request);
On 2016/01/23 16:51:43, felt wrote:
> If RequestFinishedIncludingDuplicates removes the duplicates do you need to do
> it here too?

Yes - normally duplicate requests only finish when the request they were duped
against finishes, but this is the special case where we are asked to cancel a
duplicate request before the request it was duped against finished.

I've improved the comment, since it's nonobvious why I had to use
GetExistingRequest here. It now says:

// Since |request| wasn't found in queued_requests_, queued_frame_requests_ or
// requests_ it must have been marked as a duplicate. We can't search
// duplicate_requests_ by value, so instead use GetExistingRequest to find the
// key (request it was duped against), and iterate through duplicates of that.

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:422: bool
MessageTextIsEqual(PermissionBubbleRequest* a,
On 2016/01/23 16:51:43, felt wrote:
> nit: IsMessageTextEqual

Done.

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:427:
a->GetRequestingHostname() == b->GetRequestingHostname()) {
On 2016/01/23 16:57:28, felt wrote:
> This equality check is bugging me a little bit. I know you didn't introduce
it,
> but before we start using it for more things it would be good to make sure
it's
> correct.
> 
> My concern is that each permission request implementation defines the
requesting
> hostname in their own way. So it might be equivalent to the security origin,
or
> it might not be.
> 
> If it is equivalent to the precise security origin, then I suppose comparing
the
> two GURLs does what we want.
> 
> If it is different... for example, if some portion of the URL has been removed
> for the sake of display... then could that lead to an inaccurate comparison?
> 
> Perhaps the root cause of the issue is that the code around the hostnames here
> should be more clear about whether it's about a hostname for display, or a
> security origin for meaningful security boundaries. (And whether we care about
> the distinction. Maybe it's reasonable for the hostname for display to map to
> multiple different origins.)
> 
> Have you given this any thought?

Hmm, interesting. I've put together https://codereview.chromium.org/1637913002
which renames GetRequestingHostname to GetOrigin, and rebased this onto that
patch.

Is this better? It still leaves us doing `a->GetOrigin() == b->GetOrigin()`
here. I wasn't sure if that was better or worse than doing:

url::Origin(a->GetOrigin()).IsSameOriginWith(url::Origin(b->GetOrigin()))

Possibly neither is quite right, since the former doesn't handle unique origins,
whereas the latter is perhaps overly generous in treating all file: URLs the
same (though maybe the former does that too?).

https://codereview.chromium.org/1610753002/diff/20001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:493:
duplicate_requests_.erase(request);
On 2016/01/23 16:51:43, felt wrote:
> Should you remove the duplicates in the other XIncludingDuplicates as well?

No, RequestFinished is special. It is documented as:

"It is safe for the request to be deleted at this point -- it will receive no
further message from the permission bubble system. This method will eventually
be called on every request which is not unregistered."

and calls to it are always followed by the request pointer being removed from
requests_, queued_requests_, or queued_frame_requests_.

i.e. we can think of it as PermissionBubbleManager releasing a ref on the
request; and when this happens, it makes sense for the request to similarly
release the refs on the duplicates.

[felt, 2016-01-28 06:18:14]

lgtm

https://codereview.chromium.org/1610753002/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:461:
DCHECK_EQ(request, GetExistingRequest(request))
nit: wondering why this isn't DCHECKing that GetExistingRequest returns a
nullptr. i guess because it would be in requests_. anyways it is a slightly
counterintuitive DCHECK for me.

[johnme, 2016-01-28 15:40:11]

https://codereview.chromium.org/1610753002/diff/40001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/1610753002/diff/40001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:461:
DCHECK_EQ(request, GetExistingRequest(request))
On 2016/01/28 06:18:14, felt wrote:
> nit: wondering why this isn't DCHECKing that GetExistingRequest returns a
> nullptr. i guess because it would be in requests_. anyways it is a slightly
> counterintuitive DCHECK for me.

I've changed the DCHECK comment to "Only requests in [queued_[frame_]]requests_
can have duplicates" to make this clearer.

[johnme, 2016-01-28 15:40:15]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-01-28 15:40:16]

The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org
Link to the patchset: https://codereview.chromium.org/1610753002/#ps60001
(title: "Tweak DCHECK comment")

[commit-bot: I haz the power, 2016-01-28 15:41:10]

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

[commit-bot: I haz the power, 2016-01-28 16:16:00]

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

[commit-bot: I haz the power, 2016-01-28 16:16:01]

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

[johnme, 2016-01-28 20:19:42]

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

[commit-bot: I haz the power, 2016-01-28 20:21:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610753002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610753002/80001

[commit-bot: I haz the power, 2016-01-28 21:01:45]

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

[commit-bot: I haz the power, 2016-01-28 21:01:46]

Dry run: This issue passed the CQ dry run.

[felt, 2016-01-29 16:47:32]

even more lgtm after latest patchset

[johnme, 2016-01-29 21:18:08]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-01-29 21:18:48]

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

[commit-bot: I haz the power, 2016-01-29 22:13:50]

Description was changed from

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
==========

to

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
==========

[commit-bot: I haz the power, 2016-01-29 22:13:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-29 22:14:43]

Description was changed from

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313
==========

to

==========
Fixes Permission Bubbles never responding to duplicate requests

BUG=577313

Committed: https://crrev.com/7fa91f74858e1a47646814919966b5c5cbf05e92
Cr-Commit-Position: refs/heads/master@{#372435}
==========

[commit-bot: I haz the power, 2016-01-29 22:14:44]

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