Issue 1411063007: Add mojo::StrongBindingSet and use it in GeolocationServiceContext.

Sam McNally

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

5 years, 1 month ago (2015-11-10 00:19:50 UTC) #1

Anand Mistry (off Chromium)

https://codereview.chromium.org/1411063007/diff/1/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1411063007/diff/1/content/browser/android/content_view_core_impl.cc#newcode371 content/browser/android/content_view_core_impl.cc:371: auto geolocation_service_context = Why the difference? https://codereview.chromium.org/1411063007/diff/1/content/browser/geolocation/geolocation_service_context.cc File content/browser/geolocation/geolocation_service_context.cc ...

5 years, 1 month ago (2015-11-10 04:53:39 UTC) #3

Sam McNally

https://codereview.chromium.org/1411063007/diff/1/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1411063007/diff/1/content/browser/android/content_view_core_impl.cc#newcode371 content/browser/android/content_view_core_impl.cc:371: auto geolocation_service_context = On 2015/11/10 04:53:39, Anand Mistry wrote: ...

5 years, 1 month ago (2015-11-11 07:04:03 UTC) #5

Michael van Ouwerkerk

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

5 years, 1 month ago (2015-11-11 11:20:55 UTC) #6

Michael van Ouwerkerk

Thanks Sam. For future patches, please break out changes to Mojo from things like turning ...

5 years, 1 month ago (2015-11-11 11:20:56 UTC) #7

Anand Mistry (off Chromium)

LGTM with a few minor nits. https://codereview.chromium.org/1411063007/diff/70001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1411063007/diff/70001/content/browser/frame_host/render_frame_host_impl.cc#newcode59 content/browser/frame_host/render_frame_host_impl.cc:59: #include "content/public/browser/permission_manager.h" Could ...

5 years, 1 month ago (2015-11-12 07:11:33 UTC) #8

Sam McNally

https://codereview.chromium.org/1411063007/diff/60001/content/browser/android/content_view_core_impl.cc File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/1411063007/diff/60001/content/browser/android/content_view_core_impl.cc#newcode371 content/browser/android/content_view_core_impl.cc:371: auto geolocation_service_context = On 2015/11/11 11:20:55, Michael van Ouwerkerk ...

5 years, 1 month ago (2015-11-13 00:50:00 UTC) #9

danakj

Hey, I looked at your use of std::forwarding and it looks right. Thanks for the ...

5 years, 1 month ago (2015-11-17 21:42:17 UTC) #12

danakj

Description was changed from ========== Add mojo::ServiceSet and use it in GeolocationServiceContext. Currently, mojo service ...

5 years, 1 month ago (2015-11-17 21:42:32 UTC) #13

Sam McNally

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

5 years, 1 month ago (2015-11-18 03:17:17 UTC) #15

Sam McNally

+jam for content/ outside content/browser/geolocation and mojo/ https://codereview.chromium.org/1411063007/diff/90001/mojo/common/service_set.h File mojo/common/service_set.h (right): https://codereview.chromium.org/1411063007/diff/90001/mojo/common/service_set.h#newcode46 mojo/common/service_set.h:46: Impl* CreateService(InterfaceRequest<Interface> ...

5 years, 1 month ago (2015-11-18 03:17:18 UTC) #16

danakj

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

5 years, 1 month ago (2015-11-18 19:59:11 UTC) #17

danakj

https://codereview.chromium.org/1411063007/diff/90001/mojo/common/service_set.h File mojo/common/service_set.h (right): https://codereview.chromium.org/1411063007/diff/90001/mojo/common/service_set.h#newcode59 mojo/common/service_set.h:59: void clear() { holders_.clear(); } On 2015/11/18 03:17:18, Sam ...

5 years, 1 month ago (2015-11-18 19:59:12 UTC) #18

danakj

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h File mojo/common/service_set.h (right): https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h#newcode46 mojo/common/service_set.h:46: Impl* EmplaceService(InterfaceRequest<Interface> request, Args&&... args) { I should point ...

5 years, 1 month ago (2015-11-18 19:59:51 UTC) #19

jam

+yzshen I have a few questions: -why do we need to decouple GeolocationServiceContext from WebContentsImpl? ...

5 years, 1 month ago (2015-11-19 02:45:38 UTC) #21

Sam McNally

On 2015/11/19 02:45:38, jam wrote: > +yzshen > > I have a few questions: > ...

5 years, 1 month ago (2015-11-19 04:53:26 UTC) #22

On 2015/11/19 02:45:38, jam wrote:
> +yzshen
> 
> I have a few questions:
> -why do we need to decouple GeolocationServiceContext from WebContentsImpl?

For consistency with services outside content plus a bit of "why does
WebContentsImpl even care about geolocation?"

> -even with the description, I'm still not sure why we need this (a tracking
bug
> with more info would have been helpful). Regarding lifetime management, my
naive
> question is why aren't services just shutting down when their message pipe is
> torn down automatically through StrongBinding?

The point of this is to try to make the render-exposed mojo services in content
more consistent. All of them have a context class, often for different reasons
and/or with different responsibilities, but every context class owns its
services. Doing this in terms of StrongBinding and notifying the context when a
service is destroyed (with a delete all on context destruction) also works, but
it seemed simpler to abstract away most of service lifetime management. In the
specific case of geolocation, the context is to expose a per-WebContents API to
other parts of the browser (pause for android webview, position override for
devtools).

The general problem with relying on StrongBinding alone is browser shutdown. I
experimented with converting text-to-speech to use mojo, using StrongBinding,
with the service exposed using the RenderProcessHost ServiceRegistry. This was
fine for most renderer processes, but if the renderer process contained a
ServiceWorker, the message pipe remained open until the message loop stops, and
the service has to contend with an unexpected environment: (depending on which
voice was used) the TTS service wants to send an event to the extension
providing the TTS implementation to stop speaking when it's destroyed, but when
the message loop is stopped, the BrowserContext-keyed services have been deleted
already and it hits a use-after-free.

yzshen1

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h File mojo/common/service_set.h (right): https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h#newcode34 mojo/common/service_set.h:34: class ServiceSet { With a quick look, this looks ...

5 years, 1 month ago (2015-11-19 17:34:24 UTC) #23

Ken Rockot(use gerrit already)

To me a StrongBindingSet would be a thing that maintains multiple bindings to a single ...

5 years, 1 month ago (2015-11-19 17:43:23 UTC) #24

jam

On 2015/11/19 04:53:26, Sam McNally wrote: > On 2015/11/19 02:45:38, jam wrote: > > +yzshen ...

5 years, 1 month ago (2015-11-19 17:43:57 UTC) #25

On 2015/11/19 04:53:26, Sam McNally wrote:
> On 2015/11/19 02:45:38, jam wrote:
> > +yzshen
> > 
> > I have a few questions:
> > -why do we need to decouple GeolocationServiceContext from WebContentsImpl?
> 
> For consistency with services outside content plus a bit of "why does
> WebContentsImpl even care about geolocation?"
> 
> > -even with the description, I'm still not sure why we need this (a tracking
> bug
> > with more info would have been helpful). Regarding lifetime management, my
> naive
> > question is why aren't services just shutting down when their message pipe
is
> > torn down automatically through StrongBinding?
> 
> The point of this is to try to make the render-exposed mojo services in
content
> more consistent. All of them have a context class, often for different reasons
> and/or with different responsibilities, but every context class owns its
> services.

You're talking about the browser side context class, like Geolocation's right?
This was a very old pattern in chrome, and we stopped it (although some newer
features may have copied it). If we convert more code in content, I think a
context class is the minority by far.

> Doing this in terms of StrongBinding and notifying the context when a
> service is destroyed (with a delete all on context destruction) also works,
but
> it seemed simpler to abstract away most of service lifetime management.

Can you expand on why it's simpler to not use StrongBinding? StrongBinding
should be a well understood concept for anyone using mojo. Adding another
concept here with ServiceSet adds yet another thing that devs have to be
familiar with.

> In the
> specific case of geolocation, the context is to expose a per-WebContents API
to
> other parts of the browser (pause for android webview, position override for
> devtools).
> 
> The general problem with relying on StrongBinding alone is browser shutdown. I
> experimented with converting text-to-speech to use mojo, using StrongBinding,
> with the service exposed using the RenderProcessHost ServiceRegistry. This was
> fine for most renderer processes, but if the renderer process contained a
> ServiceWorker, the message pipe remained open until the message loop stops,
and
> the service has to contend with an unexpected environment: (depending on which
> voice was used) the TTS service wants to send an event to the extension
> providing the TTS implementation to stop speaking when it's destroyed, but
when
> the message loop is stopped, the BrowserContext-keyed services have been
deleted
> already and it hits a use-after-free.

I'm not sure I follow all the above; I'm missing some context/stacktraces.

It seems that part of the above is because code is outliving the profile (i.e.
BrowserContext).Why is a renderer process outliving the profile? Or is because
RenderProcessHostImpl isn't outliving the profile, but by the time the
strongbinding notices the connection error that is later?

yzshen1

On 2015/11/19 17:43:23, Ken Rockot wrote: > To me a StrongBindingSet would be a thing ...

5 years, 1 month ago (2015-11-19 17:46:51 UTC) #26

yzshen1

On 2015/11/19 17:43:57, jam wrote: > On 2015/11/19 04:53:26, Sam McNally wrote: > > On ...

5 years, 1 month ago (2015-11-19 17:48:47 UTC) #27

On 2015/11/19 17:43:57, jam wrote:
> On 2015/11/19 04:53:26, Sam McNally wrote:
> > On 2015/11/19 02:45:38, jam wrote:
> > > +yzshen
> > > 
> > > I have a few questions:
> > > -why do we need to decouple GeolocationServiceContext from
WebContentsImpl?
> > 
> > For consistency with services outside content plus a bit of "why does
> > WebContentsImpl even care about geolocation?"
> > 
> > > -even with the description, I'm still not sure why we need this (a
tracking
> > bug
> > > with more info would have been helpful). Regarding lifetime management, my
> > naive
> > > question is why aren't services just shutting down when their message pipe
> is
> > > torn down automatically through StrongBinding?
> > 
> > The point of this is to try to make the render-exposed mojo services in
> content
> > more consistent. All of them have a context class, often for different
reasons
> > and/or with different responsibilities, but every context class owns its
> > services.
> 
> You're talking about the browser side context class, like Geolocation's right?
> This was a very old pattern in chrome, and we stopped it (although some newer
> features may have copied it). If we convert more code in content, I think a
> context class is the minority by far.
> 
> > Doing this in terms of StrongBinding and notifying the context when a
> > service is destroyed (with a delete all on context destruction) also works,
> but
> > it seemed simpler to abstract away most of service lifetime management.
> 
> Can you expand on why it's simpler to not use StrongBinding? StrongBinding
> should be a well understood concept for anyone using mojo. Adding another
> concept here with ServiceSet adds yet another thing that devs have to be
> familiar with.

+1!!

> 
> > In the
> > specific case of geolocation, the context is to expose a per-WebContents API
> to
> > other parts of the browser (pause for android webview, position override for
> > devtools).
> > 
> > The general problem with relying on StrongBinding alone is browser shutdown.
I
> > experimented with converting text-to-speech to use mojo, using
StrongBinding,
> > with the service exposed using the RenderProcessHost ServiceRegistry. This
was
> > fine for most renderer processes, but if the renderer process contained a
> > ServiceWorker, the message pipe remained open until the message loop stops,
> and
> > the service has to contend with an unexpected environment: (depending on
which
> > voice was used) the TTS service wants to send an event to the extension
> > providing the TTS implementation to stop speaking when it's destroyed, but
> when
> > the message loop is stopped, the BrowserContext-keyed services have been
> deleted
> > already and it hits a use-after-free.
> 
> I'm not sure I follow all the above; I'm missing some context/stacktraces.
> 
> It seems that part of the above is because code is outliving the profile (i.e.
> BrowserContext).Why is a renderer process outliving the profile? Or is because
> RenderProcessHostImpl isn't outliving the profile, but by the time the
> strongbinding notices the connection error that is later?

Ken Rockot(use gerrit already)

On Thu, Nov 19, 2015 at 9:46 AM, <yzshen@chromium.org> wrote: > On 2015/11/19 17:43:23, Ken ...

5 years, 1 month ago (2015-11-19 18:04:49 UTC) #28

On Thu, Nov 19, 2015 at 9:46 AM, <yzshen@chromium.org> wrote:

> On 2015/11/19 17:43:23, Ken Rockot wrote:
>
>> To me a StrongBindingSet would be a thing that maintains multiple bindings
>> to a single impl, and deletes that impl once the set is empty (which is an
>> idea that's been thrown around separately).
>>
>
> ServiceSet a set of impls which each have their own binding, so it's not
>> really the same thing.
>>
>
> On Thu, Nov 19, 2015 at 9:34 AM, <mailto:yzshen@chromium.org> wrote:
>>
>
> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
>
>> > File mojo/common/service_set.h (right):
>> >
>> >
>> >
>>
>
>
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
>
>> > mojo/common/service_set.h:34: class ServiceSet {
>> > With a quick look, this looks similar to WeakBindingSet, except that
>> > this class also owns the impl objects.
>> >
>> > Can this be made a StrongBindingSet? Or one step further, unify
>> > {Weak|Strong}BindingSet to share implementation, using different Traits
>> > to deal with impl ownership?
>> >
>> > https://codereview.chromium.org/1411063007/
>> >
>>
>
> --
>> 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.
>>
>
> Ah, thanks for the explanation!
>
> However, I would argue that a WeakBindingSet doesn't require all the
> bindings
> refer to the same impl object. Why do we want it differently in
> StrongBindingSet?
>
>
Oh... you're right :)  Not sure why I thought that was the case.


>
> https://codereview.chromium.org/1411063007/
>

-- 
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.

blundell

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

5 years, 1 month ago (2015-11-20 14:08:04 UTC) #29

blundell

Hi Sam, Can you add BUG=556749? This CL will solve that bug (I got pointed ...

5 years, 1 month ago (2015-11-20 14:08:04 UTC) #30

mlamouri (slow - plz ping)

Description was changed from ========== Add mojo::ServiceSet and use it in GeolocationServiceContext. Currently, mojo service ...

5 years, 1 month ago (2015-11-20 20:22:38 UTC) #31

mlamouri (slow - plz ping)

Description was changed from ========== Add mojo::ServiceSet and use it in GeolocationServiceContext. Currently, mojo service ...

5 years, 1 month ago (2015-11-20 20:22:39 UTC) #32

Sam McNally

On 2015/11/19 17:43:57, jam wrote: > On 2015/11/19 04:53:26, Sam McNally wrote: > > On ...

5 years, 1 month ago (2015-11-23 01:12:53 UTC) #33

On 2015/11/19 17:43:57, jam wrote:
> On 2015/11/19 04:53:26, Sam McNally wrote:
> > On 2015/11/19 02:45:38, jam wrote:
> > > +yzshen
> > > 
> > > I have a few questions:
> > > -why do we need to decouple GeolocationServiceContext from
WebContentsImpl?
> > 
> > For consistency with services outside content plus a bit of "why does
> > WebContentsImpl even care about geolocation?"
> > 
> > > -even with the description, I'm still not sure why we need this (a
tracking
> > bug
> > > with more info would have been helpful). Regarding lifetime management, my
> > naive
> > > question is why aren't services just shutting down when their message pipe
> is
> > > torn down automatically through StrongBinding?
> > 
> > The point of this is to try to make the render-exposed mojo services in
> content
> > more consistent. All of them have a context class, often for different
reasons
> > and/or with different responsibilities, but every context class owns its
> > services.
> 
> You're talking about the browser side context class, like Geolocation's right?
> This was a very old pattern in chrome, and we stopped it (although some newer
> features may have copied it). If we convert more code in content, I think a
> context class is the minority by far.

What do you propose as the alternative to a context class if a
per-WebContents/per-RenderFrame interface is required where each service
instance may also require its own state? I wasn't a fan of these context classes
either, but I couldn't find a way to get rid of them. Most of the mojo services
in content require some of the functionality that a context allows so it seems
useful to have an easy to use/copy-paste pattern to follow. Using a context +
ServiceSet also provides service lifetime guarantees; StrongBinding provides
edges on which to race during shutdown of all kinds.

Standalone services using a StrongBinding are also useful, but they don't work
well for everything and trying to force everything to follow that pattern is
just going to lead to more mess.
> 
> > Doing this in terms of StrongBinding and notifying the context when a
> > service is destroyed (with a delete all on context destruction) also works,
> but
> > it seemed simpler to abstract away most of service lifetime management.
> 
> Can you expand on why it's simpler to not use StrongBinding? StrongBinding
> should be a well understood concept for anyone using mojo. Adding another
> concept here with ServiceSet adds yet another thing that devs have to be
> familiar with.

It's fewer moving parts to worry about. StrongBinding + a context means the
context has to track the active services and the service has the notify the
context on deletion so it can clean up properly. This is just busywork for every
service with a context.
> 
> > In the
> > specific case of geolocation, the context is to expose a per-WebContents API
> to
> > other parts of the browser (pause for android webview, position override for
> > devtools).
> > 
> > The general problem with relying on StrongBinding alone is browser shutdown.
I
> > experimented with converting text-to-speech to use mojo, using
StrongBinding,
> > with the service exposed using the RenderProcessHost ServiceRegistry. This
was
> > fine for most renderer processes, but if the renderer process contained a
> > ServiceWorker, the message pipe remained open until the message loop stops,
> and
> > the service has to contend with an unexpected environment: (depending on
which
> > voice was used) the TTS service wants to send an event to the extension
> > providing the TTS implementation to stop speaking when it's destroyed, but
> when
> > the message loop is stopped, the BrowserContext-keyed services have been
> deleted
> > already and it hits a use-after-free.
> 
> I'm not sure I follow all the above; I'm missing some context/stacktraces.
> 
> It seems that part of the above is because code is outliving the profile (i.e.
> BrowserContext).Why is a renderer process outliving the profile? Or is because
> RenderProcessHostImpl isn't outliving the profile, but by the time the
> strongbinding notices the connection error that is later?

I'm not sure which it was. I decided to wait and see if mojo-in-blink would make
it simpler.

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
File mojo/common/service_set.h (right):

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
mojo/common/service_set.h:34: class ServiceSet {
On 2015/11/19 17:34:24, yzshen1 wrote:
> With a quick look, this looks similar to WeakBindingSet, except that this
class
> also owns the impl objects.
> 
> Can this be made a StrongBindingSet? Or one step further, unify
> {Weak|Strong}BindingSet to share implementation, using different Traits to
deal
> with impl ownership?

It's similar, but I think the ownership here is different enough to not try to
merge implementations, at least for now.

blundell

On 2015/11/19 02:45:38, jam wrote: > +yzshen > > I have a few questions: > ...

5 years, 1 month ago (2015-11-23 08:59:18 UTC) #34

yzshen1

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h File mojo/common/service_set.h (right): https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h#newcode34 mojo/common/service_set.h:34: class ServiceSet { On 2015/11/23 01:12:53, Sam McNally wrote: ...

5 years, 1 month ago (2015-11-23 16:12:10 UTC) #35

jam

On 2015/11/23 01:12:53, Sam McNally wrote: > On 2015/11/19 17:43:57, jam wrote: > > On ...

5 years, 1 month ago (2015-11-24 00:14:52 UTC) #36

On 2015/11/23 01:12:53, Sam McNally wrote:
> On 2015/11/19 17:43:57, jam wrote:
> > On 2015/11/19 04:53:26, Sam McNally wrote:
> > > On 2015/11/19 02:45:38, jam wrote:
> > > > +yzshen
> > > > 
> > > > I have a few questions:
> > > > -why do we need to decouple GeolocationServiceContext from
> WebContentsImpl?
> > > 
> > > For consistency with services outside content plus a bit of "why does
> > > WebContentsImpl even care about geolocation?"
> > > 
> > > > -even with the description, I'm still not sure why we need this (a
> tracking
> > > bug
> > > > with more info would have been helpful). Regarding lifetime management,
my
> > > naive
> > > > question is why aren't services just shutting down when their message
pipe
> > is
> > > > torn down automatically through StrongBinding?
> > > 
> > > The point of this is to try to make the render-exposed mojo services in
> > content
> > > more consistent. All of them have a context class, often for different
> reasons
> > > and/or with different responsibilities, but every context class owns its
> > > services.
> > 
> > You're talking about the browser side context class, like Geolocation's
right?
> > This was a very old pattern in chrome, and we stopped it (although some
newer
> > features may have copied it). If we convert more code in content, I think a
> > context class is the minority by far.
> 
> What do you propose as the alternative to a context class if a
> per-WebContents/per-RenderFrame interface is required where each service
> instance may also require its own state? I wasn't a fan of these context
classes
> either, but I couldn't find a way to get rid of them. Most of the mojo
services
> in content require some of the functionality that a context allows so it seems
> useful to have an easy to use/copy-paste pattern to follow. Using a context +
> ServiceSet also provides service lifetime guarantees; StrongBinding provides
> edges on which to race during shutdown of all kinds.
> 
> Standalone services using a StrongBinding are also useful, but they don't work
> well for everything and trying to force everything to follow that pattern is
> just going to lead to more mess.

I don't follow everything above. Can you schedule a vc to discuss this some
more?

> > 
> > > Doing this in terms of StrongBinding and notifying the context when a
> > > service is destroyed (with a delete all on context destruction) also
works,
> > but
> > > it seemed simpler to abstract away most of service lifetime management.
> > 
> > Can you expand on why it's simpler to not use StrongBinding? StrongBinding
> > should be a well understood concept for anyone using mojo. Adding another
> > concept here with ServiceSet adds yet another thing that devs have to be
> > familiar with.
> 
> It's fewer moving parts to worry about. StrongBinding + a context means the
> context has to track the active services and the service has the notify the
> context on deletion so it can clean up properly. This is just busywork for
every
> service with a context.
> > 
> > > In the
> > > specific case of geolocation, the context is to expose a per-WebContents
API
> > to
> > > other parts of the browser (pause for android webview, position override
for
> > > devtools).
> > > 
> > > The general problem with relying on StrongBinding alone is browser
shutdown.
> I
> > > experimented with converting text-to-speech to use mojo, using
> StrongBinding,
> > > with the service exposed using the RenderProcessHost ServiceRegistry. This
> was
> > > fine for most renderer processes, but if the renderer process contained a
> > > ServiceWorker, the message pipe remained open until the message loop
stops,
> > and
> > > the service has to contend with an unexpected environment: (depending on
> which
> > > voice was used) the TTS service wants to send an event to the extension
> > > providing the TTS implementation to stop speaking when it's destroyed, but
> > when
> > > the message loop is stopped, the BrowserContext-keyed services have been
> > deleted
> > > already and it hits a use-after-free.
> > 
> > I'm not sure I follow all the above; I'm missing some context/stacktraces.
> > 
> > It seems that part of the above is because code is outliving the profile
(i.e.
> > BrowserContext).Why is a renderer process outliving the profile? Or is
because
> > RenderProcessHostImpl isn't outliving the profile, but by the time the
> > strongbinding notices the connection error that is later?
> 
> I'm not sure which it was. I decided to wait and see if mojo-in-blink would
make
> it simpler.
> 
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
> File mojo/common/service_set.h (right):
> 
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
> mojo/common/service_set.h:34: class ServiceSet {
> On 2015/11/19 17:34:24, yzshen1 wrote:
> > With a quick look, this looks similar to WeakBindingSet, except that this
> class
> > also owns the impl objects.
> > 
> > Can this be made a StrongBindingSet? Or one step further, unify
> > {Weak|Strong}BindingSet to share implementation, using different Traits to
> deal
> > with impl ownership?
> 
> It's similar, but I think the ownership here is different enough to not try to
> merge implementations, at least for now.

Sam McNally

On 2015/11/24 00:14:52, jam wrote: > On 2015/11/23 01:12:53, Sam McNally wrote: > > On ...

5 years ago (2015-12-01 00:12:54 UTC) #39

On 2015/11/24 00:14:52, jam wrote:
> On 2015/11/23 01:12:53, Sam McNally wrote:
> > On 2015/11/19 17:43:57, jam wrote:
> > > On 2015/11/19 04:53:26, Sam McNally wrote:
> > > > On 2015/11/19 02:45:38, jam wrote:
> > > > > +yzshen
> > > > > 
> > > > > I have a few questions:
> > > > > -why do we need to decouple GeolocationServiceContext from
> > WebContentsImpl?
> > > > 
> > > > For consistency with services outside content plus a bit of "why does
> > > > WebContentsImpl even care about geolocation?"
> > > > 
> > > > > -even with the description, I'm still not sure why we need this (a
> > tracking
> > > > bug
> > > > > with more info would have been helpful). Regarding lifetime
management,
> my
> > > > naive
> > > > > question is why aren't services just shutting down when their message
> pipe
> > > is
> > > > > torn down automatically through StrongBinding?
> > > > 
> > > > The point of this is to try to make the render-exposed mojo services in
> > > content
> > > > more consistent. All of them have a context class, often for different
> > reasons
> > > > and/or with different responsibilities, but every context class owns its
> > > > services.
> > > 
> > > You're talking about the browser side context class, like Geolocation's
> right?
> > > This was a very old pattern in chrome, and we stopped it (although some
> newer
> > > features may have copied it). If we convert more code in content, I think
a
> > > context class is the minority by far.
> > 
> > What do you propose as the alternative to a context class if a
> > per-WebContents/per-RenderFrame interface is required where each service
> > instance may also require its own state? I wasn't a fan of these context
> classes
> > either, but I couldn't find a way to get rid of them. Most of the mojo
> services
> > in content require some of the functionality that a context allows so it
seems
> > useful to have an easy to use/copy-paste pattern to follow. Using a context
+
> > ServiceSet also provides service lifetime guarantees; StrongBinding provides
> > edges on which to race during shutdown of all kinds.
> > 
> > Standalone services using a StrongBinding are also useful, but they don't
work
> > well for everything and trying to force everything to follow that pattern is
> > just going to lead to more mess.
> 
> I don't follow everything above. Can you schedule a vc to discuss this some
> more?
> 
I've put together a doc on what this is trying to achieve:
https://docs.google.com/a/chromium.org/document/d/1DGux2YJb1dW5HPwxsFeRGfj8_9....

I checked what was happening with TTS-using-mojo and shutdown: the
RenderProcessHosts are shut down before the profile is destroyed, but the
mojo::Binding isn't notified before the message loop stops.

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
File mojo/common/service_set.h (right):

https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
mojo/common/service_set.h:34: class ServiceSet {
On 2015/11/23 16:12:09, yzshen1 wrote:
> On 2015/11/23 01:12:53, Sam McNally wrote:
> > On 2015/11/19 17:34:24, yzshen1 wrote:
> > > With a quick look, this looks similar to WeakBindingSet, except that this
> > class
> > > also owns the impl objects.
> > > 
> > > Can this be made a StrongBindingSet? Or one step further, unify
> > > {Weak|Strong}BindingSet to share implementation, using different Traits to
> > deal
> > > with impl ownership?
> > 
> > It's similar, but I think the ownership here is different enough to not try
to
> > merge implementations, at least for now.
> 
> What do you think about naming it StrongBindingSet and making the interface
> consistent with WeakBindingSet?

This class isn't publicly concerned with bindings. The public interface is a set
of services whose lifetime is managed by the set.

yzshen1

On 2015/12/01 00:12:54, Sam McNally wrote: > On 2015/11/24 00:14:52, jam wrote: > > On ...

5 years ago (2015-12-01 18:05:23 UTC) #40

On 2015/12/01 00:12:54, Sam McNally wrote:
> On 2015/11/24 00:14:52, jam wrote:
> > On 2015/11/23 01:12:53, Sam McNally wrote:
> > > On 2015/11/19 17:43:57, jam wrote:
> > > > On 2015/11/19 04:53:26, Sam McNally wrote:
> > > > > On 2015/11/19 02:45:38, jam wrote:
> > > > > > +yzshen
> > > > > > 
> > > > > > I have a few questions:
> > > > > > -why do we need to decouple GeolocationServiceContext from
> > > WebContentsImpl?
> > > > > 
> > > > > For consistency with services outside content plus a bit of "why does
> > > > > WebContentsImpl even care about geolocation?"
> > > > > 
> > > > > > -even with the description, I'm still not sure why we need this (a
> > > tracking
> > > > > bug
> > > > > > with more info would have been helpful). Regarding lifetime
> management,
> > my
> > > > > naive
> > > > > > question is why aren't services just shutting down when their
message
> > pipe
> > > > is
> > > > > > torn down automatically through StrongBinding?
> > > > > 
> > > > > The point of this is to try to make the render-exposed mojo services
in
> > > > content
> > > > > more consistent. All of them have a context class, often for different
> > > reasons
> > > > > and/or with different responsibilities, but every context class owns
its
> > > > > services.
> > > > 
> > > > You're talking about the browser side context class, like Geolocation's
> > right?
> > > > This was a very old pattern in chrome, and we stopped it (although some
> > newer
> > > > features may have copied it). If we convert more code in content, I
think
> a
> > > > context class is the minority by far.
> > > 
> > > What do you propose as the alternative to a context class if a
> > > per-WebContents/per-RenderFrame interface is required where each service
> > > instance may also require its own state? I wasn't a fan of these context
> > classes
> > > either, but I couldn't find a way to get rid of them. Most of the mojo
> > services
> > > in content require some of the functionality that a context allows so it
> seems
> > > useful to have an easy to use/copy-paste pattern to follow. Using a
context
> +
> > > ServiceSet also provides service lifetime guarantees; StrongBinding
provides
> > > edges on which to race during shutdown of all kinds.
> > > 
> > > Standalone services using a StrongBinding are also useful, but they don't
> work
> > > well for everything and trying to force everything to follow that pattern
is
> > > just going to lead to more mess.
> > 
> > I don't follow everything above. Can you schedule a vc to discuss this some
> > more?
> > 
> I've put together a doc on what this is trying to achieve:
>
https://docs.google.com/a/chromium.org/document/d/1DGux2YJb1dW5HPwxsFeRGfj8_9....
> 
> I checked what was happening with TTS-using-mojo and shutdown: the
> RenderProcessHosts are shut down before the profile is destroyed, but the
> mojo::Binding isn't notified before the message loop stops.
> 
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
> File mojo/common/service_set.h (right):
> 
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
> mojo/common/service_set.h:34: class ServiceSet {
> On 2015/11/23 16:12:09, yzshen1 wrote:
> > On 2015/11/23 01:12:53, Sam McNally wrote:
> > > On 2015/11/19 17:34:24, yzshen1 wrote:
> > > > With a quick look, this looks similar to WeakBindingSet, except that
this
> > > class
> > > > also owns the impl objects.
> > > > 
> > > > Can this be made a StrongBindingSet? Or one step further, unify
> > > > {Weak|Strong}BindingSet to share implementation, using different Traits
to
> > > deal
> > > > with impl ownership?
> > > 
> > > It's similar, but I think the ownership here is different enough to not
try
> to
> > > merge implementations, at least for now.
> > 
> > What do you think about naming it StrongBindingSet and making the interface
> > consistent with WeakBindingSet?
> 
> This class isn't publicly concerned with bindings. The public interface is a
set
> of services whose lifetime is managed by the set.

I still feel that the difference is not significant enough to justify
introducing a new concept in mojo/common. StrongBindingSet manages the lifetime
of the impl objects, too. And the learning cost is smaller because we already
have StrongBinding, WeakBinding and WeakBindingSet. It seems natural to
introduce StrongBindingSet instead of ServiceSet. Besides, the name ServiceSet
is a little too broad.

Please let me know if I have missed or misunderstood anything.

Sam McNally

On 2015/12/01 18:05:23, yzshen1 wrote: > https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h > > File mojo/common/service_set.h (right): > > > ...

5 years ago (2015-12-02 05:30:46 UTC) #41

On 2015/12/01 18:05:23, yzshen1 wrote:
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
> > File mojo/common/service_set.h (right):
> > 
> >
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
> > mojo/common/service_set.h:34: class ServiceSet {
> > On 2015/11/23 16:12:09, yzshen1 wrote:
> > > On 2015/11/23 01:12:53, Sam McNally wrote:
> > > > On 2015/11/19 17:34:24, yzshen1 wrote:
> > > > > With a quick look, this looks similar to WeakBindingSet, except that
> this
> > > > class
> > > > > also owns the impl objects.
> > > > > 
> > > > > Can this be made a StrongBindingSet? Or one step further, unify
> > > > > {Weak|Strong}BindingSet to share implementation, using different
Traits
> to
> > > > deal
> > > > > with impl ownership?
> > > > 
> > > > It's similar, but I think the ownership here is different enough to not
> try
> > to
> > > > merge implementations, at least for now.
> > > 
> > > What do you think about naming it StrongBindingSet and making the
interface
> > > consistent with WeakBindingSet?
> > 
> > This class isn't publicly concerned with bindings. The public interface is a
> set
> > of services whose lifetime is managed by the set.
> 
> I still feel that the difference is not significant enough to justify
> introducing a new concept in mojo/common. StrongBindingSet manages the
lifetime
> of the impl objects, too. And the learning cost is smaller because we already
> have StrongBinding, WeakBinding and WeakBindingSet. It seems natural to
> introduce StrongBindingSet instead of ServiceSet. Besides, the name ServiceSet
> is a little too broad.
> 
How would a StrongBindingSet providing similar functionality differ from this?
The StrongBindingSet in the mojo repo lacks functionality like iteration,
deletion and connection error notifications.

Trying to make this look like WeakBindingSet will produce a worse interface.
They have different purposes so they have different interfaces.

yzshen1

On 2015/12/02 05:30:46, Sam McNally wrote: > On 2015/12/01 18:05:23, yzshen1 wrote: > > > ...

5 years ago (2015-12-02 06:04:13 UTC) #42

On 2015/12/02 05:30:46, Sam McNally wrote:
> On 2015/12/01 18:05:23, yzshen1 wrote:
> >
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
> > > File mojo/common/service_set.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
> > > mojo/common/service_set.h:34: class ServiceSet {
> > > On 2015/11/23 16:12:09, yzshen1 wrote:
> > > > On 2015/11/23 01:12:53, Sam McNally wrote:
> > > > > On 2015/11/19 17:34:24, yzshen1 wrote:
> > > > > > With a quick look, this looks similar to WeakBindingSet, except that
> > this
> > > > > class
> > > > > > also owns the impl objects.
> > > > > > 
> > > > > > Can this be made a StrongBindingSet? Or one step further, unify
> > > > > > {Weak|Strong}BindingSet to share implementation, using different
> Traits
> > to
> > > > > deal
> > > > > > with impl ownership?
> > > > > 
> > > > > It's similar, but I think the ownership here is different enough to
not
> > try
> > > to
> > > > > merge implementations, at least for now.
> > > > 
> > > > What do you think about naming it StrongBindingSet and making the
> interface
> > > > consistent with WeakBindingSet?
> > > 
> > > This class isn't publicly concerned with bindings. The public interface is
a
> > set
> > > of services whose lifetime is managed by the set.
> > 
> > I still feel that the difference is not significant enough to justify
> > introducing a new concept in mojo/common. StrongBindingSet manages the
> lifetime
> > of the impl objects, too. And the learning cost is smaller because we
already
> > have StrongBinding, WeakBinding and WeakBindingSet. It seems natural to
> > introduce StrongBindingSet instead of ServiceSet. Besides, the name
ServiceSet
> > is a little too broad.
> > 
> How would a StrongBindingSet providing similar functionality differ from this?
> The StrongBindingSet in the mojo repo lacks functionality like iteration,
> deletion and connection error notifications.

I wasn't saying that we should copy it from the mojo repo.
We certainly can have a richer interface with the functionality you proposed.


> 
> Trying to make this look like WeakBindingSet will produce a worse interface.
> They have different purposes so they have different interfaces.

I think my suggestion is more about naming. The exact interface design is
something we can discuss separately (and which I have less strong an opinion).

The reason why I personally prefer the name StrongBindingSet is that when I
looked at the code, I told myself "ah, this actually is a binding set which also
owns the impl objects". Because it lives next to WeakBindingSet, I think it is
nice to have consistent names.

Maybe you could tell me why you think the name StrongBindingSet doesn't
precisely reflect what the class does? I may have missed something.

Sam McNally

Description was changed from ========== Add mojo::ServiceSet and use it in GeolocationServiceContext. Currently, mojo service ...

5 years ago (2015-12-02 23:45:38 UTC) #43

Sam McNally

Description was changed from ========== Add mojo::StrongBindingSet and use it in GeolocationServiceContext. Currently, mojo service ...

5 years ago (2015-12-02 23:45:56 UTC) #44

Sam McNally

On 2015/12/02 06:04:13, yzshen1 wrote: > On 2015/12/02 05:30:46, Sam McNally wrote: > > On ...

5 years ago (2015-12-02 23:52:17 UTC) #45

On 2015/12/02 06:04:13, yzshen1 wrote:
> On 2015/12/02 05:30:46, Sam McNally wrote:
> > On 2015/12/01 18:05:23, yzshen1 wrote:
> > >
> >
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_set.h
> > > > File mojo/common/service_set.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1411063007/diff/150001/mojo/common/service_se...
> > > > mojo/common/service_set.h:34: class ServiceSet {
> > > > On 2015/11/23 16:12:09, yzshen1 wrote:
> > > > > On 2015/11/23 01:12:53, Sam McNally wrote:
> > > > > > On 2015/11/19 17:34:24, yzshen1 wrote:
> > > > > > > With a quick look, this looks similar to WeakBindingSet, except
that
> > > this
> > > > > > class
> > > > > > > also owns the impl objects.
> > > > > > > 
> > > > > > > Can this be made a StrongBindingSet? Or one step further, unify
> > > > > > > {Weak|Strong}BindingSet to share implementation, using different
> > Traits
> > > to
> > > > > > deal
> > > > > > > with impl ownership?
> > > > > > 
> > > > > > It's similar, but I think the ownership here is different enough to
> not
> > > try
> > > > to
> > > > > > merge implementations, at least for now.
> > > > > 
> > > > > What do you think about naming it StrongBindingSet and making the
> > interface
> > > > > consistent with WeakBindingSet?
> > > > 
> > > > This class isn't publicly concerned with bindings. The public interface
is
> a
> > > set
> > > > of services whose lifetime is managed by the set.
> > > 
> > > I still feel that the difference is not significant enough to justify
> > > introducing a new concept in mojo/common. StrongBindingSet manages the
> > lifetime
> > > of the impl objects, too. And the learning cost is smaller because we
> already
> > > have StrongBinding, WeakBinding and WeakBindingSet. It seems natural to
> > > introduce StrongBindingSet instead of ServiceSet. Besides, the name
> ServiceSet
> > > is a little too broad.
> > > 
> > How would a StrongBindingSet providing similar functionality differ from
this?
> > The StrongBindingSet in the mojo repo lacks functionality like iteration,
> > deletion and connection error notifications.
> 
> I wasn't saying that we should copy it from the mojo repo.
> We certainly can have a richer interface with the functionality you proposed.
> 
> 
> > 
> > Trying to make this look like WeakBindingSet will produce a worse interface.
> > They have different purposes so they have different interfaces.
> 
> I think my suggestion is more about naming. The exact interface design is
> something we can discuss separately (and which I have less strong an opinion).
> 
> The reason why I personally prefer the name StrongBindingSet is that when I
> looked at the code, I told myself "ah, this actually is a binding set which
also
> owns the impl objects". Because it lives next to WeakBindingSet, I think it is
> nice to have consistent names.
> 
> Maybe you could tell me why you think the name StrongBindingSet doesn't
> precisely reflect what the class does? I may have missed something.

That makes sense. Changed to StrongBindingSet.

Ben Goodger (Google)

This seems pretty convoluted and moving further away from conventional Mojo bindings. In general, we ...

5 years ago (2015-12-03 05:13:28 UTC) #46

Sam McNally

On 2015/12/03 05:13:28, Ben Goodger (Google) wrote: > This seems pretty convoluted and moving further ...

5 years ago (2015-12-03 06:29:11 UTC) #47

On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
> This seems pretty convoluted and moving further away from conventional Mojo
> bindings. In general, we should use templates very sparingly. They seem clever
> once you've invested the time in figuring out what they're doing, but they are
> hard for casual readers to understand and especially challenging to debug. You
> should not view template use as a substitute for more straightforward code.

Is this where you draw the line for complexity? Have you looked at most of the
mojo-related code? It's full of layers of inscrutable complexity. Why have
WeakBindingSet? Or StrongBinding for that matter? TypeConverters are right out,
obviously.

> 
> Yuzhu explained what you were trying to do to me, let me play it back and
offer
> what I'd consider a "conventional" solution:
> 
> - You have some object (GeolocationServiceContext) which is owned by
> WebContentsImpl. This object acts as a factory for GeolocationServiceImpls,
> which are bound to some mojom interface, with corresponding uses in the
> renderer.
> - You want to tie the lifetime of the GeolocationServiceImpls to both the pipe
> and the context so that they are destroyed when the pipe closes or the context
> is destroyed.
> 
> The conventional way of solving this would be to have GeolocationServiceImpl
> have a StrongBinding to the interface pipe, and to have
> GeolocationServiceContext keep a set of GeolocationServiceImpl* it created.
When
> the context is destroyed, it enumerates the set and deletes the impls, which
> cause the binding to fall out of scope and close the pipe. Alternatively, when
> the pipe closes, the GeolocationServiceImpl dtor should notify the context
which
> would remove it from the set. Simple.

Simple, but buggy: you're notifying GeolocationServiceContext when it's
responsible for the deletion. Maybe it's harmless, or maybe it'll remove that
GeolocationServiceImpl* from GeolocationServiceContext's set, invalidating the
iterator its destructor was using.

The point of this is to have an abstraction that can be reused instead of each
service implementation copy-pasting or reinventing this pattern each time.

chromium-reviews

On Wed, Dec 2, 2015 at 10:29 PM, <sammc@chromium.org> wrote: > On 2015/12/03 05:13:28, Ben ...

5 years ago (2015-12-03 06:41:19 UTC) #48

On Wed, Dec 2, 2015 at 10:29 PM, <sammc@chromium.org> wrote:

> On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
>
>> This seems pretty convoluted and moving further away from conventional
>> Mojo
>> bindings. In general, we should use templates very sparingly. They seem
>> clever
>> once you've invested the time in figuring out what they're doing, but
>> they are
>> hard for casual readers to understand and especially challenging to
>> debug. You
>> should not view template use as a substitute for more straightforward
>> code.
>>
>
> Is this where you draw the line for complexity? Have you looked at most of
> the
> mojo-related code? It's full of layers of inscrutable complexity. Why have
> WeakBindingSet? Or StrongBinding for that matter? TypeConverters are right
> out,
> obviously.


If Mojo is inscrutably complex, we should fix it and not use it as a
justification to add more :-) Ken's recent straw man for burying mojom
types is a great example of a simplifying improvement. WeakBindingSet and
StrongBinding seem pretty simple from a user perspective but maybe I'm just
used to using them, so simplifying suggestions welcome. There are of course
ownership issues. There are always ownership issues.


> The conventional way of solving this would be to have
>> GeolocationServiceImpl
>>
> have a StrongBinding to the interface pipe, and to have
>> GeolocationServiceContext keep a set of GeolocationServiceImpl* it
>> created.
>>
> When
>
>> the context is destroyed, it enumerates the set and deletes the impls,
>> which
>> cause the binding to fall out of scope and close the pipe. Alternatively,
>> when
>> the pipe closes, the GeolocationServiceImpl dtor should notify the context
>>
> which
>
>> would remove it from the set. Simple.
>>
>
> Simple, but buggy: you're notifying GeolocationServiceContext when it's
> responsible for the deletion. Maybe it's harmless, or maybe it'll remove
> that
> GeolocationServiceImpl* from GeolocationServiceContext's set, invalidating
> the
> iterator its destructor was using.
>

Presumably you can have some checking to avoid a cycle.

-Ben

-- 
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.

Sam McNally

On 2015/12/03 06:41:19, chromium-reviews wrote: > On Wed, Dec 2, 2015 at 10:29 PM, <mailto:sammc@chromium.org> ...

5 years ago (2015-12-04 00:57:21 UTC) #49

On 2015/12/03 06:41:19, chromium-reviews wrote:
> On Wed, Dec 2, 2015 at 10:29 PM, <mailto:sammc@chromium.org> wrote:
> 
> > On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
> >
> >> This seems pretty convoluted and moving further away from conventional
> >> Mojo
> >> bindings. In general, we should use templates very sparingly. They seem
> >> clever
> >> once you've invested the time in figuring out what they're doing, but
> >> they are
> >> hard for casual readers to understand and especially challenging to
> >> debug. You
> >> should not view template use as a substitute for more straightforward
> >> code.
> >>
> >
> > Is this where you draw the line for complexity? Have you looked at most of
> > the
> > mojo-related code? It's full of layers of inscrutable complexity. Why have
> > WeakBindingSet? Or StrongBinding for that matter? TypeConverters are right
> > out,
> > obviously.
> 
> 
> If Mojo is inscrutably complex, we should fix it and not use it as a
> justification to add more :-) Ken's recent straw man for burying mojom
> types is a great example of a simplifying improvement. WeakBindingSet and
> StrongBinding seem pretty simple from a user perspective but maybe I'm just
> used to using them, so simplifying suggestions welcome. There are of course
> ownership issues. There are always ownership issues.
> 
If we're just looking at public interfaces (the high-level ones at least), then
sure, it's quite straightforward, but I would argue the same is true of
StrongBindingSet. It's a std::set<Impl> that automatically deletes things for
you when they're no longer needed and some of the method names are different; in
fact, in the common case (a class implementing a single mojo interface) we could
deduce the mojo interface type from the Impl type and make it just
StrongBindingSet<Impl> instead of StrongBindingSet<Interface, Impl>.

The theme of these things seems to be hiding away unnecessary complexity from
the user. Two-way ownership from both a message pipe and a C++ owner seems like
the sort of unnecessary complexity that can be hidden away.
> 
> > The conventional way of solving this would be to have
> >> GeolocationServiceImpl
> >>
> > have a StrongBinding to the interface pipe, and to have
> >> GeolocationServiceContext keep a set of GeolocationServiceImpl* it
> >> created.
> >>
> > When
> >
> >> the context is destroyed, it enumerates the set and deletes the impls,
> >> which
> >> cause the binding to fall out of scope and close the pipe. Alternatively,
> >> when
> >> the pipe closes, the GeolocationServiceImpl dtor should notify the context
> >>
> > which
> >
> >> would remove it from the set. Simple.
> >>
> >
> > Simple, but buggy: you're notifying GeolocationServiceContext when it's
> > responsible for the deletion. Maybe it's harmless, or maybe it'll remove
> > that
> > GeolocationServiceImpl* from GeolocationServiceContext's set, invalidating
> > the
> > iterator its destructor was using.
> >
> 
> Presumably you can have some checking to avoid a cycle.

Indeed, once you know what the problem is it's an easy fix, but it's another
thing to think about or to get wrong and waste time debugging when writing
something that follows this pattern.
> 
> -Ben
> 
> -- 
> 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.

jam

On 2015/12/04 00:57:21, Sam McNally wrote: > On 2015/12/03 06:41:19, chromium-reviews wrote: > > On ...

5 years ago (2015-12-04 18:17:50 UTC) #50

On 2015/12/04 00:57:21, Sam McNally wrote:
> On 2015/12/03 06:41:19, chromium-reviews wrote:
> > On Wed, Dec 2, 2015 at 10:29 PM, <mailto:sammc@chromium.org> wrote:
> > 
> > > On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
> > >
> > >> This seems pretty convoluted and moving further away from conventional
> > >> Mojo
> > >> bindings. In general, we should use templates very sparingly. They seem
> > >> clever
> > >> once you've invested the time in figuring out what they're doing, but
> > >> they are
> > >> hard for casual readers to understand and especially challenging to
> > >> debug. You
> > >> should not view template use as a substitute for more straightforward
> > >> code.
> > >>
> > >
> > > Is this where you draw the line for complexity? Have you looked at most of
> > > the
> > > mojo-related code? It's full of layers of inscrutable complexity. Why have
> > > WeakBindingSet? Or StrongBinding for that matter? TypeConverters are right
> > > out,
> > > obviously.
> > 
> > 
> > If Mojo is inscrutably complex, we should fix it and not use it as a
> > justification to add more :-) Ken's recent straw man for burying mojom
> > types is a great example of a simplifying improvement. WeakBindingSet and
> > StrongBinding seem pretty simple from a user perspective but maybe I'm just
> > used to using them, so simplifying suggestions welcome. There are of course
> > ownership issues. There are always ownership issues.
> > 
> If we're just looking at public interfaces (the high-level ones at least),
then
> sure, it's quite straightforward, but I would argue the same is true of
> StrongBindingSet. It's a std::set<Impl> that automatically deletes things for
> you when they're no longer needed and some of the method names are different;
in
> fact, in the common case (a class implementing a single mojo interface) we
could
> deduce the mojo interface type from the Impl type and make it just
> StrongBindingSet<Impl> instead of StrongBindingSet<Interface, Impl>.

Why don't we wait to see how often this comes up in practice as we convert
features. If this turns out to be a common pattern, then we can spend more time
thinking about the most straightforward generic solution. But if it's just a few
places, then manual bookkeeping is enough.

Sam McNally

On 2015/12/04 18:17:50, jam wrote: > On 2015/12/04 00:57:21, Sam McNally wrote: > > On ...

5 years ago (2015-12-07 00:42:04 UTC) #51

On 2015/12/04 18:17:50, jam wrote:
> On 2015/12/04 00:57:21, Sam McNally wrote:
> > On 2015/12/03 06:41:19, chromium-reviews wrote:
> > > On Wed, Dec 2, 2015 at 10:29 PM, <mailto:sammc@chromium.org> wrote:
> > > 
> > > > On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
> > > >
> > > >> This seems pretty convoluted and moving further away from conventional
> > > >> Mojo
> > > >> bindings. In general, we should use templates very sparingly. They seem
> > > >> clever
> > > >> once you've invested the time in figuring out what they're doing, but
> > > >> they are
> > > >> hard for casual readers to understand and especially challenging to
> > > >> debug. You
> > > >> should not view template use as a substitute for more straightforward
> > > >> code.
> > > >>
> > > >
> > > > Is this where you draw the line for complexity? Have you looked at most
of
> > > > the
> > > > mojo-related code? It's full of layers of inscrutable complexity. Why
have
> > > > WeakBindingSet? Or StrongBinding for that matter? TypeConverters are
right
> > > > out,
> > > > obviously.
> > > 
> > > 
> > > If Mojo is inscrutably complex, we should fix it and not use it as a
> > > justification to add more :-) Ken's recent straw man for burying mojom
> > > types is a great example of a simplifying improvement. WeakBindingSet and
> > > StrongBinding seem pretty simple from a user perspective but maybe I'm
just
> > > used to using them, so simplifying suggestions welcome. There are of
course
> > > ownership issues. There are always ownership issues.
> > > 
> > If we're just looking at public interfaces (the high-level ones at least),
> then
> > sure, it's quite straightforward, but I would argue the same is true of
> > StrongBindingSet. It's a std::set<Impl> that automatically deletes things
for
> > you when they're no longer needed and some of the method names are
different;
> in
> > fact, in the common case (a class implementing a single mojo interface) we
> could
> > deduce the mojo interface type from the Impl type and make it just
> > StrongBindingSet<Impl> instead of StrongBindingSet<Interface, Impl>.
> 
> Why don't we wait to see how often this comes up in practice as we convert
> features. If this turns out to be a common pattern, then we can spend more
time
> thinking about the most straightforward generic solution. But if it's just a
few
> places, then manual bookkeeping is enough.

Here's a breakdown of mojo services that may be used in chrome:

WeakBindingSet (a special case of a context where the service instances
trivially forward to the context):
- VRService
- device::usb::PermissionProvider

Context:
- PermissionService
- GeolocationService
- WakeLockService (service impls receive a WeakPtr to the context rather than
being owned by the context)
- BackgroundSync
- ServicePortService
- WebUI (with correct lifetime)

StrongBinding:
- SerialService
- serial::Connection
- BatteryMonitor
- MimeHandlerService
- extensions::KeepAlive (+ observers to delete on extension unload)
- VibrationManager (no-op non-android impl)
- ServicePortDispatcher (browser -> child)
- ImageDownloader (browser -> renderer)
- BackgroundSyncClient (browser -> renderer)
- ProxyResolverFactory (browser -> utility)
- ResourceUsageReporter (browser -> utility)
- WebUI (with potentially incorrect lifetime)

StrongBinding-like:
- ProxyResolver (browser -> utility)
- device::usb::DeviceManager
- device::usb::Device

Something else:
- PresentationService
- MediaRouter
- ProxyResolverRequestClient
- ProxyResolverFactoryRequestClient

Sam McNally

On 2015/12/07 00:42:04, Sam McNally wrote: > On 2015/12/04 18:17:50, jam wrote: > > On ...

5 years ago (2015-12-08 02:17:48 UTC) #52

On 2015/12/07 00:42:04, Sam McNally wrote:
> On 2015/12/04 18:17:50, jam wrote:
> > On 2015/12/04 00:57:21, Sam McNally wrote:
> > > On 2015/12/03 06:41:19, chromium-reviews wrote:
> > > > On Wed, Dec 2, 2015 at 10:29 PM, <mailto:sammc@chromium.org> wrote:
> > > > 
> > > > > On 2015/12/03 05:13:28, Ben Goodger (Google) wrote:
> > > > >
> > > > >> This seems pretty convoluted and moving further away from
conventional
> > > > >> Mojo
> > > > >> bindings. In general, we should use templates very sparingly. They
seem
> > > > >> clever
> > > > >> once you've invested the time in figuring out what they're doing, but
> > > > >> they are
> > > > >> hard for casual readers to understand and especially challenging to
> > > > >> debug. You
> > > > >> should not view template use as a substitute for more straightforward
> > > > >> code.
> > > > >>
> > > > >
> > > > > Is this where you draw the line for complexity? Have you looked at
most
> of
> > > > > the
> > > > > mojo-related code? It's full of layers of inscrutable complexity. Why
> have
> > > > > WeakBindingSet? Or StrongBinding for that matter? TypeConverters are
> right
> > > > > out,
> > > > > obviously.
> > > > 
> > > > 
> > > > If Mojo is inscrutably complex, we should fix it and not use it as a
> > > > justification to add more :-) Ken's recent straw man for burying mojom
> > > > types is a great example of a simplifying improvement. WeakBindingSet
and
> > > > StrongBinding seem pretty simple from a user perspective but maybe I'm
> just
> > > > used to using them, so simplifying suggestions welcome. There are of
> course
> > > > ownership issues. There are always ownership issues.
> > > > 
> > > If we're just looking at public interfaces (the high-level ones at least),
> > then
> > > sure, it's quite straightforward, but I would argue the same is true of
> > > StrongBindingSet. It's a std::set<Impl> that automatically deletes things
> for
> > > you when they're no longer needed and some of the method names are
> different;
> > in
> > > fact, in the common case (a class implementing a single mojo interface) we
> > could
> > > deduce the mojo interface type from the Impl type and make it just
> > > StrongBindingSet<Impl> instead of StrongBindingSet<Interface, Impl>.
> > 
> > Why don't we wait to see how often this comes up in practice as we convert
> > features. If this turns out to be a common pattern, then we can spend more
> time
> > thinking about the most straightforward generic solution. But if it's just a
> few
> > places, then manual bookkeeping is enough.
> 
> Here's a breakdown of mojo services that may be used in chrome:
> 
> WeakBindingSet (a special case of a context where the service instances
> trivially forward to the context):
> - VRService
> - device::usb::PermissionProvider
> 
> Context:
> - PermissionService
> - GeolocationService
> - WakeLockService (service impls receive a WeakPtr to the context rather than
> being owned by the context)
> - BackgroundSync
> - ServicePortService
> - WebUI (with correct lifetime)
> 
> StrongBinding:
> - SerialService
> - serial::Connection
> - BatteryMonitor
> - MimeHandlerService
> - extensions::KeepAlive (+ observers to delete on extension unload)
> - VibrationManager (no-op non-android impl)
> - ServicePortDispatcher (browser -> child)
> - ImageDownloader (browser -> renderer)
> - BackgroundSyncClient (browser -> renderer)
> - ProxyResolverFactory (browser -> utility)
> - ResourceUsageReporter (browser -> utility)
> - WebUI (with potentially incorrect lifetime)
> 
> StrongBinding-like:
> - ProxyResolver (browser -> utility)
> - device::usb::DeviceManager
> - device::usb::Device
> 
> Something else:
> - PresentationService
> - MediaRouter
> - ProxyResolverRequestClient
> - ProxyResolverFactoryRequestClient

Just to be clear, the "Context" services are the ones that could benefit from
StrongBindingSet, and it looks like ProxyResolverRequestClient can be added to
that list too.

Sam McNally

ProxyResolverRequestClient: https://codereview.chromium.org/1516493003/ PermissionService: https://codereview.chromium.org/1513973002/ BackgroundSyncService: https://codereview.chromium.org/1507233003/ How many times does this pattern need to occur ...

5 years ago (2015-12-15 06:01:58 UTC) #53

Michael van Ouwerkerk

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

4 years, 11 months ago (2016-01-27 14:57:00 UTC) #54

Michael van Ouwerkerk

On 2015/12/15 06:01:58, Sam McNally wrote: > ProxyResolverRequestClient: https://codereview.chromium.org/1516493003/ > PermissionService: https://codereview.chromium.org/1513973002/ > BackgroundSyncService: https://codereview.chromium.org/1507233003/ ...

4 years, 10 months ago (2016-02-12 16:40:51 UTC) #55

blundell

On 2016/02/12 16:40:51, Michael van Ouwerkerk wrote: > On 2015/12/15 06:01:58, Sam McNally wrote: > ...

4 years, 10 months ago (2016-02-15 08:41:01 UTC) #56

Sam McNally

4 years, 10 months ago (2016-02-18 07:13:40 UTC) #57

On 2016/02/15 08:41:01, blundell wrote:
> On 2016/02/12 16:40:51, Michael van Ouwerkerk wrote:
> > On 2015/12/15 06:01:58, Sam McNally wrote:
> > > ProxyResolverRequestClient: https://codereview.chromium.org/1516493003/
> > > PermissionService: https://codereview.chromium.org/1513973002/
> > > BackgroundSyncService: https://codereview.chromium.org/1507233003/
> > > 
> > > How many times does this pattern need to occur before StrongBindingSet can
> be
> > > considered?
> > 
> > Was this patch meant to fix crbug.com/556749 ?
> 
> Yes. There's also a straightforward fix here, which I abandoned when informed
of
> this CL:
> 
> https://codereview.chromium.org/1459343003/

This CL isn't likely to land any time soon. You should probably land yours.

Issue 1411063007: Add mojo::StrongBindingSet and use it in GeolocationServiceContext. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : rebase #

Patch Set 7 : #

Patch Set 8 : rebase #

Patch Set 9 : #

Patch Set 10 : #

Messages