Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object.

BUG=625051
Review-Url: https://codereview.chromium.org/2925423002
Cr-Commit-Position: refs/heads/master@{#478897}
Committed: https://chromium.googlesource.com/chromium/src/+/f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3

[dominickn, 2017-06-09 06:23:55]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 06:24:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-06-09 06:54:12]

dominickn@chromium.org changed reviewers:
+ kinuko@chromium.org

[dominickn, 2017-06-09 06:54:13]

PTAL thanks! This is follows on from
https://codereview.chromium.org/2931033003/.

[commit-bot: I haz the power, 2017-06-09 08:19:13]

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

[commit-bot: I haz the power, 2017-06-09 08:19:14]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-09 08:51:38]

Description was changed from

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object. It also addresses a bug where observers
added after the Init() method is called will never be propagated into
the underlying wrapped ServiceWorkerContextCore, even after the wrapped
object is recreated.

BUG=625051
==========

to

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object. It also addresses a bug where observers
added after the Init() method is called will never be propagated into
the underlying wrapped ServiceWorkerContextCore, even after the wrapped
object is recreated.

BUG=625051
==========

[kinuko, 2017-06-09 08:51:38]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org

[kinuko, 2017-06-09 08:51:48]

https://codereview.chromium.org/2925423002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2925423002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:276:
observer_list_(observer_list),
I might not be following this part, why is this different from
old_context->observer_list_?  Do we have a test for the bug you mentioned in the
description?

[dominickn, 2017-06-12 23:55:55]

https://codereview.chromium.org/2925423002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2925423002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_core.cc:276:
observer_list_(observer_list),
On 2017/06/09 08:51:47, kinuko wrote:
> I might not be following this part, why is this different from
> old_context->observer_list_?  Do we have a test for the bug you mentioned in
the
> description?

I think there's a potential use-after-free here. Once you add an observer, you
can never actually remove it after the ServiceWorkerContextCore is created until
it is destroyed (the only way to add/remove . Plus, if you add an observer after
the ServiceWorkerContextCore is created, it will never actually be added.

I'm not sure how much of an issue this is - I'm happy to revert this back to how
the code was (let me know if there is an underlying assumption here that I
haven't seen).

[dominickn, 2017-06-12 23:56:45]

Submit too soon:

"I think there's a potential use-after-free here. Once you add an observer, you
can never actually remove it after the ServiceWorkerContextCore is created until
it is destroyed (the only way to add/remove a CoreObserver is via the Wrapper)."

[kinuko, 2017-06-13 01:06:28]

On 2017/06/12 23:56:45, dominickn wrote:
> Submit too soon:
> 
> "I think there's a potential use-after-free here. Once you add an observer,
you
> can never actually remove it after the ServiceWorkerContextCore is created
until
> it is destroyed (the only way to add/remove a CoreObserver is via the
Wrapper)."

I mean, the observer_list is thread-safe ref-counted and they seem to be shared
by ServiceWorkerContextCore and ServiceWorkerContextWrapper, isn't it the case?

  scoped_refptr<base::ObserverListThreadSafe<ServiceWorkerContextObserver>>
      observer_list_;

  Where ObserverListThreadSafe is 

  template <class ObserverType>
  class ObserverListThreadSafe
    : public RefCountedThreadSafe<ObserverListThreadSafe<ObserverType>>

[dominickn, 2017-06-13 01:22:26]

Description was changed from

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object. It also addresses a bug where observers
added after the Init() method is called will never be propagated into
the underlying wrapped ServiceWorkerContextCore, even after the wrapped
object is recreated.

BUG=625051
==========

to

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object.

BUG=625051
==========

[dominickn, 2017-06-13 01:23:35]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-06-13 01:23:37]

On 2017/06/13 01:06:28, kinuko wrote:
> On 2017/06/12 23:56:45, dominickn wrote:
> > Submit too soon:
> > 
> > "I think there's a potential use-after-free here. Once you add an observer,
> you
> > can never actually remove it after the ServiceWorkerContextCore is created
> until
> > it is destroyed (the only way to add/remove a CoreObserver is via the
> Wrapper)."
> 
> I mean, the observer_list is thread-safe ref-counted and they seem to be
shared
> by ServiceWorkerContextCore and ServiceWorkerContextWrapper, isn't it the
case?
> 
>   scoped_refptr<base::ObserverListThreadSafe<ServiceWorkerContextObserver>>
>       observer_list_;
> 
>   Where ObserverListThreadSafe is 
> 
>   template <class ObserverType>
>   class ObserverListThreadSafe
>     : public RefCountedThreadSafe<ObserverListThreadSafe<ObserverType>>

Ah, of course! Thanks for explaining that, it makes sense now. Reverted it back.
:)

[commit-bot: I haz the power, 2017-06-13 01:24:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 03:58:29]

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

[commit-bot: I haz the power, 2017-06-13 03:58:30]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-13 04:00:35]

lgtm

[dominickn, 2017-06-13 04:02:09]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-06-13 04:02:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 04:05:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497326529043530,
"parent_rev": "317b0a78cf083a14fcf59871e03ca049fac2822d", "commit_rev":
"f86ff4e07ecc10e97d9c677b44b1a46d91d1caf3"}

[commit-bot: I haz the power, 2017-06-13 04:05:41]

Description was changed from

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object.

BUG=625051
==========

to

==========
Move all calls on ServiceWorkerContextCoreObserver to ServiceWorkerContextCore.

Currently, ServiceWorkerContextWrapper holds a base::ObserverList of
ServiceWorkerContextCoreObservers, which are passed to the wrapped
ServiceWorkerContextCore upon creation. However, the OnStorageWiped call
is only made by the wrapping class, not the wrapped class.

This CL moves the OnStorageWiped observer method to be triggered by the
wrapped ServiceWorkerContextCore object. This means all observer methods
are triggered by that object.

BUG=625051

Review-Url: https://codereview.chromium.org/2925423002
Cr-Commit-Position: refs/heads/master@{#478897}
Committed:
https://chromium.googlesource.com/chromium/src/+/f86ff4e07ecc10e97d9c677b44b1...
==========

[commit-bot: I haz the power, 2017-06-13 04:05:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f86ff4e07ecc10e97d9c677b44b1...