Issue 221963003: Reduce dependency of TiclInvalidationService on Profile

Issue 221963003: Reduce dependency of TiclInvalidationService on Profile (Closed)

Created:
6 years, 8 months ago by bartfab (slow)

Modified:
6 years, 8 months ago

Reviewers:
Andrew T Wilson (Slow), rlarocque, Nicolas Zea, dcheng

CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, maniscalco+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Reduce dependency of TiclInvalidationService on Profile With this CL, a URLRequestContext and an InvalidationStateTracker are passed to TiclInvalidationService as explicit dependencies. This reduces the dependency of TiclInvalidationService on Profile, taking a step toward eventually removing the dependency altogether. BUG=358696 TEST=Updated unit tests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264226 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264311

Patch Set 1 #

Patch Set 2 : Pass scoped_refptr as const reference. #

Total comments: 10

Patch Set 3 : Comments addressed. #

Total comments: 2

Patch Set 4 : Ensure that weak pointers cannot be used to access partially destructed objects. #

Patch Set 5 : Made InvalidationStateTracker not vend WeakPtrs. #

Total comments: 4

Patch Set 6 : Removed need to have WeakPtrFactories for other classes in production. #

Patch Set 7 : Removed need to have WeakPtrFactories for other classes in tests. #

Total comments: 3

Patch Set 8 : Pass invalidation_state_tracker to Init(). This is unnecessary for a KeyedService but may be handy … #

Patch Set 9 : Compilation fixes. #

Patch Set 10 : Experimenting with EXPORT macros to try and fix multiply-defined symbols on Windows. #

Patch Set 11 : Fix sync_client compilation. #

Patch Set 12 : Fix sync_listen_notifications. #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+179 lines, -103 lines)			Patch
M	chrome/browser/invalidation/invalidation_service_factory.cc	View	1 2 3 4 5 6 7 8	3 chunks	+9 lines, -3 lines	0 comments	Download
M	chrome/browser/invalidation/invalidation_service_test_template.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/invalidation/invalidator_storage.h	View	1 2	3 chunks	+2 lines, -4 lines	0 comments	Download
M	chrome/browser/invalidation/invalidator_storage.cc	View	1 2 4	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/invalidation/invalidator_storage_unittest.cc	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/invalidation/ticl_invalidation_service.h	View	1 2 3 4 5 6 7	6 chunks	+18 lines, -6 lines	0 comments	Download
M	chrome/browser/invalidation/ticl_invalidation_service.cc	View	1 2 3 4 5 6 7	11 chunks	+29 lines, -24 lines	0 comments	Download
M	chrome/browser/invalidation/ticl_invalidation_service_unittest.cc	View	1 2 3 4 5 6 7	6 chunks	+18 lines, -4 lines	0 comments	Download
M	sync/notifier/fake_invalidation_state_tracker.h	View	1 2 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	sync/notifier/fake_invalidation_state_tracker.cc	View	1 2 4	1 chunk	+1 line, -1 line	0 comments	Download
M	sync/notifier/invalidation_state_tracker.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -6 lines	0 comments	Download
M	sync/notifier/non_blocking_invalidator.h	View	1 2 3 4 5	4 chunks	+23 lines, -13 lines	0 comments	Download
M	sync/notifier/non_blocking_invalidator.cc	View	1 2 3 4 5	9 chunks	+63 lines, -25 lines	0 comments	Download
M	sync/notifier/non_blocking_invalidator_unittest.cc	View	1 2 3 4 5 6	4 chunks	+1 line, -6 lines	0 comments	Download
M	sync/tools/null_invalidation_state_tracker.h	View	1 2 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	sync/tools/null_invalidation_state_tracker.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	sync/tools/sync_client.cc	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+1 line, -3 lines	0 comments	Download
M	sync/tools/sync_listen_notifications.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -2 lines	0 comments	Download

Messages

Total messages: 39 (0 generated)

Expand Messages | Collapse Messages

bartfab (slow)

Hi Daniel, Could you review chrome/browser/invalidation/*? Hi Drew, Could you review sync/notifier/invalidation_state_tracker.h?

6 years, 8 months ago (2014-04-02 16:33:27 UTC) #1

bartfab (slow)

Richard, since Drew is out next week, could you review sync/notifier/invalidation_state_tracker.h? Daniel, do you have ...

6 years, 8 months ago (2014-04-11 18:58:59 UTC) #3

dcheng

https://codereview.chromium.org/221963003/diff/20001/chrome/browser/invalidation/invalidation_service_factory.cc File chrome/browser/invalidation/invalidation_service_factory.cc (right): https://codereview.chromium.org/221963003/diff/20001/chrome/browser/invalidation/invalidation_service_factory.cc#newcode26 chrome/browser/invalidation/invalidation_service_factory.cc:26: #include "net/url_request/url_request_context_getter.h" Do we actually need this include? It ...

6 years, 8 months ago (2014-04-11 19:54:00 UTC) #4

rlarocque

On 2014/04/11 19:54:00, dcheng wrote: > https://codereview.chromium.org/221963003/diff/20001/chrome/browser/invalidation/invalidation_service_factory.cc > File chrome/browser/invalidation/invalidation_service_factory.cc (right): > > https://codereview.chromium.org/221963003/diff/20001/chrome/browser/invalidation/invalidation_service_factory.cc#newcode26 > ...

6 years, 8 months ago (2014-04-11 20:03:37 UTC) #5

bartfab (slow)

6 years, 8 months ago (2014-04-14 09:20:08 UTC) #6

dcheng

https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidation_state_tracker.h File sync/notifier/invalidation_state_tracker.h (right): https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidation_state_tracker.h#newcode40 sync/notifier/invalidation_state_tracker.h:40: return weak_factory_.GetWeakPtr(); I don't think we should be publicly ...

6 years, 8 months ago (2014-04-14 21:39:59 UTC) #7

bartfab (slow)

6 years, 8 months ago (2014-04-14 22:06:00 UTC) #8

https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
File sync/notifier/invalidation_state_tracker.h (right):

https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
sync/notifier/invalidation_state_tracker.h:40: return
weak_factory_.GetWeakPtr();
On 2014/04/14 21:39:59, dcheng wrote:
> I don't think we should be publicly exposing weak pointers in the interface.
> It's generally considered an antipattern, and ajwong points out that this
could
> result in if(ptr) returning true for a partially destructed object.
> 
> Not sure of a good solution yet though... this is an issue that's been in sync
> for a long time, but previously, it was at least a hidden implementation
detail
> inside chrome/.
> 
> A simple (but not necessarily correct fix) might be to make this ref
counted...
> I'm not sure how I feel about that, but it's definitely better than having
this
> interface return weak pointers.

Actually, based on discussions we had on chromium-dev, an explicit GetWeakPtr()
method is preferable to tbe implicit SupportsWeakPtr, which is now deprecated
because it makes the risk of accessing a partially destroyed object even worse.

Ref counting is not a solution to the same problem. With weak pointers, you have
explicit ownership and lifetime. Whoever has a naked pointer owns the object.
Everyone else has weak pointers that may become invalid at any point, when the
object is destroyed. With ref counting, ownership is shared and nobody can be
sure when exactly the object gets destroyed. Whoever happens to drop the last
reference will trigger the destructor. This makes it impossible to clean up and
e.g. unregister as an observer in a well-defined manner.

If you are worried about weak pointers being used to access a partially
destroyed object here, I implemented a simple solution: The derived class the
destructor is the very first thing that gets called when an object goes out of
scope. If that destructor calls weak_factory_.InvalidateWeakPtrs(); as the very
first thing, you are guaranteed that nobody can access a partially destructed
object.

dcheng

On 2014/04/14 22:06:00, bartfab wrote: > https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidation_state_tracker.h > File sync/notifier/invalidation_state_tracker.h (right): > > https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidation_state_tracker.h#newcode40 > ...

6 years, 8 months ago (2014-04-14 22:11:08 UTC) #9

On 2014/04/14 22:06:00, bartfab wrote:
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> File sync/notifier/invalidation_state_tracker.h (right):
> 
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> sync/notifier/invalidation_state_tracker.h:40: return
> weak_factory_.GetWeakPtr();
> On 2014/04/14 21:39:59, dcheng wrote:
> > I don't think we should be publicly exposing weak pointers in the interface.
> > It's generally considered an antipattern, and ajwong points out that this
> could
> > result in if(ptr) returning true for a partially destructed object.
> > 
> > Not sure of a good solution yet though... this is an issue that's been in
sync
> > for a long time, but previously, it was at least a hidden implementation
> detail
> > inside chrome/.
> > 
> > A simple (but not necessarily correct fix) might be to make this ref
> counted...
> > I'm not sure how I feel about that, but it's definitely better than having
> this
> > interface return weak pointers.
> 
> Actually, based on discussions we had on chromium-dev, an explicit
GetWeakPtr()
> method is preferable to tbe implicit SupportsWeakPtr, which is now deprecated
> because it makes the risk of accessing a partially destroyed object even
worse.
> 

Right. I think having GetWeakPtr() as an interface method is also bad.

> Ref counting is not a solution to the same problem. With weak pointers, you
have
> explicit ownership and lifetime. Whoever has a naked pointer owns the object.
> Everyone else has weak pointers that may become invalid at any point, when the
> object is destroyed. With ref counting, ownership is shared and nobody can be
> sure when exactly the object gets destroyed. Whoever happens to drop the last
> reference will trigger the destructor. This makes it impossible to clean up
and
> e.g. unregister as an observer in a well-defined manner.
> 

That's true... but InvalidatorStorage also does nothing significant in its
destructor, nor does it have observers. cs.chromium.org appears to be broken
atm, so it's hard to do more research into why things exist the way they do.
It's unfortunate that things work the way they do today with WeakPtr/WeakHandle,
but I'd rather not make it worse.

> If you are worried about weak pointers being used to access a partially
> destroyed object here, I implemented a simple solution: The derived class the
> destructor is the very first thing that gets called when an object goes out of
> scope. If that destructor calls weak_factory_.InvalidateWeakPtrs(); as the
very
> first thing, you are guaranteed that nobody can access a partially destructed
> object.

This is dependent on all implementations remembering to include this first,
which is not ideal.

+zea to see if he has any insight into why it exists the way it does today--at
least that might help guide a short-term solution that's acceptable.

bartfab (slow)

On 2014/04/14 22:11:08, dcheng wrote: > On 2014/04/14 22:06:00, bartfab wrote: > > > https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidation_state_tracker.h ...

6 years, 8 months ago (2014-04-14 22:23:27 UTC) #10

On 2014/04/14 22:11:08, dcheng wrote:
> On 2014/04/14 22:06:00, bartfab wrote:
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > File sync/notifier/invalidation_state_tracker.h (right):
> > 
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > sync/notifier/invalidation_state_tracker.h:40: return
> > weak_factory_.GetWeakPtr();
> > On 2014/04/14 21:39:59, dcheng wrote:
> > > I don't think we should be publicly exposing weak pointers in the
interface.
> > > It's generally considered an antipattern, and ajwong points out that this
> > could
> > > result in if(ptr) returning true for a partially destructed object.
> > > 
> > > Not sure of a good solution yet though... this is an issue that's been in
> sync
> > > for a long time, but previously, it was at least a hidden implementation
> > detail
> > > inside chrome/.
> > > 
> > > A simple (but not necessarily correct fix) might be to make this ref
> > counted...
> > > I'm not sure how I feel about that, but it's definitely better than having
> > this
> > > interface return weak pointers.
> > 
> > Actually, based on discussions we had on chromium-dev, an explicit
> GetWeakPtr()
> > method is preferable to tbe implicit SupportsWeakPtr, which is now
deprecated
> > because it makes the risk of accessing a partially destroyed object even
> worse.
> > 
> 
> Right. I think having GetWeakPtr() as an interface method is also bad.

I think weak pointers are very useful and if anything, consider ref counting
dangerous. A git grep for GetWeakPtr() in the Chrome source leads to 3165 hits
by the way. This is a very common pattern in Chrome that is often the correct
solution to a problem.

> 
> > Ref counting is not a solution to the same problem. With weak pointers, you
> have
> > explicit ownership and lifetime. Whoever has a naked pointer owns the
object.
> > Everyone else has weak pointers that may become invalid at any point, when
the
> > object is destroyed. With ref counting, ownership is shared and nobody can
be
> > sure when exactly the object gets destroyed. Whoever happens to drop the
last
> > reference will trigger the destructor. This makes it impossible to clean up
> and
> > e.g. unregister as an observer in a well-defined manner.
> > 
> 
> That's true... but InvalidatorStorage also does nothing significant in its
> destructor, nor does it have observers. http://cs.chromium.org appears to be
broken
> atm, so it's hard to do more research into why things exist the way they do.
> It's unfortunate that things work the way they do today with
WeakPtr/WeakHandle,
> but I'd rather not make it worse.

Yes, a WeakHandle is the consumer of the WeakPtr here. While code search is
down, you can see all the places that access weak pointers for this class right
in the CL, because I had to change all occurrences of AsWeakPtr() to
GetWeakPtr(). You can see the conversion to a WeakHandle in line 423 of
ticl_invalidation_service.cc.

> 
> > If you are worried about weak pointers being used to access a partially
> > destroyed object here, I implemented a simple solution: The derived class
the
> > destructor is the very first thing that gets called when an object goes out
of
> > scope. If that destructor calls weak_factory_.InvalidateWeakPtrs(); as the
> very
> > first thing, you are guaranteed that nobody can access a partially
destructed
> > object.
> 
> This is dependent on all implementations remembering to include this first,
> which is not ideal.

True. But it is equally true (or actually, slightly worse) with the
SupportsWeakPtr that was previously used. There is no way for a base class to
ensure that a derived class does something specific at a given point. C++ does
not have a mechanism for it.

As you said yourself, InvalidatorStorage does nothing significant in its
destructore. For ref counting to be safe, you would have to rely on this always
remaining the case. If you are willing to make that bet, WeakPtr is also safe.
If the InvalidatorStorage destructor does not access any of its members, the
base class destructor will run immediately after that, tearing down the weak
pointer factory. There is no opportunity for other code to make invalid accesses
in between - the entire destruction is single-threaded because WeakPtrs are not
thread safe and must never be accessed from different threeads.

> 
> +zea to see if he has any insight into why it exists the way it does today--at
> least that might help guide a short-term solution that's acceptable.

Nicolas Zea

On 2014/04/14 22:23:27, bartfab wrote: > On 2014/04/14 22:11:08, dcheng wrote: > > On 2014/04/14 ...

6 years, 8 months ago (2014-04-14 22:51:27 UTC) #11

On 2014/04/14 22:23:27, bartfab wrote:
> On 2014/04/14 22:11:08, dcheng wrote:
> > On 2014/04/14 22:06:00, bartfab wrote:
> > >
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > > File sync/notifier/invalidation_state_tracker.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > > sync/notifier/invalidation_state_tracker.h:40: return
> > > weak_factory_.GetWeakPtr();
> > > On 2014/04/14 21:39:59, dcheng wrote:
> > > > I don't think we should be publicly exposing weak pointers in the
> interface.
> > > > It's generally considered an antipattern, and ajwong points out that
this
> > > could
> > > > result in if(ptr) returning true for a partially destructed object.
> > > > 
> > > > Not sure of a good solution yet though... this is an issue that's been
in
> > sync
> > > > for a long time, but previously, it was at least a hidden implementation
> > > detail
> > > > inside chrome/.
> > > > 
> > > > A simple (but not necessarily correct fix) might be to make this ref
> > > counted...
> > > > I'm not sure how I feel about that, but it's definitely better than
having
> > > this
> > > > interface return weak pointers.
> > > 
> > > Actually, based on discussions we had on chromium-dev, an explicit
> > GetWeakPtr()
> > > method is preferable to tbe implicit SupportsWeakPtr, which is now
> deprecated
> > > because it makes the risk of accessing a partially destroyed object even
> > worse.
> > > 
> > 
> > Right. I think having GetWeakPtr() as an interface method is also bad.
> 
> I think weak pointers are very useful and if anything, consider ref counting
> dangerous. A git grep for GetWeakPtr() in the Chrome source leads to 3165 hits
> by the way. This is a very common pattern in Chrome that is often the correct
> solution to a problem.
> 
> > 
> > > Ref counting is not a solution to the same problem. With weak pointers,
you
> > have
> > > explicit ownership and lifetime. Whoever has a naked pointer owns the
> object.
> > > Everyone else has weak pointers that may become invalid at any point, when
> the
> > > object is destroyed. With ref counting, ownership is shared and nobody can
> be
> > > sure when exactly the object gets destroyed. Whoever happens to drop the
> last
> > > reference will trigger the destructor. This makes it impossible to clean
up
> > and
> > > e.g. unregister as an observer in a well-defined manner.
> > > 
> > 
> > That's true... but InvalidatorStorage also does nothing significant in its
> > destructor, nor does it have observers. http://cs.chromium.org appears to be
> broken
> > atm, so it's hard to do more research into why things exist the way they do.
> > It's unfortunate that things work the way they do today with
> WeakPtr/WeakHandle,
> > but I'd rather not make it worse.
> 
> Yes, a WeakHandle is the consumer of the WeakPtr here. While code search is
> down, you can see all the places that access weak pointers for this class
right
> in the CL, because I had to change all occurrences of AsWeakPtr() to
> GetWeakPtr(). You can see the conversion to a WeakHandle in line 423 of
> ticl_invalidation_service.cc.
> 
> > 
> > > If you are worried about weak pointers being used to access a partially
> > > destroyed object here, I implemented a simple solution: The derived class
> the
> > > destructor is the very first thing that gets called when an object goes
out
> of
> > > scope. If that destructor calls weak_factory_.InvalidateWeakPtrs(); as the
> > very
> > > first thing, you are guaranteed that nobody can access a partially
> destructed
> > > object.
> > 
> > This is dependent on all implementations remembering to include this first,
> > which is not ideal.
> 
> True. But it is equally true (or actually, slightly worse) with the
> SupportsWeakPtr that was previously used. There is no way for a base class to
> ensure that a derived class does something specific at a given point. C++ does
> not have a mechanism for it.
> 
> As you said yourself, InvalidatorStorage does nothing significant in its
> destructore. For ref counting to be safe, you would have to rely on this
always
> remaining the case. If you are willing to make that bet, WeakPtr is also safe.
> If the InvalidatorStorage destructor does not access any of its members, the
> base class destructor will run immediately after that, tearing down the weak
> pointer factory. There is no opportunity for other code to make invalid
accesses
> in between - the entire destruction is single-threaded because WeakPtrs are
not
> thread safe and must never be accessed from different threeads.
> 
> > 
> > +zea to see if he has any insight into why it exists the way it does
today--at
> > least that might help guide a short-term solution that's acceptable.

If I remember/understand this code correctly, the weakptrs/weakhandles are just
a way of dealing with an uncertain lifetime of the invalidator registrar, which
lives on the UI thread but gets called from the IO thread. But, it seems that
the TiclInvalidationService manages the lifetime of both the registrar and the
NonBlockingInvalidator, so I think this may not be necessary any more. You might
be able to have the NonBlockingInvalidator have a raw pointer to the
registrar/state tracker, with the assumption that it will always outlive it. The
Core could then call into the NonBlockingInvalidator via a weakptr it vends
internally, which can then call into the state tracker directly.

(Still playing catchup on this though, as it's been a while since I looked at
any of this, so please correct  me if I'm wrong)

bartfab (slow)

On 2014/04/14 22:51:27, Nicolas Zea wrote: > On 2014/04/14 22:23:27, bartfab wrote: > > On ...

6 years, 8 months ago (2014-04-15 12:39:02 UTC) #12

On 2014/04/14 22:51:27, Nicolas Zea wrote:
> On 2014/04/14 22:23:27, bartfab wrote:
> > On 2014/04/14 22:11:08, dcheng wrote:
> > > On 2014/04/14 22:06:00, bartfab wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > > > File sync/notifier/invalidation_state_tracker.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/221963003/diff/40001/sync/notifier/invalidati...
> > > > sync/notifier/invalidation_state_tracker.h:40: return
> > > > weak_factory_.GetWeakPtr();
> > > > On 2014/04/14 21:39:59, dcheng wrote:
> > > > > I don't think we should be publicly exposing weak pointers in the
> > interface.
> > > > > It's generally considered an antipattern, and ajwong points out that
> this
> > > > could
> > > > > result in if(ptr) returning true for a partially destructed object.
> > > > > 
> > > > > Not sure of a good solution yet though... this is an issue that's been
> in
> > > sync
> > > > > for a long time, but previously, it was at least a hidden
implementation
> > > > detail
> > > > > inside chrome/.
> > > > > 
> > > > > A simple (but not necessarily correct fix) might be to make this ref
> > > > counted...
> > > > > I'm not sure how I feel about that, but it's definitely better than
> having
> > > > this
> > > > > interface return weak pointers.
> > > > 
> > > > Actually, based on discussions we had on chromium-dev, an explicit
> > > GetWeakPtr()
> > > > method is preferable to tbe implicit SupportsWeakPtr, which is now
> > deprecated
> > > > because it makes the risk of accessing a partially destroyed object even
> > > worse.
> > > > 
> > > 
> > > Right. I think having GetWeakPtr() as an interface method is also bad.
> > 
> > I think weak pointers are very useful and if anything, consider ref counting
> > dangerous. A git grep for GetWeakPtr() in the Chrome source leads to 3165
hits
> > by the way. This is a very common pattern in Chrome that is often the
correct
> > solution to a problem.
> > 
> > > 
> > > > Ref counting is not a solution to the same problem. With weak pointers,
> you
> > > have
> > > > explicit ownership and lifetime. Whoever has a naked pointer owns the
> > object.
> > > > Everyone else has weak pointers that may become invalid at any point,
when
> > the
> > > > object is destroyed. With ref counting, ownership is shared and nobody
can
> > be
> > > > sure when exactly the object gets destroyed. Whoever happens to drop the
> > last
> > > > reference will trigger the destructor. This makes it impossible to clean
> up
> > > and
> > > > e.g. unregister as an observer in a well-defined manner.
> > > > 
> > > 
> > > That's true... but InvalidatorStorage also does nothing significant in its
> > > destructor, nor does it have observers. http://cs.chromium.org appears to
be
> > broken
> > > atm, so it's hard to do more research into why things exist the way they
do.
> > > It's unfortunate that things work the way they do today with
> > WeakPtr/WeakHandle,
> > > but I'd rather not make it worse.
> > 
> > Yes, a WeakHandle is the consumer of the WeakPtr here. While code search is
> > down, you can see all the places that access weak pointers for this class
> right
> > in the CL, because I had to change all occurrences of AsWeakPtr() to
> > GetWeakPtr(). You can see the conversion to a WeakHandle in line 423 of
> > ticl_invalidation_service.cc.
> > 
> > > 
> > > > If you are worried about weak pointers being used to access a partially
> > > > destroyed object here, I implemented a simple solution: The derived
class
> > the
> > > > destructor is the very first thing that gets called when an object goes
> out
> > of
> > > > scope. If that destructor calls weak_factory_.InvalidateWeakPtrs(); as
the
> > > very
> > > > first thing, you are guaranteed that nobody can access a partially
> > destructed
> > > > object.
> > > 
> > > This is dependent on all implementations remembering to include this
first,
> > > which is not ideal.
> > 
> > True. But it is equally true (or actually, slightly worse) with the
> > SupportsWeakPtr that was previously used. There is no way for a base class
to
> > ensure that a derived class does something specific at a given point. C++
does
> > not have a mechanism for it.
> > 
> > As you said yourself, InvalidatorStorage does nothing significant in its
> > destructore. For ref counting to be safe, you would have to rely on this
> always
> > remaining the case. If you are willing to make that bet, WeakPtr is also
safe.
> > If the InvalidatorStorage destructor does not access any of its members, the
> > base class destructor will run immediately after that, tearing down the weak
> > pointer factory. There is no opportunity for other code to make invalid
> accesses
> > in between - the entire destruction is single-threaded because WeakPtrs are
> not
> > thread safe and must never be accessed from different threeads.
> > 
> > > 
> > > +zea to see if he has any insight into why it exists the way it does
> today--at
> > > least that might help guide a short-term solution that's acceptable.
> 
> If I remember/understand this code correctly, the weakptrs/weakhandles are
just
> a way of dealing with an uncertain lifetime of the invalidator registrar,
which
> lives on the UI thread but gets called from the IO thread. But, it seems that
> the TiclInvalidationService manages the lifetime of both the registrar and the
> NonBlockingInvalidator, so I think this may not be necessary any more. You
might
> be able to have the NonBlockingInvalidator have a raw pointer to the
> registrar/state tracker, with the assumption that it will always outlive it.
The
> Core could then call into the NonBlockingInvalidator via a weakptr it vends
> internally, which can then call into the state tracker directly.
> 
> (Still playing catchup on this though, as it's been a while since I looked at
> any of this, so please correct  me if I'm wrong)

I changed the CL as you suggested: InvalidationStateTracker and the classes that
inherit from it no longer vend WeakPtrs. Instead, anyone who requires a WeakPtr
to such an object uses their own external WeakPtrFactory. This applies to
NonBlockingInvalidator and various test classes.

I made the changes you requested but I do not think the resulting code is
better. External WeakPtrFactories are a construct without precedent anywhere in
the Chrome code AFAICT. It is dangerous to be introducing such a new paradigm,
hoping that everyone will get it right in the future, simply because you do not
like GetWeakPtr() as part of a class interface.

Nicolas Zea

https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc File sync/notifier/non_blocking_invalidator.cc (right): https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc#newcode189 sync/notifier/non_blocking_invalidator.cc:189: delegate_observer_.Call( these calls here would no longer interact directly ...

6 years, 8 months ago (2014-04-15 16:30:41 UTC) #13

bartfab (slow)

https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc File sync/notifier/non_blocking_invalidator.cc (right): https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc#newcode189 sync/notifier/non_blocking_invalidator.cc:189: delegate_observer_.Call( On 2014/04/15 16:30:42, Nicolas Zea wrote: > these ...

6 years, 8 months ago (2014-04-15 16:48:43 UTC) #14

Nicolas Zea

https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc File sync/notifier/non_blocking_invalidator.cc (right): https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocking_invalidator.cc#newcode189 sync/notifier/non_blocking_invalidator.cc:189: delegate_observer_.Call( On 2014/04/15 16:48:44, bartfab wrote: > On 2014/04/15 ...

6 years, 8 months ago (2014-04-15 17:50:54 UTC) #15

https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocki...
File sync/notifier/non_blocking_invalidator.cc (right):

https://codereview.chromium.org/221963003/diff/70001/sync/notifier/non_blocki...
sync/notifier/non_blocking_invalidator.cc:189: delegate_observer_.Call(
On 2014/04/15 16:48:44, bartfab wrote:
> On 2014/04/15 16:30:42, Nicolas Zea wrote:
> > these calls here would no longer interact directly with the invalidation
> > handler's weak handle. Instead they'd be routed through the
> > NonBlockingInvalidator to deal with lifetime issues.
> 
> If it was only NonBlockingInvalidator::Core that accessed the
> InvalidationStateTracker, what you said would be easy. However, the WeakHandle
> is passed on to a lot of other classes that invoke methods on it:
> 
> - NonBlockingInvalidator::Core passes the invalidation_state_tracker to an
> InvalidationNotifier.
> - The InvalidationNotifier passes the invalidation_state_tracker to a
> SyncInvalidationListener.
> - The SyncInvalidationListener makes calls to the invalidation_state_tracker
> directly.
> 
> You would need to route all these calls back up to
NonBlockingInvalidator::Core
> who could then dispatch them if the InvalidationStateTracker is still alive.
> 
> This is a large refactoring of your code. I do not know the code well enough
to
> want to make such refactorings. If you want to make such fundamental changes,
> let's agree on a first step that will allow me to remove the Profile
dependency
> and I will hand off to you at that point.

Anything beyond the core isn't a concern though. I still think this can be a
minimal change.

One way would be to have the NonBlockingInvalidator implement (or have a member
implement) the InvalidationHandler interface. It would then just delegate all
the methods to the real invalidation handler. You could then create a WeakHandle
to the NonBlockingInvalidator (or whichever member implements the interface),
and have that be what you pass around.

The main thing is that the NonBlockingInvalidator controls lifetime itself, so
there's no need to care about getting a weak pointer to anything outside of it.

bartfab (slow)

Please take another look. The WeakPtr is contained to NonBlockingInvalidator only now.

6 years, 8 months ago (2014-04-15 20:01:11 UTC) #16

dcheng

https://codereview.chromium.org/221963003/diff/110001/chrome/browser/invalidation/ticl_invalidation_service.cc File chrome/browser/invalidation/ticl_invalidation_service.cc (right): https://codereview.chromium.org/221963003/diff/110001/chrome/browser/invalidation/ticl_invalidation_service.cc#newcode331 chrome/browser/invalidation/ticl_invalidation_service.cc:331: invalidation_state_tracker_.reset(); Hm. This means that if we shutdown sync ...

6 years, 8 months ago (2014-04-15 20:12:41 UTC) #17

bartfab (slow)

6 years, 8 months ago (2014-04-15 20:16:05 UTC) #18

bartfab (slow)

6 years, 8 months ago (2014-04-15 20:19:39 UTC) #19

dcheng

On 2014/04/15 21:56:00, Nicolas Zea wrote: > sync/ LGTM c/b/invalidations/ LGTM.

6 years, 8 months ago (2014-04-15 22:29:05 UTC) #21

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/221963003/130001

6 years, 8 months ago (2014-04-15 22:31:06 UTC) #23

commit-bot: I haz the power

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

6 years, 8 months ago (2014-04-15 23:29:39 UTC) #24

commit-bot: I haz the power

Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel

6 years, 8 months ago (2014-04-15 23:29:40 UTC) #25

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/221963003/50002

6 years, 8 months ago (2014-04-16 09:11:49 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/221963003/160001

6 years, 8 months ago (2014-04-16 11:58:20 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/221963003/180001

6 years, 8 months ago (2014-04-16 17:44:59 UTC) #32

eae

On 2014/04/16 17:44:59, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...

6 years, 8 months ago (2014-04-16 17:47:06 UTC) #33

bartfab (slow)

On 2014/04/16 17:47:06, eae wrote: > On 2014/04/16 17:44:59, I haz the power (commit-bot) wrote: ...

6 years, 8 months ago (2014-04-16 17:48:16 UTC) #34

eae

On 2014/04/16 17:48:16, bartfab wrote: > On 2014/04/16 17:47:06, eae wrote: > > On 2014/04/16 ...

6 years, 8 months ago (2014-04-16 17:50:00 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/221963003/200001

6 years, 8 months ago (2014-04-16 17:52:30 UTC) #38

Message was sent while issue was closed.

Change committed as 264311

Expand Messages | Collapse Messages