Service Worker: Make ServiceWorkerRegistration.update() return a promise. (Blink 1/3)

Service Worker: Make ServiceWorkerRegistration.update() return a promise. (Blink 1/3)

As per the resolution of f2f, ServiceWorkerRegistration.update() should return a
promise that transforms the promise returned by Update algorithm.

In this CL, ServiceWorkerContextCore::UpdateServiceWorker method has been
overloaded to cover two invocation paths: a scheduled update without a
provider_host and a callback (Soft Update in the spec) and a call initiated from
the script surface using ServiceWorkerRegistration.update().

This is a web-exposed API change. But it has no compatibility risk because 
existing user code doesn't expect to receive anything from update().

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-registration-update
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/311

Companion CL (Chromium): https://codereview.chromium.org/1270513002/
Companion CL (Blink layout test): https://codereview.chromium.org/1268663003/

BUG=513655
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199814

[jungkees, 2015-07-30 11:26:42]

jungkee.song@samsung.com changed reviewers:
+ jinho.bang@samsung.com, l.gombos@samsung.com

[jungkees, 2015-07-30 11:33:52]

As per the f2f discussion, it's been decided to make registration.update()
return a promise resolves with undefined. This CL's addressing this change.
PTAL.

[zino, 2015-07-31 00:38:51]

On 2015/07/30 11:33:52, jungkees wrote:
> As per the f2f discussion, it's been decided to make registration.update()
> return a promise resolves with undefined. This CL's addressing this change.
> PTAL.

I think you should keep the existing |update()| method for now.
You can just add a overloaded version of the method and then remove the old
method later.

Please see:
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...

[jungkees, 2015-07-31 05:43:26]

zino@, thanks for the review and the discussion. As per our discussion,
overloading the IDL methods seems not possible as only the return types are
different. So, we came up with an idea like the following:

1. Commit Blink (WebServiceWorkerRegistration.h)
  virtual void update(WebServiceWorkerProvider*) { }
  virtual void update(WebServiceWorkerProvider* provider,
WebServiceWorkerUpdateCallbacks*) { this->update(provider); }
- Leave the existing void update(p) so it compiles with the existing chromium
code base.
- Call this->update(provider) from within update(p, c) so the execution path can
follow the existing chromium code path.

2. Commit Chromium
- The chromium-side patch lands. It still can work with the patch landed in #1
as it overrides the update(p, c).

3. Commit Blink layout test patch
- In this patch, we'll also change WebServiceWorkerRegistration.h file as below:
(-) virtual void update(WebServiceWorkerProvider*) { }
(-) virtual void update(WebServiceWorkerProvider* provider,
WebServiceWorkerUpdateCallbacks*) { this->update(provider); }
(+) virtual void update(WebServiceWorkerProvider*,
WebServiceWorkerUpdateCallbacks*) { }

SW OWNERs, PTAL and let me know if there's any better way to do it.

[jungkees, 2015-07-31 05:45:04]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, michaeln@chromium.org,
nhiroki@chromium.org

[jungkees, 2015-07-31 05:56:40]

The CQ bit was checked by jungkee.song@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-31 05:57:00]

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

[commit-bot: I haz the power, 2015-07-31 05:57:03]

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

[commit-bot: I haz the power, 2015-07-31 05:57:03]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[nhiroki, 2015-07-31 06:56:32]

https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
File public/platform/WebServiceWorkerRegistration.h (right):

https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
public/platform/WebServiceWorkerRegistration.h:36: virtual void
update(WebServiceWorkerProvider* provider, WebServiceWorkerUpdateCallbacks*) {
this->update(provider); }
How do you switch implementation? IIUC, the next Chromium patch will break
"update(WebServiceWorkerProvider*)" version and layout tests will fail?

[nhiroki, 2015-07-31 07:02:58]

https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
File public/platform/WebServiceWorkerRegistration.h (right):

https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
public/platform/WebServiceWorkerRegistration.h:36: virtual void
update(WebServiceWorkerProvider* provider, WebServiceWorkerUpdateCallbacks*) {
this->update(provider); }
On 2015/07/31 06:56:32, nhiroki wrote:
> How do you switch implementation? IIUC, the next Chromium patch will break
> "update(WebServiceWorkerProvider*)" version and layout tests will fail?

We could temporary disable tests to be broken in this CL and re-enable them in
the 3rd CL.

[zino, 2015-07-31 07:07:37]

On 2015/07/31 06:56:32, nhiroki wrote:
>
https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
> File public/platform/WebServiceWorkerRegistration.h (right):
> 
>
https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
> public/platform/WebServiceWorkerRegistration.h:36: virtual void
> update(WebServiceWorkerProvider* provider, WebServiceWorkerUpdateCallbacks*) {
> this->update(provider); }
> How do you switch implementation? IIUC, the next Chromium patch will break
> "update(WebServiceWorkerProvider*)" version and layout tests will fail?

Hi nhiroki@,

I'm not sure but I don't think the layout test will fail.

After applying the chromium side patch, overrided method in chromium will be
called.

The update() method return value is changed but it seems to be okay because
there is no tests to use its promise.

Thank you.

[nhiroki (google), 2015-07-31 07:17:11]

On 2015/07/31 07:07:37, zino wrote:
> On 2015/07/31 06:56:32, nhiroki wrote:
> >
>
https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
> > File public/platform/WebServiceWorkerRegistration.h (right):
> > 
> >
>
https://codereview.chromium.org/1267703003/diff/20001/public/platform/WebServ...
> > public/platform/WebServiceWorkerRegistration.h:36: virtual void
> > update(WebServiceWorkerProvider* provider, WebServiceWorkerUpdateCallbacks*)
{
> > this->update(provider); }
> > How do you switch implementation? IIUC, the next Chromium patch will break
> > "update(WebServiceWorkerProvider*)" version and layout tests will fail?
> 
> Hi nhiroki@,
> 
> I'm not sure but I don't think the layout test will fail.
> 
> After applying the chromium side patch, overrided method in chromium will be
> called.
> 
> The update() method return value is changed but it seems to be okay because
> there is no tests to use its promise.
> 
> Thank you.

Ah, I got it! That's smart :)

Thank you for your explanation.

[jungkees, 2015-07-31 07:21:37]

Yes, right. The scripts will get the promise but just won't use it for that
instances. :-) And the platform behavior is not affected by this change, but it
only informs Blink and the script surface the moment it's updated. (and the
failures if they failed.)

[nhiroki, 2015-07-31 07:25:42]

LGTM!

[jungkees, 2015-07-31 07:26:35]

The CQ bit was checked by jungkee.song@samsung.com

[nhiroki, 2015-07-31 07:26:59]

(and sorry, I missed msg#4)

[commit-bot: I haz the power, 2015-07-31 07:27:05]

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

[zino, 2015-07-31 07:30:18]

I've just checked there was no regression after applying 1st(blink) and
2rd(chromium) patches.

So, non-owner lgtm.

Thank you.

[commit-bot: I haz the power, 2015-07-31 07:32:25]

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

[commit-bot: I haz the power, 2015-07-31 07:32:26]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[zino, 2015-07-31 07:33:14]

jinho.bang@samsung.com changed reviewers:
+ tkent@chromium.org

[zino, 2015-07-31 07:33:38]

On 2015/07/31 07:32:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

+ tkent@ for public/*

[tkent, 2015-07-31 08:16:43]

This is a web-exposed API change.  So, an API owner should review.

This change is trivial, and has no compatibility risk because existing user code
doesn't expect to receive anything from update().

lgtm.

[jungkees, 2015-07-31 10:12:55]

Thanks for review tkent@. I updated the CL description with your comment.

[jungkees, 2015-07-31 10:13:01]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2015-07-31 10:13:16]

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

[commit-bot: I haz the power, 2015-07-31 10:16:30]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199814

[johnme, 2015-07-31 13:25:55]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1264143002/ by johnme@chromium.org.

The reason for reverting is: Sorry, this seems to cause
http/tests/serviceworker/update.html to fail consistently on WebKit Linux Leak
and WebKit Linux Oilpan Leak with the following error:

(leak detected: ({"numberOfLiveActiveDOMObjects":[2,3]}))

See flakiness dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=htt...

And example failed build:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/bu....

[jungkees, 2015-07-31 14:49:43]

johnme@, thanks for reporting this.

I carried out an investigation and just found the reason. This happens,  due to
the landing plan, only before the chromium-side patch
(https://codereview.chromium.org/1270513002/) will land. Now the chromium-side
patch is ready to commit with required lgtms. Can I try this again followed by
the chromium CL commit back to back?

Here's the analysis:

1 http/tests/serviceworker/update.html
2  V8ServiceWorkerRegistration::updateMethod
3    ServiceWorkerRegistration::update
4      ScriptPromiseResolver::create: <-- ++ActiveDOMObjects
5      WebServiceWorkerRegistration::update(provider,
callback(ScriptPromiseResolver))
6        WebServiceWorkerRegistration::update(provider) <-- Due to the landing
plan, it ignores the callback(ScriptPromiseResolver) before chromium patch
lands.

The ActiveDOMObjects count increased by #4 is supposed to be decreased when the
callback(ScriptPromiseResolver) is used and removed in
service_worker_dispatcher.cc after having an IPC message back from the browser
upon update completion. This expected scenario will work once the chromium-side
patch (https://codereview.chromium.org/1270513002/) lands.

/cc tkent@, nhiroki@

[johnme, 2015-07-31 17:07:52]

On 2015/07/31 14:49:43, jungkees wrote:
> Can I try this again followed by the chromium CL commit back to back?

It's not really possible to commit CLs back to back, since the blink
repository gets rolled periodically into the chromium repository, with
inevitable delay since the auto-roll bot needs to build and run tests.

If possible, it's best to do a 3-sided patch,
(chromium then blink then chromium, or blink then chromium then blink),
such that neither tree is ever in an inconsistent state.

But if that's impractical, then it's probably reasonable to re-land this,
wait for it to roll into chromium, then land the chromium patch, since
this only affects relatively minor bots (that hopefully won't close the
tree etc).

(This'll all become much easier once the blink repository gets merged
into the chromium repository!)

[zino, 2015-07-31 19:14:26]

jungkees@

Oh, I didn't think that far. I'm sorry.

I think you can fix it as follows simply but it is better to check locally
before re-landing.

"virtual void update(WebServiceWorkerProvider* provider,
WebServiceWorkerUpdateCallbacks* callback) { this->update(provider);
callback.onSuccess(); }"

[jungkees, 2015-08-01 02:29:03]

Thanks for the explanation johnme@. And I agree what zino@ suggested here.

So in order to try that, I think I need to upload a new snapshot with this
change and ask review to OWNERS again. Should I start a new CL for this? Or will
it work to do the update on this (closed) CL?