service worker: Observe when resource context is shutting down

service worker: Observe when resource context is shutting down

Make SWContextWrapper use OnContextShuttingDown, so it can clear its
ResourceContext on the IO thread then. Before it would wait until
StoragePartition destruction calls Shutdown() which then posts a task on the IO
thread, which can be too late (see bug).

BUG=587621
Committed: https://crrev.com/a8bee310d105d8167d39a9d4d496ba49c823576d
Cr-Commit-Position: refs/heads/master@{#380349}

[falken, 2016-03-08 09:59:10]

falken@chromium.org changed reviewers:
+ horo@chromium.org, mmenke@chromium.org

[falken, 2016-03-08 09:59:11]

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

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:143:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
mmenke: URLRequestContextGetterObserver documentation says "network thread" but
it seems to be called on the IO thread via
ChromeURLRequestContextGetter::NotifyContextShuttingDown. Is this DCHECK safe?

[michaeln, 2016-03-08 22:04:27]

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

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:133: void
ServiceWorkerContextWrapper::InitializeOnIO(
naming nit driveby: since there already are Init() and InitInternal() methods on
this object (with the latter intended to be run on the IO thread), maybe call
this one InitializeResourceContext

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:143:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/03/08 09:59:11, falken wrote:
> mmenke: URLRequestContextGetterObserver documentation says "network thread"
but
> it seems to be called on the IO thread via
> ChromeURLRequestContextGetter::NotifyContextShuttingDown. Is this DCHECK safe?

yes, content::iothread == net::networkthread

[mmenke, 2016-03-09 18:33:43]

This fix LGTM.

Long term, I'd really like to see a careful thinking of StoragePartition /
ProfileIOData shutdown ordering that would make this not be needed, but not
expecting you guys to do that.

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

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:143:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/03/08 22:04:27, michaeln wrote:
> On 2016/03/08 09:59:11, falken wrote:
> > mmenke: URLRequestContextGetterObserver documentation says "network thread"
> but
> > it seems to be called on the IO thread via
> > ChromeURLRequestContextGetter::NotifyContextShuttingDown. Is this DCHECK
safe?
> 
> yes, content::iothread == net::networkthread

Correct.  net/ is below content/, so doesn't know what thread it will be used on
(It should also be possible to create multiple URLRequestContexts that live on
different threads, though there are currently some globals that make doing so a
bit iffy)

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:148: // This
doesn't seem to be called when using content_shell, so we still must
This bug may still exist with content shell, or other embedders, depending on
their URLRequestContext ownership model (If the getter owns the context, they're
safe.  Otherwise, they will probably still have this problem)

It's up to the class that owns the URLRequestContext to tell the
URLRequestContextGetter when it goes away, but currently, only ProfileIOData
does that, I believe.

[falken, 2016-03-10 01:25:34]

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

[falken, 2016-03-10 01:25:35]

Thanks all.

+kinuko for content/browser/storage_partition_impl_map.cc

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

https://codereview.chromium.org/1771283002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.cc:133: void
ServiceWorkerContextWrapper::InitializeOnIO(
On 2016/03/08 22:04:27, michaeln wrote:
> naming nit driveby: since there already are Init() and InitInternal() methods
on
> this object (with the latter intended to be run on the IO thread), maybe call
> this one InitializeResourceContext

Done.

[kinuko, 2016-03-10 03:11:56]

lgtm

[falken, 2016-03-10 03:43:29]

Oh those try job failures were real. Added an if check since
URLResourceContextGetter is null in some unittests.

[falken, 2016-03-10 03:43:45]

The CQ bit was checked by falken@chromium.org

[falken, 2016-03-10 03:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1771283002/#ps60001
(title: "fix unittests")

[commit-bot: I haz the power, 2016-03-10 03:44:09]

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

[commit-bot: I haz the power, 2016-03-10 05:46:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-10 05:48:42]

Description was changed from

==========
service worker: Observe when resource context is shutting down

Make SWContextWrapper use OnContextShuttingDown, so it can clear its
ResourceContext on the IO thread then. Before it would wait until
StoragePartition destruction calls Shutdown() which then posts a task on the IO
thread, which can be too late (see bug).

BUG=587621
==========

to

==========
service worker: Observe when resource context is shutting down

Make SWContextWrapper use OnContextShuttingDown, so it can clear its
ResourceContext on the IO thread then. Before it would wait until
StoragePartition destruction calls Shutdown() which then posts a task on the IO
thread, which can be too late (see bug).

BUG=587621

Committed: https://crrev.com/a8bee310d105d8167d39a9d4d496ba49c823576d
Cr-Commit-Position: refs/heads/master@{#380349}
==========

[commit-bot: I haz the power, 2016-03-10 05:48:43]

Patchset 4 (id:??) landed as
https://crrev.com/a8bee310d105d8167d39a9d4d496ba49c823576d
Cr-Commit-Position: refs/heads/master@{#380349}

[horo, 2016-03-10 06:50:36]

lgtm

[michaeln, 2016-03-10 21:15:53]

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

[michaeln, 2016-03-10 21:15:53]

sorry for the late comment

https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:146:
request_context_getter_->RemoveObserver(this);
You mentioned this isn't called on ContentShell.

I think it might be good to RemoveObserver() in ShutdownOnIO() and to assert
that resource_context_ is null in the dtor? So we don't depend on
ResourcedContextShutdown happening prior to ServiceWorkerContextWrapper
destruction.

[mmenke, 2016-03-10 21:20:51]

https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:146:
request_context_getter_->RemoveObserver(this);
On 2016/03/10 21:15:53, michaeln wrote:
> You mentioned this isn't called on ContentShell.
> 
> I think it might be good to RemoveObserver() in ShutdownOnIO() and to assert
> that resource_context_ is null in the dtor? So we don't depend on
> ResourcedContextShutdown happening prior to ServiceWorkerContextWrapper
> destruction.

I view this as a hack to resolve incorrect class dependencies.  I'm reluctant to
encourage increasing the amount of code that relies on it.

[michaeln, 2016-03-10 22:21:46]

> > You mentioned this isn't called on ContentShell.
> > 
> > I think it might be good to RemoveObserver() in ShutdownOnIO() and to assert
> > that resource_context_ is null in the dtor? So we don't depend on
> > ResourcedContextShutdown happening prior to ServiceWorkerContextWrapper
> > destruction.
> 
> I view this as a hack to resolve incorrect class dependencies.  I'm reluctant
to
> encourage increasing the amount of code that relies on it.

Relies on what? I'm suggesting to not rely on order for these relatively loosely
couple classes.

What order occurs for normal vs incognito vs isolated storage partitons, in all
cases?

Are you saying that OnContextShuttingDown() must be called prior to this object
being deleted? And if so, why is that a good thing and not the opposite? And
who/what agency is responsible for maintaining that invariant?

[mmenke, 2016-03-10 22:33:08]

On 2016/03/10 22:21:46, michaeln wrote:
> > > You mentioned this isn't called on ContentShell.
> > > 
> > > I think it might be good to RemoveObserver() in ShutdownOnIO() and to
assert
> > > that resource_context_ is null in the dtor? So we don't depend on
> > > ResourcedContextShutdown happening prior to ServiceWorkerContextWrapper
> > > destruction.
> > 
> > I view this as a hack to resolve incorrect class dependencies.  I'm
reluctant
> to
> > encourage increasing the amount of code that relies on it.
> 
> Relies on what? I'm suggesting to not rely on order for these relatively
loosely
> couple classes.
> 
> What order occurs for normal vs incognito vs isolated storage partitons, in
all
> cases?
> 
> Are you saying that OnContextShuttingDown() must be called prior to this
object
> being deleted? And if so, why is that a good thing and not the opposite? And
> who/what agency is responsible for maintaining that invariant?

My complaint here is that the storage partition is shutting down after the
ProfileIOData, which it very strongly depends on.  The reason for
URLRequestContextGetter's shutdown stuff was added largely for very weak
dependencies, but that doesn't really seem to be the case here.

[mmenke, 2016-03-10 22:37:58]

On 2016/03/10 21:20:51, mmenke wrote:
>
https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_context_wrapper.cc (right):
> 
>
https://codereview.chromium.org/1771283002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_context_wrapper.cc:146:
> request_context_getter_->RemoveObserver(this);
> On 2016/03/10 21:15:53, michaeln wrote:
> > You mentioned this isn't called on ContentShell.
> > 
> > I think it might be good to RemoveObserver() in ShutdownOnIO() and to assert
> > that resource_context_ is null in the dtor? So we don't depend on
> > ResourcedContextShutdown happening prior to ServiceWorkerContextWrapper
> > destruction.
> 
> I view this as a hack to resolve incorrect class dependencies.  I'm reluctant
to
> encourage increasing the amount of code that relies on it.

Perhaps being a bit strong here...If there were motivated by URLFetchers, I
wouldn't be too concerned about going in this direction.  In this particular
case, I don't think this is really what we want to be relying on, long term.

[michaeln, 2016-03-10 23:04:41]

> Perhaps being a bit strong here...If there were motivated by URLFetchers, I
> wouldn't be too concerned about going in this direction.  In this particular
> case, I don't think this is really what we want to be relying on, long term.

Last word on this... didn't intend to stir up anything...

Forgot what classes we're talking about in particular and look at what this CL
is depending on as coded. Depending on that is folly. I'm saying change that
ever so slightly to not crash if OnBWentByeBye() doesn't happen (as falken
suggested is the case in at least some cases in at least one of our many build
targets). Ymmv.

class A {
  ~A() {
    // Lets just assume OnBWentByeBye was called by complex far removed code.
  }

  void setB(B* b) {
    b_ = b;
    b_ ->Observe(this);
  }

  void OnBWentByeBye() {
    b_ ->RemoveObserver(this)
    b_ = null;
  }
};

class B {
  Observe(...);
  RemoveObserver(...);
};

[mmenke, 2016-03-10 23:30:06]

On 2016/03/10 23:04:41, michaeln wrote:
> > Perhaps being a bit strong here...If there were motivated by URLFetchers, I
> > wouldn't be too concerned about going in this direction.  In this particular
> > case, I don't think this is really what we want to be relying on, long term.
> 
> Last word on this... didn't intend to stir up anything...
> 
> Forgot what classes we're talking about in particular and look at what this CL
> is depending on as coded. Depending on that is folly. I'm saying change that
> ever so slightly to not crash if OnBWentByeBye() doesn't happen (as falken
> suggested is the case in at least some cases in at least one of our many build
> targets). Ymmv.
> 
> class A {
>   ~A() {
>     // Lets just assume OnBWentByeBye was called by complex far removed code.
>   }
> 
>   void setB(B* b) {
>     b_ = b;
>     b_ ->Observe(this);
>   }
> 
>   void OnBWentByeBye() {
>     b_ ->RemoveObserver(this)
>     b_ = null;
>   }
> };
> 
> class B {
>   Observe(...);
>   RemoveObserver(...);
> };

The thing is....Very little depends on the URLRequestContextGetter, almost
everything depends on the Profile, the ProfileIOData, or the RequestContext.

So it's more like:

class A {
  void setB(B* b) {
    b_ = b;
    b_->c()->Observe(this);
  }

  void OnClassCWentByeBye() {
    b_->c()->RemoveObserver(this)
    b_ = null;
  }
}

[falken, 2016-03-11 03:43:18]

On 2016/03/10 23:30:06, mmenke wrote:
> On 2016/03/10 23:04:41, michaeln wrote:
> > > Perhaps being a bit strong here...If there were motivated by URLFetchers,
I
> > > wouldn't be too concerned about going in this direction.  In this
particular
> > > case, I don't think this is really what we want to be relying on, long
term.
> > 
> > Last word on this... didn't intend to stir up anything...
> > 
> > Forgot what classes we're talking about in particular and look at what this
CL
> > is depending on as coded. Depending on that is folly. I'm saying change that
> > ever so slightly to not crash if OnBWentByeBye() doesn't happen (as falken
> > suggested is the case in at least some cases in at least one of our many
build
> > targets). Ymmv.
> > 
> > class A {
> >   ~A() {
> >     // Lets just assume OnBWentByeBye was called by complex far removed
code.
> >   }
> > 
> >   void setB(B* b) {
> >     b_ = b;
> >     b_ ->Observe(this);
> >   }
> > 
> >   void OnBWentByeBye() {
> >     b_ ->RemoveObserver(this)
> >     b_ = null;
> >   }
> > };
> > 
> > class B {
> >   Observe(...);
> >   RemoveObserver(...);
> > };
> 
> The thing is....Very little depends on the URLRequestContextGetter, almost
> everything depends on the Profile, the ProfileIOData, or the RequestContext.
> 
> So it's more like:
> 
> class A {
>   void setB(B* b) {
>     b_ = b;
>     b_->c()->Observe(this);
>   }
> 
>   void OnClassCWentByeBye() {
>     b_->c()->RemoveObserver(this)
>     b_ = null;
>   }
> }

Thanks all. I need to change the patch a little anyway. The membots are
complaining of a leak on unit_tests
<https://bugs.chromium.org/p/chromium/issues/detail?id=593913>. It looks like
request_context_getter_ must be released in the IO thread in ShutdownOnIO. I
guess relying on it being destroyed in the dtor is too late.

I think calling RemoveObserver in ShutdownOnIO as Michael suggests is also good
(or needed?) for cleanup. Although the class dependencies may be incorrect, it's
the current reality this class has to deal with, and it's just a small amount of
code.