Change RunAfterStartWorker to not start the worker if it is already running.

Change RunAfterStartWorker to not start the worker if it is already running.

Also swaps the order of the two arguments to match the convention of having
an error callback be the last argument.

BUG=570820
Committed: https://crrev.com/ce3eb37e37d28577d748d31a65e468cbf662ba9c
Cr-Commit-Position: refs/heads/master@{#370433}

[Marijn Kruisselbrink, 2016-01-15 23:49:15]

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

[Marijn Kruisselbrink, 2016-01-15 23:49:15]

Addresses the two issues with RunAfterStartWorker pointed out in
https://codereview.chromium.org/1575283003

[johnme, 2016-01-18 19:06:51]

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

[johnme, 2016-01-18 19:06:51]

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

https://codereview.chromium.org/1584653009/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:512: RunSoon(task);
Using RunSoon (and hence PostTask) introduces a race condition: the SW might
currently be running, but stop before the task runs. Instead, how about:

void ServiceWorkerVersion::RunAfterStartWorker(
    const base::Closure& task,
    const StatusCallback& error_callback) {
  RunSoon(base::Bind(&ServiceWorkerVersion::RunAfterStartWorkerInternal, ...);
}
void ServiceWorkerVersion::RunAfterStartWorkerInternal(
    const base::Closure& task) {
  if (running_status() == RUNNING) {
    task.Run();
  } else {
    StartWorker(base::Bind(&RunTaskAfterStartWorker, ...));
  }
}

[nhiroki, 2016-01-19 01:36:39]

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

https://codereview.chromium.org/1584653009/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:512: RunSoon(task);
On 2016/01/18 19:06:51, johnme wrote:
> Using RunSoon (and hence PostTask) introduces a race condition: the SW might
> currently be running, but stop before the task runs. Instead, how about:
> 
> void ServiceWorkerVersion::RunAfterStartWorker(
>     const base::Closure& task,
>     const StatusCallback& error_callback) {
>   RunSoon(base::Bind(&ServiceWorkerVersion::RunAfterStartWorkerInternal, ...);
> }
> void ServiceWorkerVersion::RunAfterStartWorkerInternal(
>     const base::Closure& task) {
>   if (running_status() == RUNNING) {
>     task.Run();
>   } else {
>     StartWorker(base::Bind(&RunTaskAfterStartWorker, ...));
>   }
> }

Or, we could immediately run the callback when the worker is running.

void ServiceWorkerVersion::RunAfterStartWorker(
    const base::Closure& task,
    const StatusCallback& error_callback) {
  if (running_status() == RUNNING) {
    DCHECK(start_callbacks_.empty());
    task.Run();
    return;
  }
  StartWorker(...);
}

https://codereview.chromium.org/1584653009/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:516: error_callback,
task));
You may also want to swap arguments of RunTaskAfterStartWorker.

[Marijn Kruisselbrink, 2016-01-19 22:49:15]

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

https://codereview.chromium.org/1584653009/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:512: RunSoon(task);
On 2016/01/19 at 01:36:39, nhiroki wrote:
> On 2016/01/18 19:06:51, johnme wrote:
> > Using RunSoon (and hence PostTask) introduces a race condition: the SW might
> > currently be running, but stop before the task runs. Instead, how about:
> > 
> > void ServiceWorkerVersion::RunAfterStartWorker(
> >     const base::Closure& task,
> >     const StatusCallback& error_callback) {
> >   RunSoon(base::Bind(&ServiceWorkerVersion::RunAfterStartWorkerInternal,
...);
> > }
> > void ServiceWorkerVersion::RunAfterStartWorkerInternal(
> >     const base::Closure& task) {
> >   if (running_status() == RUNNING) {
> >     task.Run();
> >   } else {
> >     StartWorker(base::Bind(&RunTaskAfterStartWorker, ...));
> >   }
> > }
> 
> Or, we could immediately run the callback when the worker is running.
> 
> void ServiceWorkerVersion::RunAfterStartWorker(
>     const base::Closure& task,
>     const StatusCallback& error_callback) {
>   if (running_status() == RUNNING) {
>     DCHECK(start_callbacks_.empty());
>     task.Run();
>     return;
>   }
>   StartWorker(...);
> }

Normally I don't like methods that sometimes call a callback synchronously and
sometimes async (how I ended up with the RunSoon in the first place), but I
think in this case it might actually be the best option, so I've changed it to
that (and expanded the documentation for RunAfterStartWorker to make it clear
that is the behavior).

https://codereview.chromium.org/1584653009/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.cc:516: error_callback,
task));
On 2016/01/19 at 01:36:39, nhiroki wrote:
> You may also want to swap arguments of RunTaskAfterStartWorker.

I could, but I don't think it's worth it yet (given all the other use
RunTaskAfterStartWorker still has). But I'll definitely keep it in mind for the
cleanup after all events have been migrated.

[nhiroki, 2016-01-20 01:53:12]

LGTM. Do we need to merge this to M49?

[johnme, 2016-01-20 12:17:17]

lgtm, thanks :)

[johnme, 2016-01-20 12:17:17]

lgtm, thanks :)

[Marijn Kruisselbrink, 2016-01-20 17:38:54]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-01-20 17:39:09]

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

[Marijn Kruisselbrink, 2016-01-20 17:40:24]

On 2016/01/20 at 01:53:12, nhiroki wrote:
> LGTM. Do we need to merge this to M49?

I don't think we need to. Only three places call this method: geofencing,
navigator.connect and background sync. The first two aren't launched and
shouldn't be used by anybody (they don't really work anyway). Background sync is
launched in M49, and will be used by people, but the code there only calls
RunAfterStartWorker if the worker isn't already running, so isn't effected by
the change in behavior either.

[commit-bot: I haz the power, 2016-01-20 17:48:56]

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

[commit-bot: I haz the power, 2016-01-20 17:48:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marijn Kruisselbrink, 2016-01-20 17:53:12]

mek@chromium.org changed reviewers:
+ jkarlin@chromium.org

[Marijn Kruisselbrink, 2016-01-20 17:53:13]

+jkarlin for background_sync OWNERS

[jkarlin, 2016-01-20 18:01:42]

background_sync/ lgtm

[Marijn Kruisselbrink, 2016-01-20 18:02:44]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-01-20 18:04:45]

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

[commit-bot: I haz the power, 2016-01-20 18:38:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-20 18:39:32]

Description was changed from

==========
Change RunAfterStartWorker to not start the worker if it is already running.

Also swaps the order of the two arguments to match the convention of having
an error callback be the last argument.

BUG=570820
==========

to

==========
Change RunAfterStartWorker to not start the worker if it is already running.

Also swaps the order of the two arguments to match the convention of having
an error callback be the last argument.

BUG=570820

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

[commit-bot: I haz the power, 2016-01-20 18:39:33]

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