Have GeolocationServiceImpl refer to RFHI via a weak pointer

Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
Committed: https://crrev.com/33dd166ed31bc6ed3a534f50a989eb57fa3ab465
Cr-Commit-Position: refs/heads/master@{#377878}

[blundell, 2015-11-20 11:08:11]

blundell@chromium.org changed reviewers:
+ creis@chromium.org, rockot@chromium.org

[blundell, 2015-11-20 11:08:11]

creis: for OWNERS.
rockot: in case you've thought of any other way to deal with this kind of issue.

[Ken Rockot(use gerrit already), 2015-11-20 14:04:00]

rockot@chromium.org changed reviewers:
+ sammc@chromium.org

[Ken Rockot(use gerrit already), 2015-11-20 14:04:01]

This CL may be of interest to you:

https://codereview.chromium.org/1411063007

In general though another approach to this problem (which IIRC we are doing for
WebUSB-related services) is to monitor RFH lifetime as a WebContentsObserver.

[blundell, 2015-11-20 14:08:45]

On 2015/11/20 14:04:01, Ken Rockot wrote:
> This CL may be of interest to you:
> 
> https://codereview.chromium.org/1411063007
> 
> In general though another approach to this problem (which IIRC we are doing
for
> WebUSB-related services) is to monitor RFH lifetime as a WebContentsObserver.

Ah, that CL will solve this issue. I'll comment there and close this one.
Thanks!

[blundell, 2015-11-20 14:08:53]

Description was changed from

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
==========

to

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
==========

[blundell, 2016-02-18 08:19:57]

Charlie, resurrecting this CL as https://codereview.chromium.org/1411063007/ is
stalled. Thanks!

[Michael van Ouwerkerk, 2016-02-18 09:37:57]

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

[Michael van Ouwerkerk, 2016-02-18 09:37:57]

Thanks Colin!

drive-by lgtm for what it's worth

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1704: // pointer.
Nit: maybe also refer to crbug.com/556749 in this comment.

[Charlie Reis, 2016-02-18 22:51:16]

[CC'ing Nick, who has thought about these lifetime issues as well.]

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1733:
base::Unretained(this)));
Won't this be a problem for all of these Mojo services as well?

Also, what processing do all these services do after the RFH is gone and before
their pipes are closed, even if they have a weak pointer and don't call back to
the RFH?

I'm wondering if they should be listening for the RFH's deletion to clean
themselves up instead (e.g., via something like
WebContentsObserver::RenderFrameDeleted, though you have to be careful with that
one).

[ncarter (slow), 2016-02-19 00:44:35]

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

[ncarter (slow), 2016-02-19 00:44:36]

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1709:
weak_ptr_factory_.GetWeakPtr())));
The RenderFrameHostImpl owns the ServiceRegistry, so I am a bit surprised to
learn that that this callback, which we hand to the ServiceRegistry, can be
invoked after we've destroyed the ServiceRegistry.

So what's actually going on here? Are we sure this isn't a bug in the
ServiceRegistry?

[ncarter (slow), 2016-02-19 23:09:44]

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1709:
weak_ptr_factory_.GetWeakPtr())));
On 2016/02/19 00:44:36, ncarter wrote:
> The RenderFrameHostImpl owns the ServiceRegistry, so I am a bit surprised to
> learn that that this callback, which we hand to the ServiceRegistry, can be
> invoked after we've destroyed the ServiceRegistry.
> 
> So what's actually going on here? Are we sure this isn't a bug in the
> ServiceRegistry?

I looked at this a little more, and am inferring that it's not the
ServiceRegistry that is outliving the RFH, but the inner callback here, which is
retained by the GeolocationServiceImpl, which is what actually winds up
outliving the RFH.

In that case the WeakPtr here seems fine -- so lgtm in the sense that this fixes
a UaF.

I do wish there was a way to actively kill off the services that so that they
couldn't outlive their associated RenderFrameHost or WebContents, and breed this
kind of UaF possibility. Without a live RFH we really ought not to be allowing
any kind of IPCs in (see also: https://codereview.chromium.org/1534933002/ and
its associated bugs)

[ncarter (slow), 2016-02-25 20:55:13]

Let's land this? Charlie?

[Charlie Reis, 2016-02-25 21:00:57]

LGTM.

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1733:
base::Unretained(this)));
On 2016/02/18 22:51:16, Charlie Reis (slow til Mar 7) wrote:
> Won't this be a problem for all of these Mojo services as well?
> 
> Also, what processing do all these services do after the RFH is gone and
before
> their pipes are closed, even if they have a weak pointer and don't call back
to
> the RFH?
> 
> I'm wondering if they should be listening for the RFH's deletion to clean
> themselves up instead (e.g., via something like
> WebContentsObserver::RenderFrameDeleted, though you have to be careful with
that
> one).

Ok, Nick explained that these other services shouldn't have the same problem. 
Works for me, though it might be nice (as he mentions) to have a way to avoid
this type of bug in the future.

[ncarter (slow), 2016-02-25 21:05:34]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2016-02-25 21:08:37]

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

[commit-bot: I haz the power, 2016-02-25 21:49:23]

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

[commit-bot: I haz the power, 2016-02-25 21:49:25]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[blundell, 2016-02-26 12:32:50]

Apologies for the delayed response.

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1709:
weak_ptr_factory_.GetWeakPtr())));
On 2016/02/19 23:09:44, ncarter (slow-ooo until mar 7) wrote:
> On 2016/02/19 00:44:36, ncarter wrote:
> > The RenderFrameHostImpl owns the ServiceRegistry, so I am a bit surprised to
> > learn that that this callback, which we hand to the ServiceRegistry, can be
> > invoked after we've destroyed the ServiceRegistry.
> > 
> > So what's actually going on here? Are we sure this isn't a bug in the
> > ServiceRegistry?
> 
> I looked at this a little more, and am inferring that it's not the
> ServiceRegistry that is outliving the RFH, but the inner callback here, which
is
> retained by the GeolocationServiceImpl, which is what actually winds up
> outliving the RFH.
> 

Correct.

> In that case the WeakPtr here seems fine -- so lgtm in the sense that this
fixes
> a UaF.
> 
> I do wish there was a way to actively kill off the services that so that they
> couldn't outlive their associated RenderFrameHost or WebContents, and breed
this
> kind of UaF possibility. Without a live RFH we really ought not to be allowing
> any kind of IPCs in (see also: https://codereview.chromium.org/1534933002/ and
> its associated bugs)

https://codereview.chromium.org/1411063007 is related, although I don't think it
would be a complete silver bullet.

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1733:
base::Unretained(this)));
On 2016/02/25 21:00:57, Charlie Reis (slow til Mar 7) wrote:
> On 2016/02/18 22:51:16, Charlie Reis (slow til Mar 7) wrote:
> > Won't this be a problem for all of these Mojo services as well?
> > 
> > Also, what processing do all these services do after the RFH is gone and
> before
> > their pipes are closed, even if they have a weak pointer and don't call back
> to
> > the RFH?
> > 
> > I'm wondering if they should be listening for the RFH's deletion to clean
> > themselves up instead (e.g., via something like
> > WebContentsObserver::RenderFrameDeleted, though you have to be careful with
> that
> > one).
> 
> Ok, Nick explained that these other services shouldn't have the same problem. 
> Works for me, though it might be nice (as he mentions) to have a way to avoid
> this type of bug in the future.

Good catch on pointing out the other cases here that are potentially
problematic. I also double-checked that all of the other services that take in a
reference to the RFHI handle lifetime properly (in various different custom
ways, as Nick pointed out).

[blundell, 2016-02-26 12:34:12]

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1459343003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1704: // pointer.
On 2016/02/18 09:37:57, Michael van Ouwerkerk wrote:
> Nit: maybe also refer to crbug.com/556749 in this comment.

I think the comment is sufficient to explain the issue as-is, but happy to
follow up if you feel strongly.

[blundell, 2016-02-26 12:34:25]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2016-02-26 12:34:35]

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

[commit-bot: I haz the power, 2016-02-26 13:51:34]

Description was changed from

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
==========

to

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
==========

[commit-bot: I haz the power, 2016-02-26 13:51:35]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-26 13:52:41]

Description was changed from

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749
==========

to

==========
Have GeolocationServiceImpl refer to RFHI via a weak pointer

GeolocationServiceImpl's lifetime is conceptually the same as its containing
RenderFrameHostImpl, as the former is destroyed when it receives a message that
the pipe to the renderer has been closed (which occurs when the RenderFrame is
destroyed). However, there's no actual guaranteed ordering between
GeolocationServiceImpl receiving that message and the RenderFrameHostImpl being
destroyed, leading to a potential race at shutdown wherein
GeolocationServiceImpl calls back into RenderFrameHostImpl after it's been
destroyed.

This CL handles the race condition by giving GeolocationServiceImpl a WeakPtr
to RenderFrameHostImpl rather than a raw pointer.

BUG=556749

Committed: https://crrev.com/33dd166ed31bc6ed3a534f50a989eb57fa3ab465
Cr-Commit-Position: refs/heads/master@{#377878}
==========

[commit-bot: I haz the power, 2016-02-26 13:52:42]

Patchset 1 (id:??) landed as
https://crrev.com/33dd166ed31bc6ed3a534f50a989eb57fa3ab465
Cr-Commit-Position: refs/heads/master@{#377878}