[ServiceWorker] Close the Mojo binding after reported a bad message
Changed by	leon.han@intel.com
Changed at	2018-02-06 10:17 PM (PST)
Repository	https://chromium.googlesource.com/chromium/src
Branch	
Revision	019349817400ef44b9fe4a984827dc06841b2f09
Comments
[ServiceWorker] Close the Mojo binding after reported a bad message

This is just a cleanup for ServiceWorkerVersion. As we do not expect
the connection error handler to do any extra work, we can just close the
Mojo binding of ServiceWorkerHost interface after reported a bad
message, rather than running the response callback with some nonsense
parameters.

BUG= 772793 

Change-Id: I42f5b29b6d4257ee958cb06b17277f0e0c85e8ab
Reviewed-on: https://chromium-review.googlesource.com/905752
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Han Leon <leon.han@intel.com>
Cr-Commit-Position: refs/heads/master@{#534933}
Changed files
content/browser/service_worker/service_worker_version.cc

The log shows an error:
[24577:24577:0207/014428.120287:INFO:CONSOLE(1)] "Uncaught TypeError: window.runEventTest is not a function", source: chrome-extension://bngalghedhkbhajkfnedbmdokmblalem/on_updated.html (1)

It seems unlikely that the change in c#1 caused these failures since non-service-worker tests look failing too. But we can revert and see if it fixes the flakiness, since it's just a clean-up patch for us.

I'd like to share some of my findings until now.

For ServiceWorkerTestWithNativeBindings/ServiceWorkerTest.Events/0, the error message is:
[20492:20492:0207/013931.398638:INFO:CONSOLE(1)] "Uncaught TypeError: window.runEventTest is not a function", source: chrome-extension://bngalghedhkbhajkfnedbmdokmblalem/on_updated.html (1)

For ServiceWorkerTestWithNativeBindings/ServiceWorkerTest.TabsCreate/0, the error message is:
[14807:14807:0207/171020.986071:INFO:CONSOLE(1)] "Uncaught TypeError: window.runServiceWorker is not a function", source: (1)

Seems the entry points window.run{Event,ServiceWorker} are not found at all.

Also, I checked around the extension tests (chrome/test/data/extensions/api_test/service_worker/{events,tabs_create}/*), AFAICT these javascript code could never trigger ServiceWorkerGlobalScope.Clients.claim(), thus could never come to the code touched by the above CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b08e796f0822bf727ece98328fc425cad7858f88

commit b08e796f0822bf727ece98328fc425cad7858f88
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Feb 08 03:40:24 2018

Revert "[ServiceWorker] Close the Mojo binding after reported a bad message"

This reverts commit 019349817400ef44b9fe4a984827dc06841b2f09.

Reason for revert: Suspected cause of  https://crbug.com/810227 

Original change's description:
> [ServiceWorker] Close the Mojo binding after reported a bad message
> 
> This is just a cleanup for ServiceWorkerVersion. As we do not expect
> the connection error handler to do any extra work, we can just close the
> Mojo binding of ServiceWorkerHost interface after reported a bad
> message, rather than running the response callback with some nonsense
> parameters.
> 
> BUG= 772793 
> 
> Change-Id: I42f5b29b6d4257ee958cb06b17277f0e0c85e8ab
> Reviewed-on: https://chromium-review.googlesource.com/905752
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Commit-Queue: Han Leon <leon.han@intel.com>
> Cr-Commit-Position: refs/heads/master@{#534933}

TBR=falken@chromium.org,kinuko@chromium.org,shimazu@chromium.org,leon.han@intel.com

Change-Id: Iba3679b7aef3d45c381dd2464c8937a732f10444
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  772793 ,  810227 
Reviewed-on: https://chromium-review.googlesource.com/907010
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535292}
[modify] https://crrev.com/b08e796f0822bf727ece98328fc425cad7858f88/content/browser/service_worker/service_worker_version.cc

Fair enough, it looks highly unlikely. I'll revert anyway to eliminate the CL as a cause.

stevenjb: are all these tests about extensions? Maybe:
https://chromium-review.googlesource.com/905405

I'm really not familiar with any of the code involved, I'm just the gardener :)

The tests are stll failing > #535292, so 019349817400ef44b9fe4a984827dc06841b2f09 does not appear to be the cause.

ServiceWorkerTestWithNativeBindings/ServiceWorkerTest.TabsCreate/0 was disbaled for  issue 810397  which may be the same root cause. I will keep an eye out for further ServiceWorkerTestWithNativeBindings/ServiceWorkerTest.Events/0 failures.

Haven't closely looked at it or reproduced it yet, adding some context:

The test loads page.html and is calling content::ExecuteScript("window.runEventTest") which is supposed to be defined on the page's script page.js

The issue that we're seeing "Uncaught TypeError: window.runEventTest is not a function" sounds like some kind of race.

The other failure test:  https://crbug.com/810397  seems to be the same underlying cause to me. That one shows "Uncaught TypeError: window.runServiceWorker is not a function".

@stevenjb Highly likely that this is due to some CL, not the tests themselves :(

I agree. I can get the test to semi-reliably time out locally. An initial pass suggests that started happening between #534226 - 534915, but the test runs are slow and it's flake so... I will see what I can do to bisect but it will take a while.

 Issue 811096  has been merged into this issue.

This also affects Linux, and a whole bunch of other ServiceWorkerTestWithNativeBindings tests:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=ServiceWorkerTestWithNativeBindings

List of flaky tests:
BackgroundPageIsWokenIfAsleep
Events
FetchArbitraryPaths
LoadingBackgroundPageBypassesServiceWorker
SWServedBackgroundPageReceivesEvent
ServiceWorkerPostsMessageToBackgroundClient
ServiceWorkerSuspensionOnExtensionUnload
RegisterSucceeds

Local testing suggests that this started (or at least got worse) at some point after master@{#534898}, but after that my bisect gets fuzzy (and I've had to switch my checkout to other things).

I'm setting up a parallely checkout to continue the bisect to try to help isolate this.

It looks like the tests are still flaky after the revert in c#4, which was expected.

Since these are extensions-based tests, re-assigning to lazyboy. Sounds like we could also dupe to  issue 810397 .

 Issue 812387  has been merged into this issue.

[worker triage] ping: did we nail down these flakes?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75ad1d95cb588f787a804ae21080beadeb054c7b

commit 75ad1d95cb588f787a804ae21080beadeb054c7b
Author: Istiaque Ahmed <lazyboy@chromium.org>
Date: Thu Aug 30 05:16:54 2018

[Extension SW]: Fix race conditions in tests.

ServiceWorkerTest* had race conditions around promises that
set up workers to run tests. It was possible for those workers
to start replying (postMessage) to extension background script
before the script was ready to receive them. Fix this by running
the promise a bit later in time, after background script is set
up correctly.

This CL fixes a known flakiness: ServiceWorkerTest.TabsCreate
and enables the test.

In addition to that, this CL also fixes similar potential race
conditions, throughout extension service worker tests.

Bug:  810397 ,  810227 
Change-Id: I0ecbd0e915113ca12e37d33b5fae097ccfac76aa
Reviewed-on: https://chromium-review.googlesource.com/967412
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587449}
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/events/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/events_to_stopped_extension/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/events_to_stopped_worker/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/notifications/has_permission/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/push_messaging/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/sync/page.js
[modify] https://crrev.com/75ad1d95cb588f787a804ae21080beadeb054c7b/chrome/test/data/extensions/api_test/service_worker/tabs_create/page.js

I don't see references to disabled tests with this bug number, so it looks fixed. Great work!