service worker: use 200 OK for update requests even in the no update case

service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
Committed: https://crrev.com/42bc9253c79fa01fc6a6af5b3908dcc063a12023
Cr-Commit-Position: refs/heads/master@{#376032}

[falken, 2016-02-05 13:06:29]

Description was changed from

==========
service worker: move "identical with incumbent" status for update attempts out
of the network system

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

to

==========
service worker: use 200 OK for update requests even if the worker is identical
to the incumbent

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

[falken, 2016-02-05 13:07:23]

Description was changed from

==========
service worker: use 200 OK for update requests even if the worker is identical
to the incumbent

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

to

==========
service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

[falken, 2016-02-05 13:08:48]

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

[falken, 2016-02-05 13:08:49]

This is ready for a look but not quite complete:
- I want to add an integration test
- PRESUBMIT complains about serveAsynchronousMockedRequests

[michaeln, 2016-02-06 01:13:59]

lgtm

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.h:117: bool
pause_after_download = false);
i think our style guide prohibits default param values, has that changed?

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:538: if
(new_version()) {
maybe put this block up top to defend against the addition of an early return at
some point in this method body

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right):

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:111:
EXPECT_CALL(*m_mockClient, createServiceWorkerProvider())
ok, i'm not too familiar with testing::Mock, the .Times(0) means
EXPECT_NOT_CALLED

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:142:
TEST_F(WebEmbeddedWorkerImplTest, Success)
DoNotPauseAfterDownload()?

[nhiroki, 2016-02-10 04:56:15]

LGTM

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:422: if (process_id()
== ChildProcessHost::kInvalidUniqueID)
We may also need to check |status_| like this:

if (process_id() == CPH::kInvalidUniqueID || status_ != STARTING)
  return;

because the worker in STOPPING state could have a valid |process_handle_|.

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:578: void
ServiceWorkerRegisterJob::OnScriptLoaded() {
Can you add "DCHECK(pause_after_download())" here?

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right):

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:147:
.WillOnce(::testing::Return(nullptr));
EXPECT_CALL(*m_mockClient, workerScriptLoaded()).Times(1) ?

[falken, 2016-02-10 05:40:41]

Thanks for the reviews. This patch is doing a lot in WebEmbeddedWorkerImplTest,
and the PRESUBMIT is failing there. I'm going to first solve that issue in
another patch.

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:422: if (process_id()
== ChildProcessHost::kInvalidUniqueID)
On 2016/02/10 04:56:14, nhiroki (slow) wrote:
> We may also need to check |status_| like this:
> 
> if (process_id() == CPH::kInvalidUniqueID || status_ != STARTING)
>   return;
> 
> because the worker in STOPPING state could have a valid |process_handle_|.

Good catch. Done.

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.h:117: bool
pause_after_download = false);
On 2016/02/06 01:13:58, michaeln wrote:
> i think our style guide prohibits default param values, has that changed?

AFAICT yes: https://google.github.io/styleguide/cppguide.html#Default_Arguments

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:538: if
(new_version()) {
On 2016/02/06 01:13:58, michaeln wrote:
> maybe put this block up top to defend against the addition of an early return
at
> some point in this method body

Done.

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:578: void
ServiceWorkerRegisterJob::OnScriptLoaded() {
On 2016/02/10 04:56:14, nhiroki (slow) wrote:
> Can you add "DCHECK(pause_after_download())" here?

It's not true though because pause_after_download may have been false when
StartWorker is called, if there's no incumbent.

To really do this DCHECK I'd need to add a bool "did_pause_after_download" to
register job and I'm not sure it's worth doing that.

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp (right):

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:111:
EXPECT_CALL(*m_mockClient, createServiceWorkerProvider())
On 2016/02/06 01:13:58, michaeln wrote:
> ok, i'm not too familiar with testing::Mock, the .Times(0) means
> EXPECT_NOT_CALLED

Yes, I was surprised too.

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:142:
TEST_F(WebEmbeddedWorkerImplTest, Success)
On 2016/02/06 01:13:58, michaeln wrote:
> DoNotPauseAfterDownload()?

Done.

https://codereview.chromium.org/1675613002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebEmbeddedWorkerImplTest.cpp:147:
.WillOnce(::testing::Return(nullptr));
On 2016/02/10 04:56:15, nhiroki (slow) wrote:
> EXPECT_CALL(*m_mockClient, workerScriptLoaded()).Times(1) ?

Redid these tests to be more thorough.

[falken, 2016-02-10 06:14:25]

Spun off https://codereview.chromium.org/1687803002/

[nhiroki, 2016-02-10 06:38:29]

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:578: void
ServiceWorkerRegisterJob::OnScriptLoaded() {
On 2016/02/10 05:40:40, falken wrote:
> On 2016/02/10 04:56:14, nhiroki (slow) wrote:
> > Can you add "DCHECK(pause_after_download())" here?
> 
> It's not true though because pause_after_download may have been false when
> StartWorker is called, if there's no incumbent.
> 
> To really do this DCHECK I'd need to add a bool "did_pause_after_download" to
> register job and I'm not sure it's worth doing that.

Hmmm... I'm still thinking this event handler can be called only when
|new_version()->pause_after_download()| is true. The handler is added to the
version only when the flag is set at line 347-348, and it's removed only when
the flag is unset at line 539-540.

If this assumption is not correct, an assertion in
WebEmbeddedWorkerImpl::resumeAfterDownload() seems to be failed.

 163 void WebEmbeddedWorkerImpl::resumeAfterDownload()
 164 {
 165     ASSERT(!m_askedToTerminate);
 166     ASSERT(m_pauseAfterDownloadState == IsPausedAfterDownload);
^^^ This assertion. In the case, m_pauseAfterDownloadState could be
DontPauseAfterDownload.

(sorry if I'm missing something...)

[falken, 2016-02-10 06:53:40]

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

https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_register_job.cc:578: void
ServiceWorkerRegisterJob::OnScriptLoaded() {
On 2016/02/10 06:38:29, nhiroki (slow) wrote:
> On 2016/02/10 05:40:40, falken wrote:
> > On 2016/02/10 04:56:14, nhiroki (slow) wrote:
> > > Can you add "DCHECK(pause_after_download())" here?
> > 
> > It's not true though because pause_after_download may have been false when
> > StartWorker is called, if there's no incumbent.
> > 
> > To really do this DCHECK I'd need to add a bool "did_pause_after_download"
to
> > register job and I'm not sure it's worth doing that.
> 
> Hmmm... I'm still thinking this event handler can be called only when
> |new_version()->pause_after_download()| is true. The handler is added to the
> version only when the flag is set at line 347-348, and it's removed only when
> the flag is unset at line 539-540.

Ah, of course. EmbeddedWorkerInstance::OnScriptLoaded is called but the register
job is not listening unless it was paused. Done, thanks!

[nhiroki, 2016-02-12 04:05:36]

On 2016/02/10 06:53:40, falken wrote:
>
https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_register_job.cc (right):
> 
>
https://codereview.chromium.org/1675613002/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_register_job.cc:578: void
> ServiceWorkerRegisterJob::OnScriptLoaded() {
> On 2016/02/10 06:38:29, nhiroki (slow) wrote:
> > On 2016/02/10 05:40:40, falken wrote:
> > > On 2016/02/10 04:56:14, nhiroki (slow) wrote:
> > > > Can you add "DCHECK(pause_after_download())" here?
> > > 
> > > It's not true though because pause_after_download may have been false when
> > > StartWorker is called, if there's no incumbent.
> > > 
> > > To really do this DCHECK I'd need to add a bool "did_pause_after_download"
> to
> > > register job and I'm not sure it's worth doing that.
> > 
> > Hmmm... I'm still thinking this event handler can be called only when
> > |new_version()->pause_after_download()| is true. The handler is added to the
> > version only when the flag is set at line 347-348, and it's removed only
when
> > the flag is unset at line 539-540.
> 
> Ah, of course. EmbeddedWorkerInstance::OnScriptLoaded is called but the
register
> job is not listening unless it was paused. Done, thanks!

LGTM

[falken, 2016-02-15 07:04:50]

PTAL. This patch has been updated after landing the intermediate patch
<https://codereview.chromium.org/1687803002/>.

The new code is in the delta against patchset #5: mostly it's additional
testing.

[nhiroki, 2016-02-15 09:11:04]

Looks good. Can you fix compile failures on win bots and memory leak on linux
asan bots?

https://codereview.chromium.org/1675613002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/1675613002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_write_to_cache_job.h:60: // script
is byte-by-byte identical to the incumbent script.
nit: s/byte-by-byte/byte-for-byte/

"byte-for-byte" would be preferable because other places (eg. comments in
ServiceWorkerRegisterJob::OnScriptLoaded) and the spec use "byte-for-byte".

[falken, 2016-02-17 04:13:08]

I'm pretty sure the memory leak is innocuous: the test runner just needs to
learn to terminate the workers and run GC for the worker isolate before ending,
see https://code.google.com/p/chromium/issues/detail?id=586897

I'm trying to fix that (need to talk to kouhei@), but don't want to block this
patch on it. So I'll continue with committing this.

[falken, 2016-02-17 04:17:24]

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

[falken, 2016-02-17 04:17:25]

tkent: can you review web/?

[tkent, 2016-02-17 04:46:51]

lgtm

[falken, 2016-02-17 05:32:38]

The CQ bit was checked by falken@chromium.org

[falken, 2016-02-17 05:32:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1675613002/#ps140001
(title: "asan and fix win compile?")

[commit-bot: I haz the power, 2016-02-17 05:34:53]

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

[commit-bot: I haz the power, 2016-02-17 05:46:12]

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

[commit-bot: I haz the power, 2016-02-17 05:46:13]

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

[falken, 2016-02-17 05:57:17]

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

[falken, 2016-02-17 05:57:18]

+tsepez: please review messages

[Tom Sepez, 2016-02-17 17:46:19]

Messages LGTM

[falken, 2016-02-17 23:31:56]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-02-17 23:33:47]

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

[commit-bot: I haz the power, 2016-02-17 23:40:27]

Description was changed from

==========
service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

to

==========
service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

[commit-bot: I haz the power, 2016-02-17 23:40:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-17 23:41:27]

Description was changed from

==========
service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797
==========

to

==========
service worker: use 200 OK for update requests even in the no update case

The main motivation is to stop network failure error spew in DevTools console.
This patch reinstates the "pause on download" functionality we once had.

BUG=541797

Committed: https://crrev.com/42bc9253c79fa01fc6a6af5b3908dcc063a12023
Cr-Commit-Position: refs/heads/master@{#376032}
==========

[commit-bot: I haz the power, 2016-02-17 23:41:28]

Patchset 8 (id:??) landed as
https://crrev.com/42bc9253c79fa01fc6a6af5b3908dcc063a12023
Cr-Commit-Position: refs/heads/master@{#376032}

[Nico, 2016-02-18 13:47:01]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1710903002/ by thakis@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20T...
got much more flaky after this landed. Speculatively revert and see if it helps.
See also http://crbug.com/586897.