service worker: Avoid starting up for activation during shutdown

service worker: Avoid starting up for activation during shutdown

Activation is prone to start failures due to browser shutdown, since
it's triggered when tabs close. This patch:
- Delays activation for a second, when triggered by tab close
- Tries to detect when shutdown happened when activation fails

BUG=600226
Committed: https://crrev.com/6daa334159472055f2d967e17cc8de3bc98ff232
Cr-Commit-Position: refs/heads/master@{#397649}

[falken, 2016-05-31 10:15:32]

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

[falken, 2016-05-31 10:15:33]

nhiroki: can you review of this?

[nhiroki, 2016-06-01 05:51:48]

https://codereview.chromium.org/2027583002/diff/40001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/2027583002/diff/40001/base/compiler_specific....
base/compiler_specific.h:58: #define NON_EXPORTED_BASE(...) \
Why do we need this change?

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1366:
runner->RunPendingTasks();
Just to confirm: We call Run*Pending*Tasks() here, but Run*Until*Idle() at line
1267. Is this difference intended?

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:296: // failures,
wait a bit before continuing.
Let me add this comment just for future reference...

[Before offline chat w/ falken@]

When delay == true, how about checking shutdown state first and avoiding
activation when in shutdown instead of using a timer?

[After offline chat]

According to falken@, ServiceWorkerContextWrapper::Shutdown() seems to be called
after this point and we may not be able to see the precise shutdown state here.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:363:
timer_.SetTaskRunner(task_runner);
std::move(task_runner) ?

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:402:
CompleteActivation(activating_version, status, false);
Always recording OK w/ |is_shutdown = false| would be misleading. How about
checking the shutdown state even when an activation succeeds, or skipping
recording OK when not in shutdown?

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:178: const
scoped_refptr<ServiceWorkerVersion>& activating_version);
[Optional] As you might already know, passing "const scoped_refptr<>&" is not
encouraged any more:
http://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-about-pa...

I know other places in this file have been still using this idiom, but it'd be
nice if we could adopt the new idiom one by one.

[falken, 2016-06-01 09:20:05]

Updated

https://codereview.chromium.org/2027583002/diff/40001/base/compiler_specific.h
File base/compiler_specific.h (right):

https://codereview.chromium.org/2027583002/diff/40001/base/compiler_specific....
base/compiler_specific.h:58: #define NON_EXPORTED_BASE(...) \
On 2016/06/01 05:51:47, nhiroki wrote:
> Why do we need this change?

For some reason ServiceWorkerRegistration had to become RefCountedThreadSafe
after it started using the timer. It must use DeleteOnIOThread or some crashes
happen. That means it went from inheriting:
base::RefCounted<ServiceWorkerRegistration>
to:
base::RefCountedThreadSafe<ServiceWorkerRegistration,
BrowserThread::DeleteOnIOThread> 

That comma destroys the NON_EXPORTED_BASE macro since it thinks it's two
arguments.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1366:
runner->RunPendingTasks();
On 2016/06/01 05:51:47, nhiroki wrote:
> Just to confirm: We call Run*Pending*Tasks() here, but Run*Until*Idle() at
line
> 1267. Is this difference intended?

Good catch. Unintentional. RunUntilIdle seems the way to go, since
RunPendingTasks doesn't run tasks posted by tasks.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:296: // failures,
wait a bit before continuing.
On 2016/06/01 05:51:47, nhiroki wrote:
> Let me add this comment just for future reference...
> 
> [Before offline chat w/ falken@]
> 
> When delay == true, how about checking shutdown state first and avoiding
> activation when in shutdown instead of using a timer?
> 
> [After offline chat]
> 
> According to falken@, ServiceWorkerContextWrapper::Shutdown() seems to be
called
> after this point and we may not be able to see the precise shutdown state
here.

Acknowledged.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:363:
timer_.SetTaskRunner(task_runner);
On 2016/06/01 05:51:47, nhiroki wrote:
> std::move(task_runner) ?

Done.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:402:
CompleteActivation(activating_version, status, false);
On 2016/06/01 05:51:47, nhiroki wrote:
> Always recording OK w/ |is_shutdown = false| would be misleading. How about
> checking the shutdown state even when an activation succeeds, or skipping
> recording OK when not in shutdown?

OK I went with checking the state always. I had wanted to make the shutdown
stuff be the slow path only taken when needed (e.g., the 1 sec delay is only
when a tab closes, and this UI hop was only on failure). But you're right it's
more accurate UMA to get the shutdown state in all cases, and the UI hop
shouldn't be too costly.

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:178: const
scoped_refptr<ServiceWorkerVersion>& activating_version);
On 2016/06/01 05:51:48, nhiroki wrote:
> [Optional] As you might already know, passing "const scoped_refptr<>&" is not
> encouraged any more:
>
http://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-about-pa...
> 
> I know other places in this file have been still using this idiom, but it'd be
> nice if we could adopt the new idiom one by one.

I changed to use scoped_refptr and std::move when it looked safe. For
base::Bind, do you use std::move?

[falken, 2016-06-01 09:25:50]

falken@chromium.org changed reviewers:
+ danakj@chromium.org, mpearson@chromium.org

[falken, 2016-06-01 09:25:50]

danakj: can you review base/?
mpearson: can you review histograms?

[danakj, 2016-06-01 17:57:18]

On 2016/06/01 09:25:50, falken wrote:
> danakj: can you review base/?

Can you explain why you're changing that?

> mpearson: can you review histograms?

[Mark P, 2016-06-01 21:10:42]

https://codereview.chromium.org/2027583002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2027583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:96701: +  <affected-histogram
name="ServiceWorker.ActivateEventStatus"/>
Please revise the histogram description to cover these new suffixes.  The
current description is "The result of dispatching the activate event to the
worker."  For the suffixes, this is clearly not still true.  Does _every_ result
of a dispatch event get sent to one of those two suffixed histograms?  If so,
then you can revise it by adding the following sentence:  "This is recorded to
both ServiceWorker.ActivateEventStatus and whichever of
ServiceWorker.ActivateEventStatus_InShutdown and
ServiceWorker.ActivateEventStatus_NotInShutdown is appropriate."  If not, then
you'll have to revise the description yourself.

[falken, 2016-06-01 22:50:01]

On 2016/06/01 17:57:18, danakj wrote:
> On 2016/06/01 09:25:50, falken wrote:
> > danakj: can you review base/?
> 
> Can you explain why you're changing that?

Yes. Sorry, I should have pointed to the discussion above:
"For some reason ServiceWorkerRegistration had to become RefCountedThreadSafe
after it started using the timer. It must use DeleteOnIOThread or some crashes
happen. That means it went from inheriting:
base::RefCounted<ServiceWorkerRegistration>
to:
base::RefCountedThreadSafe<ServiceWorkerRegistration,
BrowserThread::DeleteOnIOThread> 

That comma destroys the NON_EXPORTED_BASE macro since it thinks it's two
arguments."

> 
> > mpearson: can you review histograms?

[danakj, 2016-06-01 23:01:49]

On 2016/06/01 22:50:01, falken wrote:
> On 2016/06/01 17:57:18, danakj wrote:
> > On 2016/06/01 09:25:50, falken wrote:
> > > danakj: can you review base/?
> > 
> > Can you explain why you're changing that?
> 
> Yes. Sorry, I should have pointed to the discussion above:
> "For some reason ServiceWorkerRegistration had to become RefCountedThreadSafe
> after it started using the timer. It must use DeleteOnIOThread or some crashes
> happen. That means it went from inheriting:
> base::RefCounted<ServiceWorkerRegistration>
> to:
> base::RefCountedThreadSafe<ServiceWorkerRegistration,
> BrowserThread::DeleteOnIOThread> 

Uhm, but ServiceWorkerRegistration is not thread-safe. The destructor will
DCHECK that it's on the right thread. So that doesn't seem like a reasonable
change to make here, esp without understanding the reasons for it well.

> 
> That comma destroys the NON_EXPORTED_BASE macro since it thinks it's two
> arguments."

I see thanks. The base change is fine, but the rest should be concerning.

> 
> > 
> > > mpearson: can you review histograms?

[falken, 2016-06-02 07:12:33]

I (with nhiroki's help) investigated the thread safety issue more. 

It looks like callbacks posted to the Timer can outlive the IO thread even if
they're posted on the IO thread. At a later point during shutdown, they are
destroyed. In SWRegistration's case, the dtor complains because the
CurrentlyOn(IO) check fails. The DCHECK prints: "Must be called on
Chrome_IOThread; actually called on Chrome_IOThread." which probably means IO
thread is no longer alive.

I also tried using PostDelayedTask instead of Timer (suspecting the ref count
cycle with the Timer was keeping the registration alive too long), but the same
issue occurred. Here are some stack traces:
https://docs.google.com/document/d/1r7VHLcjmcyBn21eb82IB1OSN61h24HID18jivBS3A...

The Threading guidelines
<https://www.chromium.org/developers/design-documents/threading> suggest using
RefCountedThreadSafe with DeleteOnThread to ensure the dtor runs on a specific
thread ("If your object has a non-trivial destructor that needs to run on a
specific thread, you can use the following trait"). This way the dtor would be
skipped on shutdown:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

Another possibility is to use WeakPtr so the callback doesn't keep the
registration alive, but then the callback would never run in many cases since
the reference count goes to 0. That's not too terrible, since the next time the
service worker is needed it would continue activating, but it'd be preferable
for activation to finish before that.

https://codereview.chromium.org/2027583002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2027583002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:96701: +  <affected-histogram
name="ServiceWorker.ActivateEventStatus"/>
On 2016/06/01 21:10:42, Mark P wrote:
> Please revise the histogram description to cover these new suffixes.  The
> current description is "The result of dispatching the activate event to the
> worker."  For the suffixes, this is clearly not still true.  Does _every_
result
> of a dispatch event get sent to one of those two suffixed histograms?  If so,
> then you can revise it by adding the following sentence:  "This is recorded to
> both ServiceWorker.ActivateEventStatus and whichever of
> ServiceWorker.ActivateEventStatus_InShutdown and
> ServiceWorker.ActivateEventStatus_NotInShutdown is appropriate."  If not, then
> you'll have to revise the description yourself.

Done.

[kinuko, 2016-06-02 09:27:48]

Maybe just use WeakPtr and let someone else (e.g. service_worker_context) hold
references while waiting to activate?

(Also: querying shutdown status on UI thread only for histogram feels slightly
overkill, it might be fine just using lock-protected small flag for the
purpose...)

[falken, 2016-06-02 09:42:57]

On 2016/06/02 09:27:48, kinuko wrote:
> Maybe just use WeakPtr and let someone else (e.g. service_worker_context) hold
> references while waiting to activate?
> 
> (Also: querying shutdown status on UI thread only for histogram feels slightly
> overkill, it might be fine just using lock-protected small flag for the
> purpose...)

Will try some WeakPtr approaches. For future reference, the pending tasks
outliving the thread seem by design. Thread destruction is:
(1) Thread::CleanUp
(2) MessageLoop::~MessageLoop

Thread::Cleanup sets globals.threads[IO] to null, then later the message loop
destroys pending tasks, and DCHECK_CURRENTLY_ON(IO) cries because
global.threads[IO] is null.

[falken, 2016-06-02 12:17:23]

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

[falken, 2016-06-02 12:17:24]

Updated with new approach. This uses ref counting with Bind as usual, and the
dtor CURRENTLY_ON check is just revised to detect when the IO thread is dead.

It also uses uses a lock in SWProcessManager, which simplifies things.

I tried WeakPtr with outside ref counting, but it got complex: SWContext must be
told when to protect and release a registration, and all the callbacks involved
in activation (the 1 sec delay, the start worker callbacks) should be moved to
weak ptr or else dtor CURRENTLY_ON check would fail if shutdown happened with
those still alive.

[Mark P, 2016-06-02 17:05:38]

histograms.xml lgtm

[danakj, 2016-06-02 17:57:46]

On Thu, Jun 2, 2016 at 2:42 AM, <falken@chromium.org> wrote:

> On 2016/06/02 09:27:48, kinuko wrote:
> > Maybe just use WeakPtr and let someone else (e.g.
> service_worker_context) hold
> > references while waiting to activate?
> >
> > (Also: querying shutdown status on UI thread only for histogram feels
> slightly
> > overkill, it might be fine just using lock-protected small flag for the
> > purpose...)
>
> Will try some WeakPtr approaches. For future reference, the pending tasks
> outliving the thread seem by design. Thread destruction is:
> (1) Thread::CleanUp
> (2) MessageLoop::~MessageLoop
>
> Thread::Cleanup sets globals.threads[IO] to null, then later the message
> loop
> destroys pending tasks, and DCHECK_CURRENTLY_ON(IO) cries because
> global.threads[IO] is null.
>

Thank you for investigating.

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

[nhiroki, 2016-06-03 00:54:30]

lgtm

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/2027583002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_registration.h:178: const
scoped_refptr<ServiceWorkerVersion>& activating_version);
On 2016/06/01 09:20:05, falken wrote:
> On 2016/06/01 05:51:48, nhiroki wrote:
> > [Optional] As you might already know, passing "const scoped_refptr<>&" is
not
> > encouraged any more:
> >
>
http://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-about-pa...
> > 
> > I know other places in this file have been still using this idiom, but it'd
be
> > nice if we could adopt the new idiom one by one.
> 
> I changed to use scoped_refptr and std::move when it looked safe. For
> base::Bind, do you use std::move?

I use std::move if an object is not used anymore after base::Bind.

[falken, 2016-06-03 01:43:38]

Patchset #9 (id:160001) has been deleted

[falken, 2016-06-03 01:47:15]

nhiroki: I added a new unit test. Can you review it please?

[nhiroki, 2016-06-03 02:14:05]

lgtm

[falken, 2016-06-03 02:15:34]

The CQ bit was checked by falken@chromium.org

[falken, 2016-06-03 02:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2027583002/#ps180001
(title: "add unittest")

[commit-bot: I haz the power, 2016-06-03 02:16:42]

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

[commit-bot: I haz the power, 2016-06-03 06:40:48]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-06-03 06:43:19]

Description was changed from

==========
service worker: Avoid starting up for activation during shutdown

Activation is prone to start failures due to browser shutdown, since
it's triggered when tabs close. This patch:
- Delays activation for a second, when triggered by tab close
- Tries to detect when shutdown happened when activation fails

BUG=600226
==========

to

==========
service worker: Avoid starting up for activation during shutdown

Activation is prone to start failures due to browser shutdown, since
it's triggered when tabs close. This patch:
- Delays activation for a second, when triggered by tab close
- Tries to detect when shutdown happened when activation fails

BUG=600226

Committed: https://crrev.com/6daa334159472055f2d967e17cc8de3bc98ff232
Cr-Commit-Position: refs/heads/master@{#397649}
==========

[commit-bot: I haz the power, 2016-06-03 06:43:21]

Patchset 9 (id:??) landed as
https://crrev.com/6daa334159472055f2d967e17cc8de3bc98ff232
Cr-Commit-Position: refs/heads/master@{#397649}