Dismiss permission bubble request if it's a duplicate

Dismiss permission bubble request if it's a duplicate

Currently seeing a crash when two iframes from the same origin request the
same permission. The issue arises because they have the same ID and therefore
the same entry in the hash map, which makes the request_ptr destruct. This CL
cancels the second (redundant) permission request.

BUG=433877
Committed: https://crrev.com/5d8a530b942dd6786cbe7a484b8b6a9ce9fe2499
Cr-Commit-Position: refs/heads/master@{#313673}

[felt, 2014-12-12 05:40:20]

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

[felt, 2014-12-12 05:42:43]

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

[felt, 2014-12-12 05:42:44]

Markus, Groby,

PTAL. Also I have a question for you. This fixes the crash but there is one
problem: the second iframe's callback won't be triggered when the user interacts
with the bubble. Is there a more "correct" way to merge the two requests besides
just not pursuing the second one?

[markusheintz_, 2014-12-12 09:53:32]

On 2014/12/12 05:42:44, felt wrote:
> Markus, Groby,
> 
> PTAL. Also I have a question for you. This fixes the crash but there is one
> problem: the second iframe's callback won't be triggered when the user
interacts
> with the bubble. Is there a more "correct" way to merge the two requests
besides
> just not pursuing the second one?

I think Rachel can better answer this question. This is all new code and its
been a while since I reviewed parts of it.

[groby-ooo-7-16, 2014-12-12 18:54:09]

groby@chromium.org changed reviewers:
+ miguelg@chromium.org

[groby-ooo-7-16, 2014-12-12 18:54:09]

If you want callbacks executed, here's what I'd do:

First, get rid of pending_bubbles_. It's only used to keep track of bubbles and
delete them when the request is finished. (miguelg: is there a subtlety I'm
missing?)

Instead, change PermissionRequestImpl so that it doesn't take a delete_callback
anymore. Just call "delete this" in RequestFinished. 

If instead  you want to keep callback semantics for the requests' deletion,
you'll need a way to pass the callback after the request is created. 

(The *real* fix is to have the PermissionBubbleManager actually own requests,
and simply deleting them after calling RequestFinished. But that's probably a
bit beyond the scope of this CL :)

[felt, 2014-12-12 19:12:50]

On 2014/12/12 18:54:09, groby wrote:
> If you want callbacks executed, here's what I'd do:
> 
> First, get rid of pending_bubbles_. It's only used to keep track of bubbles
and
> delete them when the request is finished. (miguelg: is there a subtlety I'm
> missing?)
> 
> Instead, change PermissionRequestImpl so that it doesn't take a
delete_callback
> anymore. Just call "delete this" in RequestFinished. 
> 
> If instead  you want to keep callback semantics for the requests' deletion,
> you'll need a way to pass the callback after the request is created. 
> 
> (The *real* fix is to have the PermissionBubbleManager actually own requests,
> and simply deleting them after calling RequestFinished. But that's probably a
> bit beyond the scope of this CL :)

Hmm. OK, proposing the following:

* Go with this CL to fix the crash.
* Write another CL to make the PermissionBubbleManager own requests, which
sounds like a good idea in general aside from this issue.

WDYT?

[groby-ooo-7-16, 2014-12-12 19:25:39]

You might be biting off more than you think with changing the Manager :)

Some places don't implement their own request objects, but just implement the
interface. (I.e. MediaStreamDevicesController)

Which means the callbacks would be disabled for a long time - sure you don't
want to go with the "delete this" approach outlined above?

(If you don't, the code as is is LGTM, but you should file a bug to track the
missing callback issue)

[chromium-reviews, 2014-12-15 09:49:36]

The way I understand it this should not happen, two iframes from the same
domain should have a different bridge_id and as such have different
PermissionRequestID(s) (that is precisely why bridge_id was introduced).

I suspect that whatever permission this is failing for (notifications it
seems) is not setting the bridge_id to a unique id. Does it really only
happen on mac?

> First, get rid of pending_bubbles_. It's only used to keep track of
bubbles and
> delete them when the request is finished. (miguelg: is there a subtlety
I'm
> missing?)

This is also used in case the permission request is cancelled midway (for
example if the page is navigated away while the infobar/bubble is showing)
so that we can clean up. The same approach is used for infobars (see
https://cs.corp.google.com/#clankium/src/chrome/browser/content_settings/perm...)
so something but be different in the bubble case if we are not crashing
there.





On Fri, Dec 12, 2014 at 7:25 PM, <groby@chromium.org> wrote:
>
> You might be biting off more than you think with changing the Manager :)
>
> Some places don't implement their own request objects, but just implement
> the
> interface. (I.e. MediaStreamDevicesController)
>
> Which means the callbacks would be disabled for a long time - sure you
> don't
> want to go with the "delete this" approach outlined above?
>
> (If you don't, the code as is is LGTM, but you should file a bug to track
> the
> missing callback issue)
>
> https://codereview.chromium.org/799783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-12-15 09:55:42]

+Mounir and Peter who have been refactoring notfications as well.

On Mon, Dec 15, 2014 at 9:49 AM, Miguel Garcia <miguelg@google.com> wrote:
>
> The way I understand it this should not happen, two iframes from the same
> domain should have a different bridge_id and as such have different
> PermissionRequestID(s) (that is precisely why bridge_id was introduced).
>
> I suspect that whatever permission this is failing for (notifications it
> seems) is not setting the bridge_id to a unique id. Does it really only
> happen on mac?
>
> > First, get rid of pending_bubbles_. It's only used to keep track of
> bubbles and
> > delete them when the request is finished. (miguelg: is there a subtlety
> I'm
> > missing?)
>
> This is also used in case the permission request is cancelled midway (for
> example if the page is navigated away while the infobar/bubble is showing)
> so that we can clean up. The same approach is used for infobars (see
>
https://cs.corp.google.com/#clankium/src/chrome/browser/content_settings/perm...)
> so something but be different in the bubble case if we are not crashing
> there.
>
>
>
>
>
> On Fri, Dec 12, 2014 at 7:25 PM, <groby@chromium.org> wrote:
>>
>> You might be biting off more than you think with changing the Manager :)
>>
>> Some places don't implement their own request objects, but just implement
>> the
>> interface. (I.e. MediaStreamDevicesController)
>>
>> Which means the callbacks would be disabled for a long time - sure you
>> don't
>> want to go with the "delete this" approach outlined above?
>>
>> (If you don't, the code as is is LGTM, but you should file a bug to track
>> the
>> missing callback issue)
>>
>> https://codereview.chromium.org/799783002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2014-12-15 19:33:12]

On 2014/12/15 09:49:36, chromium-reviews wrote:
> > First, get rid of pending_bubbles_. It's only used to keep track of
> bubbles and
> > delete them when the request is finished. (miguelg: is there a subtlety
> I'm
> > missing?)
> 
> This is also used in case the permission request is cancelled midway (for
> example if the page is navigated away while the infobar/bubble is showing)
> so that we can clean up. 

Sorry, question was imprecise. I meant to ask if it's used for anything else but
resource cleanup. Any "request is done" path, including abandoning a request,
routes through RequestFinished, so we can just "delete this" in that place.

[Miguel Garcia, 2014-12-16 10:09:02]

I see, in that case, yes this is correct.

On Mon, Dec 15, 2014 at 7:33 PM, <groby@chromium.org> wrote:
>
> On 2014/12/15 09:49:36, chromium-reviews wrote:
>
>> > First, get rid of pending_bubbles_. It's only used to keep track of
>> bubbles and
>> > delete them when the request is finished. (miguelg: is there a subtlety
>> I'm
>> > missing?)
>>
>
>  This is also used in case the permission request is cancelled midway (for
>> example if the page is navigated away while the infobar/bubble is showing)
>> so that we can clean up.
>>
>
> Sorry, question was imprecise. I meant to ask if it's used for anything
> else but
> resource cleanup. Any "request is done" path, including abandoning a
> request,
> routes through RequestFinished, so we can just "delete this" in that place.
>
>
> https://codereview.chromium.org/799783002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[felt, 2015-01-27 21:53:52]

markusheintz_@, ready for your review.

I've modified the CL to have a DCHECK as well as the return statement (so that
we'll be alerted to future
problems like this, but it will be handled for non-debug builds).

I agree with Miguel, the underlying problem seems to be that they should have
different bridge_ids.
I will comment on the bug to ask Peter and Mounir to look at it.

On 2014/12/16 10:09:02, Miguel Garcia wrote:
> I see, in that case, yes this is correct.
> 
> On Mon, Dec 15, 2014 at 7:33 PM, <mailto:groby@chromium.org> wrote:
> >
> > On 2014/12/15 09:49:36, chromium-reviews wrote:
> >
> >> > First, get rid of pending_bubbles_. It's only used to keep track of
> >> bubbles and
> >> > delete them when the request is finished. (miguelg: is there a subtlety
> >> I'm
> >> > missing?)
> >>
> >
> >  This is also used in case the permission request is cancelled midway (for
> >> example if the page is navigated away while the infobar/bubble is showing)
> >> so that we can clean up.
> >>
> >
> > Sorry, question was imprecise. I meant to ask if it's used for anything
> > else but
> > resource cleanup. Any "request is done" path, including abandoning a
> > request,
> > routes through RequestFinished, so we can just "delete this" in that place.
> >
> >
> > https://codereview.chromium.org/799783002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[markusheintz_, 2015-01-28 21:06:13]

LGTM

[groby-ooo-7-16, 2015-01-28 22:47:00]

I hate being that person, but...

https://codereview.chromium.org/799783002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/799783002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:123:
DCHECK(pending_bubbles_.get(id.ToString()) != NULL);
Please don't do that - style guide explicitly calls for either DCHECK or error
handling.

[felt, 2015-01-29 06:06:06]

A few notes of mine.

* groby@ suggested getting rid of pending_bubbles_ and killing the callback.
However then you still need to figure out how to get CancelPermissionRequest to
kill the right request, which requires having a list of pending requests
somewhere. So we will still need a list, although it doesn't have to be a
hashmap.

* Infobars don't have this problem because the pending infobar requests are
stored in a vector, not a hashmap. New ones are pushed on, and whenever one is
cancelled, it iterates through the whole list to remove the right one. This
approach can't directly work for us because PermissionBubbleRequests don't
contain their own IDs, although we could make a vector of
PermissionBubbleRequestImps instead and have that keep track of their own IDs.

* It seems like the underlying problem is that bridge_id is always being set to
the same thing, even though the PermissionRequestID comment claims that
bridge_id should always be unique.

https://codereview.chromium.org/799783002/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.cc (right):

https://codereview.chromium.org/799783002/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.cc:123:
DCHECK(pending_bubbles_.get(id.ToString()) != NULL);
On 2015/01/28 22:47:00, groby wrote:
> Please don't do that - style guide explicitly calls for either DCHECK or error
> handling. 
> 
> 
> 

Done. Picked error handling.

[felt, 2015-01-29 06:06:32]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2015-01-29 06:07:12]

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

[commit-bot: I haz the power, 2015-01-29 06:57:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-29 07:00:09]

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

[mlamouri (slow - plz ping), 2015-01-30 14:37:28]

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