Allow retrieval of media device ID salt even after ResourceContext has gone away.

Allow retrieval of media device ID salt even after ResourceContext has gone away.

TBR=willchan@chromium.org
BUG=341211
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251024

[Jói, 2014-02-11 13:22:59]

WDYT?

[tommi (sloooow) - chröme, 2014-02-11 15:09:46]

https://codereview.chromium.org/143003031/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/device_request_message_filter.cc
(right):

https://codereview.chromium.org/143003031/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/device_request_message_filter.cc:37:
CancelAllRequests();
I'm not sure this is actually what is happening.  My thinking has been that in
all cases we are on the IO thread when we get here and that OnChannelClosing has
already been called (hence the DCHECK that was here).  We could actually try
changing the DCHECK to a CHECK to make sure that this assumption holds true
always.

Is there a way to know about profile teardown on the UI thread and somehow make
sure that all requests have been cancelled before the profile goes away (or
rather, _starts_ going away since that afaik triggers deletion of resource
contexts)?

Alternatively, I think that you're suggestion of holding on to the
MediaDeviceIDSalt object would be a safer way to go...

Having said that I see that accessing the salt in that class is done on the IO
thread whereas ShutdownOnUIThread deletes the salt on the UI thread and there's
no locking, so is there a chance we could have a race there as well?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...

[Jói, 2014-02-11 16:31:29]

Hmm, good points all, am reading the code to try to figure a correct
understanding.

Re the call to PrefMember::Destroy in
MediaDeviceIDSalt::ShutdownOnUIThread, this is safe. See
base/prefs/pref_member.h, which documents that GetValue, IsManaged,
and IsUserModifiable can still be called from another thread (to which
the member was moved using PrefMember::MoveToThread, as done in the
constructor) but will no longer update from PrefService.

Cheers,
Jói


On Tue, Feb 11, 2014 at 3:09 PM,  <tommi@chromium.org> wrote:
>
>
https://codereview.chromium.org/143003031/diff/1/content/browser/renderer_hos...
> File
> content/browser/renderer_host/media/device_request_message_filter.cc
> (right):
>
>
https://codereview.chromium.org/143003031/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/media/device_request_message_filter.cc:37:
> CancelAllRequests();
> I'm not sure this is actually what is happening.  My thinking has been
> that in all cases we are on the IO thread when we get here and that
> OnChannelClosing has already been called (hence the DCHECK that was
> here).  We could actually try changing the DCHECK to a CHECK to make
> sure that this assumption holds true always.
>
> Is there a way to know about profile teardown on the UI thread and
> somehow make sure that all requests have been cancelled before the
> profile goes away (or rather, _starts_ going away since that afaik
> triggers deletion of resource contexts)?
>
> Alternatively, I think that you're suggestion of holding on to the
> MediaDeviceIDSalt object would be a safer way to go...
>
> Having said that I see that accessing the salt in that class is done on
> the IO thread whereas ShutdownOnUIThread deletes the salt on the UI
> thread and there's no locking, so is there a chance we could have a race
> there as well?
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...
>
> https://codereview.chromium.org/143003031/

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

[Jói, 2014-02-12 14:07:04]

Please take another look, this is the refcounted approach we discussed.

Cheers,
Jói

[tommi (sloooow) - chröme, 2014-02-12 14:31:54]

lgtm

[Jói, 2014-02-12 14:34:48]

+willchan for content/browser/profiles/profile_io_data*

[willchan no longer on Chromium, 2014-02-12 19:32:22]

https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:479: mutable
scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
Who owns MediaDeviceIDSalt now?

[Jói, 2014-02-12 19:48:09]

Generally speaking, the last reference will be owned by the
ResourceContext implementation, but in rare cases the last reference
will be owned by a task being done on the IO thread (see bug).

Honestly, I would like to fix this in a better way, but we need a fix
ASAP and the larger fix seems hard and will probably need some
discussion. It seems wrong if ProfileIOData (and with it,
ResourceContext) is going away (on the IO thread, but actually
destroyed from UI thread) before RenderProcessHost teardown is
finished (RPHImpl owns DeviceRequestMessageFilter, which during
teardown receives an OnChannelClosing and removes the objects that
might later use the ResourceContext).

Cheers,
Jói


On Wed, Feb 12, 2014 at 7:32 PM,  <willchan@chromium.org> wrote:
>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> File chrome/browser/profiles/profile_io_data.h (right):
>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> chrome/browser/profiles/profile_io_data.h:479: mutable
> scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
> Who owns MediaDeviceIDSalt now?
>
> https://codereview.chromium.org/143003031/

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

[willchan no longer on Chromium, 2014-02-12 19:51:55]

You're right, that seems broken and we should fix it. Two more questions:
* Why is this urgent?
* If we refcount this one, but a RenderProcessHost is still alive and
can send us IPCs that other FooMessageFilters might process, aren't we
just going to crash in some other place instead?

On Wed, Feb 12, 2014 at 11:45 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> Generally speaking, the last reference will be owned by the
> ResourceContext implementation, but in rare cases the last reference
> will be owned by a task being done on the IO thread (see bug).
>
> Honestly, I would like to fix this in a better way, but we need a fix
> ASAP and the larger fix seems hard and will probably need some
> discussion. It seems wrong if ProfileIOData (and with it,
> ResourceContext) is going away (on the IO thread, but actually
> destroyed from UI thread) before RenderProcessHost teardown is
> finished (RPHImpl owns DeviceRequestMessageFilter, which during
> teardown receives an OnChannelClosing and removes the objects that
> might later use the ResourceContext).
>
> Cheers,
> Jói
>
>
> On Wed, Feb 12, 2014 at 7:32 PM,  <willchan@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
>> File chrome/browser/profiles/profile_io_data.h (right):
>>
>>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
>> chrome/browser/profiles/profile_io_data.h:479: mutable
>> scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
>> Who owns MediaDeviceIDSalt now?
>>
>> https://codereview.chromium.org/143003031/

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

[Jói, 2014-02-12 21:16:34]

This is a fairly frequent crasher, hence urgent. A variant of this was
merged to M33 recently.

There might be other crashes due to the same brokenness, if my analysis of
the situation is correct. I don't know of specific ones though.

Tommi, anything to add?

Cheers,
Jói


On Wednesday, February 12, 2014, William Chan (陈智昌) <willchan@chromium.org>
wrote:

> You're right, that seems broken and we should fix it. Two more questions:
> * Why is this urgent?
> * If we refcount this one, but a RenderProcessHost is still alive and
> can send us IPCs that other FooMessageFilters might process, aren't we
> just going to crash in some other place instead?
>
> On Wed, Feb 12, 2014 at 11:45 AM, Jói Sigurðsson
<joi@chromium.org<javascript:;>>
> wrote:
> > Generally speaking, the last reference will be owned by the
> > ResourceContext implementation, but in rare cases the last reference
> > will be owned by a task being done on the IO thread (see bug).
> >
> > Honestly, I would like to fix this in a better way, but we need a fix
> > ASAP and the larger fix seems hard and will probably need some
> > discussion. It seems wrong if ProfileIOData (and with it,
> > ResourceContext) is going away (on the IO thread, but actually
> > destroyed from UI thread) before RenderProcessHost teardown is
> > finished (RPHImpl owns DeviceRequestMessageFilter, which during
> > teardown receives an OnChannelClosing and removes the objects that
> > might later use the ResourceContext).
> >
> > Cheers,
> > Jói
> >
> >
> > On Wed, Feb 12, 2014 at 7:32 PM,  <willchan@chromium.org <javascript:;>>
> wrote:
> >>
> >>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> >> File chrome/browser/profiles/profile_io_data.h (right):
> >>
> >>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> >> chrome/browser/profiles/profile_io_data.h:479: mutable
> >> scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
> >> Who owns MediaDeviceIDSalt now?
> >>
> >> https://codereview.chromium.org/143003031/
>

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

[willchan no longer on Chromium, 2014-02-12 21:19:13]

On Wed, Feb 12, 2014 at 1:16 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> This is a fairly frequent crasher, hence urgent. A variant of this was
> merged to M33 recently.

Got it. I am fine with this approach with comments explaining it if
you think that this will fix the problem (I'm unsure it will).

>
> There might be other crashes due to the same brokenness, if my analysis of
> the situation is correct. I don't know of specific ones though.

FWIW, I believe your analysis of the situation is correct. I worry
that we've had a regression in RenderProcessHost lifetime, and we need
to track that down. This patch might simply change the symptom. That
said, if this is something we can verify with a canary build, I'm
happy to take this changelist and re-evaluate next week or so.

>
> Tommi, anything to add?
>
> Cheers,
> Jói
>
>
> On Wednesday, February 12, 2014, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> You're right, that seems broken and we should fix it. Two more questions:
>> * Why is this urgent?
>> * If we refcount this one, but a RenderProcessHost is still alive and
>> can send us IPCs that other FooMessageFilters might process, aren't we
>> just going to crash in some other place instead?
>>
>> On Wed, Feb 12, 2014 at 11:45 AM, Jói Sigurðsson <joi@chromium.org> wrote:
>> > Generally speaking, the last reference will be owned by the
>> > ResourceContext implementation, but in rare cases the last reference
>> > will be owned by a task being done on the IO thread (see bug).
>> >
>> > Honestly, I would like to fix this in a better way, but we need a fix
>> > ASAP and the larger fix seems hard and will probably need some
>> > discussion. It seems wrong if ProfileIOData (and with it,
>> > ResourceContext) is going away (on the IO thread, but actually
>> > destroyed from UI thread) before RenderProcessHost teardown is
>> > finished (RPHImpl owns DeviceRequestMessageFilter, which during
>> > teardown receives an OnChannelClosing and removes the objects that
>> > might later use the ResourceContext).
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > On Wed, Feb 12, 2014 at 7:32 PM,  <willchan@chromium.org> wrote:
>> >>
>> >>
>> >>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
>> >> File chrome/browser/profiles/profile_io_data.h (right):
>> >>
>> >>
>> >>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
>> >> chrome/browser/profiles/profile_io_data.h:479: mutable
>> >> scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
>> >> Who owns MediaDeviceIDSalt now?
>> >>
>> >> https://codereview.chromium.org/143003031/

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

[tommi (sloooow) - chröme, 2014-02-12 21:32:52]

Not much to add really I think you've explained it well.

I think this approach makes sense given what we know and avoids major
larger architectural changes which is a good thing right now.  I guess we
could have considered a WeakPtr approach to make lifetime more explicit but
as is this lgtm.


On Wed, Feb 12, 2014 at 10:19 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Feb 12, 2014 at 1:16 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > This is a fairly frequent crasher, hence urgent. A variant of this was
> > merged to M33 recently.
>
> Got it. I am fine with this approach with comments explaining it if
> you think that this will fix the problem (I'm unsure it will).
>
> >
> > There might be other crashes due to the same brokenness, if my analysis
> of
> > the situation is correct. I don't know of specific ones though.
>
> FWIW, I believe your analysis of the situation is correct. I worry
> that we've had a regression in RenderProcessHost lifetime, and we need
> to track that down. This patch might simply change the symptom. That
> said, if this is something we can verify with a canary build, I'm
> happy to take this changelist and re-evaluate next week or so.
>
> >
> > Tommi, anything to add?
> >
> > Cheers,
> > Jói
> >
> >
> > On Wednesday, February 12, 2014, William Chan (陈智昌) <
> willchan@chromium.org>
> > wrote:
> >>
> >> You're right, that seems broken and we should fix it. Two more
> questions:
> >> * Why is this urgent?
> >> * If we refcount this one, but a RenderProcessHost is still alive and
> >> can send us IPCs that other FooMessageFilters might process, aren't we
> >> just going to crash in some other place instead?
> >>
> >> On Wed, Feb 12, 2014 at 11:45 AM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >> > Generally speaking, the last reference will be owned by the
> >> > ResourceContext implementation, but in rare cases the last reference
> >> > will be owned by a task being done on the IO thread (see bug).
> >> >
> >> > Honestly, I would like to fix this in a better way, but we need a fix
> >> > ASAP and the larger fix seems hard and will probably need some
> >> > discussion. It seems wrong if ProfileIOData (and with it,
> >> > ResourceContext) is going away (on the IO thread, but actually
> >> > destroyed from UI thread) before RenderProcessHost teardown is
> >> > finished (RPHImpl owns DeviceRequestMessageFilter, which during
> >> > teardown receives an OnChannelClosing and removes the objects that
> >> > might later use the ResourceContext).
> >> >
> >> > Cheers,
> >> > Jói
> >> >
> >> >
> >> > On Wed, Feb 12, 2014 at 7:32 PM,  <willchan@chromium.org> wrote:
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> >> >> File chrome/browser/profiles/profile_io_data.h (right):
> >> >>
> >> >>
> >> >>
>
https://codereview.chromium.org/143003031/diff/210002/chrome/browser/profiles...
> >> >> chrome/browser/profiles/profile_io_data.h:479: mutable
> >> >> scoped_refptr<MediaDeviceIDSalt> media_device_id_salt_;
> >> >> Who owns MediaDeviceIDSalt now?
> >> >>
> >> >> https://codereview.chromium.org/143003031/
>

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

[Jói, 2014-02-13 10:39:09]

Thanks guys. I've added a TODO comment to resource_context.h about the
possibility of a proper fix and am pulling the CQ trigger with TBR=willchan.

Cheers,
Jói

[Jói, 2014-02-13 10:39:16]

The CQ bit was checked by joi@chromium.org

[commit-bot: I haz the power, 2014-02-13 10:39:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/143003031/390001

[commit-bot: I haz the power, 2014-02-13 15:34:28]

Change committed as 251024

[willchan no longer on Chromium, 2014-02-13 16:50:07]

lgtm