Hi, dominickn@, many unit tests in InstallableManagerBrowserTest become flaky, this is one of them, did not find direct culprit CL, could you take a quick look of it?

 Issue 658698  has been merged into this issue.

 Issue 658697  has been merged into this issue.

 Issue 658694  has been merged into this issue.

 Issue 658693  has been merged into this issue.

 Issue 658692  has been merged into this issue.

"InstallableManagerBrowserTest.CheckNoManifest" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyOAsSBUZsYWtlIi1JbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja05vTWFuaWZlc3QM.

"InstallableManagerBrowserTest.CheckManifestCorruptedIcon" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyQwsSBUZsYWtlIjhJbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja01hbmlmZXN0Q29ycnVwdGVkSWNvbgw.


"InstallableManagerBrowserTest.CheckWebappInIframe" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 4 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyPAsSBUZsYWtlIjFJbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja1dlYmFwcEluSWZyYW1lDA.


"InstallableManagerBrowserTest.CheckInstallableParamsDefaultConstructor" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 4 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyUQsSBUZsYWtlIkZJbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja0luc3RhbGxhYmxlUGFyYW1zRGVmYXVsdENvbnN0cnVjdG9yDA.


"InstallableManagerBrowserTest.CheckManifestOnly" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyOgsSBUZsYWtlIi9JbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja01hbmlmZXN0T25seQw.

Looks like a crash related to mojo and ServiceWorker

[ RUN      ] InstallableManagerBrowserTest.CheckManifestOnly
[27278:27278:1023/230500:WARNING:chrome_browser_main_chromeos.cc(340)] Running as stub user with profile dir: test-user
[27278:27278:1023/230500:WARNING:audio_manager.cc(317)] Multiple instances of AudioManager detected
[27278:27278:1023/230500:WARNING:audio_manager.cc(278)] Multiple instances of AudioManager detected
[27278:27278:1023/230500:ERROR:logging_chrome.cc(173)] Unable to create symlink /tmp/.org.chromium.Chromium.RKTuUh/d0ztwgz/test-user/chrome_debug.log pointing at /tmp/.org.chromium.Chromium.RKTuUh/d0ztwgz/test-user/chrome_debug_20161023-230500.log: No such file or directory
Xlib:  extension "RANDR" missing on display ":9".
[27278:27310:1023/230500:ERROR:drive_integration_service.cc(120)] /tmp should have been created as clear.
[27278:27295:1023/230500:WARNING:local_extension_cache.cc(259)] Extensions will not be installed from update URLs until /tmp/.org.chromium.Chromium.RKTuUh/d0ztwgz/stub_device_local_extension_cache/.initialized exists.
[27278:27297:1023/230500:WARNING:freezer_cgroup_process_manager.cc(59)] Cgroup freezer does not exist or is not writable. Unable to freeze renderer processes.
[27278:27380:1023/230501:WARNING:simple_synchronous_entry.cc(1054)] Could not open platform files for entry.
[27278:27356:1023/230501:WARNING:embedded_test_server.cc(201)] Request not handled. Returning 404: /favicon.ico
[27312:27312:1023/230501:ERROR:gles2_cmd_decoder.cc(16763)] [.DisplayCompositor-0x1cf8ce7000]GL ERROR :GL_INVALID_OPERATION : glCreateAndConsumeTextureCHROMIUM: invalid mailbox name
[27312:27312:1023/230501:ERROR:gles2_cmd_decoder.cc(9395)] [.DisplayCompositor-0x1cf8ce7000]RENDER WARNING: texture bound to texture unit 0 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering.
[27312:27312:1023/230501:ERROR:gles2_cmd_decoder.cc(16763)] [.DisplayCompositor-0x1cf8ce7000]GL ERROR :GL_INVALID_OPERATION : glCreateAndConsumeTextureCHROMIUM: invalid mailbox name
[27312:27312:1023/230501:ERROR:gles2_cmd_decoder.cc(9395)] [.DisplayCompositor-0x1cf8ce7000]RENDER WARNING: texture bound to texture unit 0 is not renderable. It maybe non-power-of-2 and have incompatible texture filtering.
[27278:27278:1023/230502:WARNING:pref_notifier_impl.cc(23)] Pref observer found at shutdown.
[27278:27278:1023/230502:WARNING:pref_notifier_impl.cc(23)] Pref observer found at shutdown.
[27278:27278:1023/230502:WARNING:pref_notifier_impl.cc(23)] Pref observer found at shutdown.
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x000002f3fcde base::debug::StackTrace::StackTrace()
#1 0x00000355c5c9 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#2 0x7f6d6e9aa0b0 <unknown>
#3 0x000002962b21 content::ServiceWorkerVersion::OnStoppedInternal()
#4 0x0000028ecae3 content::EmbeddedWorkerInstance::OnDetached()
#5 0x000003f710ff mojo::InterfaceEndpointClient::NotifyError()
#6 0x000003f7693b mojo::internal::MultiplexRouter::ProcessNotifyErrorTask()
#7 0x000003f74ba6 mojo::internal::MultiplexRouter::ProcessTasks()
#8 0x000003f73981 mojo::internal::MultiplexRouter::OnPipeConnectionError()
#9 0x000003f6c807 mojo::Connector::HandleError()
#10 0x000003f6ce70 mojo::Connector::OnHandleReadyInternal()
#11 0x000003f7dda9 mojo::Watcher::OnHandleReady()
#12 0x000003f7de48 mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()
#13 0x000002f5f200 base::MessageLoop::~MessageLoop()
#14 0x000000921559 base::MessageLoopForUI::~MessageLoopForUI()
#15 0x000002fa441a base::Thread::ThreadMain()
#16 0x000002f9df0c base::(anonymous namespace)::ThreadFunc()
#17 0x7f6d73705e9a start_thread
#18 0x7f6d6ea6736d clone

I'm suspecting this change:


https://codereview.chromium.org/2436223004

Use the serviceworker with mojo by default

This patch changes --mojo-service-worker to --disable-mojo-service-worker.
Let's try mojo for the service worker.

BUG= 629701 
Committed: https://crrev.com/391fa63bcefd25b35addf678356828dded5ae71e
Cr-Commit-Position: refs/heads/master@{#427013}

Reverting: https://codereview.chromium.org/2443983002/

 Issue 658727  has been merged into this issue.

I'm still not sure the exact reason, but this seems to happen only on Release build and OnDetach looks called as the mojo error handler.

# Changed to P2 because currently the tests are green.

Detected 4 new flakes for test/step "AppBannerManagerBrowserTest.WebAppBannerInIFrame". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyOwsSBUZsYWtlIjBBcHBCYW5uZXJNYW5hZ2VyQnJvd3NlclRlc3QuV2ViQXBwQmFubmVySW5JRnJhbWUM. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).

Detected 6 new flakes for test/step "InstallableManagerBrowserTest.CheckInstallableParamsDefaultConstructor". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyUQsSBUZsYWtlIkZJbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja0luc3RhbGxhYmxlUGFyYW1zRGVmYXVsdENvbnN0cnVjdG9yDA. This message was posted automatically by the chromium-try-flakes app.

Detected 6 new flakes for test/step "InstallableManagerBrowserTest.CheckWebappInIframe". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyPAsSBUZsYWtlIjFJbnN0YWxsYWJsZU1hbmFnZXJCcm93c2VyVGVzdC5DaGVja1dlYmFwcEluSWZyYW1lDA. This message was posted automatically by the chromium-try-flakes app. Since flakiness is ongoing, the issue was moved back into Sheriff Bug Queue (unless already there).

Removing Sheriff-Chromium since I'm not seeing new failures on the tree

I got the log "crash_log.txt" while executing AppBannerManagerBrowserTest.WebAppBannerInIFrame test with 2436223004 and the attached patch "log_swversion.diff" only once.
(Unfortunately this crash is flaky and it is not easy to reproduce...)

According to the log EmbeddedWorkerInstance::OnDetached() is called twice.

#0 0x7f6abecce30e base::debug::StackTrace::StackTrace()
#1 0x7f6ab73e7fb5 content::ServiceWorkerVersion::OnStoppedInternal()
#2 0x7f6ab73e88f6 content::ServiceWorkerVersion::OnDetached()
#3 0x7f6ab724a205 content::EmbeddedWorkerInstance::OnDetached()
#4 0x7f6ab726239c content::EmbeddedWorkerRegistry::RemoveChildProcessSender()
#5 0x7f6ab730e9e1 content::ServiceWorkerDispatcherHost::OnFilterRemoved()
#6 0x7f6ab5e95dba content::BrowserMessageFilter::Internal::OnFilterRemoved()
#7 0x7f6ab9f8d1c3 IPC::ChannelProxy::Context::OnChannelClosed()


#0 0x7f6abecce30e base::debug::StackTrace::StackTrace()
#1 0x7f6ab73e7fb5 content::ServiceWorkerVersion::OnStoppedInternal()
#2 0x7f6ab73e88f6 content::ServiceWorkerVersion::OnDetached()
#3 0x7f6ab724a205 content::EmbeddedWorkerInstance::OnDetached()
#4 0x7f6ab7249c83 content::EmbeddedWorkerInstance::Detach()
#5 0x7f6ab5fa12c5 _ZN4base8internal13FunctorTraitsIMN5blink23WebIDBDatabaseCallbacksEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_
#6 0x7f6ab725d191 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN7content22EmbeddedWorkerInstanceEFvvEJPS5_EEEvOT_DpOT0_
#7 0x7f6ab725d137 _ZN4base8internal7InvokerINS0_9BindStateIMN7content22EmbeddedWorkerInstanceEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#8 0x7f6ab725d07c _ZN4base8internal7InvokerINS0_9BindStateIMN7content22EmbeddedWorkerInstanceEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#9 0x7f6abf40afdb base::internal::RunMixin<>::Run()
#10 0x7f6abf418f20 mojo::InterfaceEndpointClient::NotifyError()
#11 0x7f6abf42b616 mojo::internal::MultiplexRouter::ProcessNotifyErrorTask()
#12 0x7f6abf428f43 mojo::internal::MultiplexRouter::ProcessTasks()
#13 0x7f6abf427837 mojo::internal::MultiplexRouter::OnPipeConnectionError()
#14 0x7f6abf413bc5 _ZN4base8internal13FunctorTraitsIMN4mojo8internal19ControlMessageProxyEFvvEvE6InvokeIPS4_JEEEvS6_OT_DpOT0_
#15 0x7f6abf432a21 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN4mojo8internal15MultiplexRouterEFvvEJPS6_EEEvOT_DpOT0_
#16 0x7f6abf4329c7 _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo8internal15MultiplexRouterEFvvEJNS0_17UnretainedWrapperIS5_EEEEEFvvEE7RunImplIRKS7_RKSt5tupleIJS9_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#17 0x7f6abf43290c _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo8internal15MultiplexRouterEFvvEJNS0_17UnretainedWrapperIS5_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#18 0x7f6abf40afdb base::internal::RunMixin<>::Run()
#19 0x7f6abf408d87 mojo::Connector::HandleError()
#20 0x7f6abf409dc2 mojo::Connector::OnHandleReadyInternal()
#21 0x7f6abf409c8b mojo::Connector::OnWatcherHandleReady()
#22 0x7f6abf40c12c _ZN4base8internal13FunctorTraitsIMN4mojo9ConnectorEFvjEvE6InvokeIPS3_JjEEEvS5_OT_DpOT0_
#23 0x7f6abf40c036 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN4mojo9ConnectorEFvjEJPS5_jEEEvOT_DpOT0_
#24 0x7f6abf40bfc7 _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo9ConnectorEFvjEJNS0_17UnretainedWrapperIS4_EEEEEFvjEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEEOj
#25 0x7f6abf40becc _ZN4base8internal7InvokerINS0_9BindStateIMN4mojo9ConnectorEFvjEJNS0_17UnretainedWrapperIS4_EEEEEFvjEE3RunEPNS0_13BindStateBaseEOj
#26 0x7f6abf3c7c3f base::internal::RunMixin<>::Run()
#27 0x7f6abf3c7506 mojo::Watcher::OnHandleReady()
#28 0x7f6abf3c7fe5 mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()

Deleted: log_swversion.diff 1.2 KB

log_swversion.diff 1.2 KB Download

Deleted: crash_log.txt 19.3 KB

crash_log.txt 19.3 KB View Download

horo-san: Thanks a lot!

OnDetached is usually called only by IPC::ChannelProxy::Context::OnChannelClosed (First one), so there seems some corner case.

I'm now suspecting there might be a situation that ServiceWorkerVersion is still alive even after ServiceWorkerDispatcherHost is destructed (which happens just after the first OnDetached is called). If there is such situation, EmbeddedWorkerInstance::Detach might be called as an error handler of mojo.

Also,  Issue 604762  might be related to this issue.

yzshen: Is connection_error_handler possible to be called even after InterfacePtr::reset() is called? [1] is where I intended to discard the InterfacePtr, but the connection_error_handler (Detach() in this case) seems to be called according c#18.

[1] https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_worker_instance.cc?sq=package:chromium&dr=CSs&rcl=1477524050&l=892

Hmmm.... I'm still not sure why this happened.

1: On calling IPC::ChannelProxy::Context::OnChannelClosed(), InterfacePtr<EmbeddedWorkerInstanceClient>::reset() must be called because content::EmbeddedWorkerInstance::OnDetached() is called.
2: This means InterfacePtr gets destructed at the time.
3: InterfacePtr owns mojo::Watcher through InterfacePtrState, MultiplexRouter and Connector, so mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop should not be called.

RE #21:
connection error handler won't be called after InterfacePtr reset or destruction (otherwise, it is a bug that we should fix).

RE #22:
Could you please point me to the code related to "On calling IPC::ChannelProxy::Context::OnChannelClosed(), InterfacePtr<EmbeddedWorkerInstanceClient>::reset() must be called because content::EmbeddedWorkerInstance::OnDetached() is called."?

After that I could repro this and find the crash seems to happen around observer.OnRunningStateChanged():
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_version.cc?q=ServiceWorkerVersion::OnStoppedInternal&sq=package:chromium&dr=CSs&l=1844
but I haven't understand yet why OnDetach is called twice.

yzshen@:

Thanks, I got it.
InterfacePtr<>::reset is here:
https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_worker_instance.cc?sq=package:chromium&dr=CSs&rcl=1477941524&l=894
This is called by the following steps;

OnChannelClosed calls MessageFilter::OnFilterRemoved:
https://cs.chromium.org/chromium/src/ipc/ipc_channel_proxy.cc?sq=package:chromium&dr=CSs&rcl=1477941524&l=216

ServiceWorkerDispatcherHost implements the MessageFilter, and this calls EmbeddedWorkerRegistry::RemoveChildProcessSender:
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_dispatcher_host.cc?sq=package:chromium&dr=CSs&rcl=1477941524&l=129

RemoveChildProcessSender triggers EmbeddedWorkerInstance::OnDetached:
https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_worker_registry.cc?sq=package:chromium&dr=CSs&rcl=1477941524&l=208

failure.txt is a log when I could repro the failure with additional logs (see add_logs.diff). For comparison, I also attached success.txt which is a log when the test passed.
This shows the crash is happening when SWVersion hasn't been destructed yet. In this case, client_.is_bound() on EWInstance was true though content::EmbeddedWorkerInstance::ReleaseProcess() had been already called and this means InterfacePtr<>::reset() should be called.

I'm wondering who is the owner of SWVersion after SWContextCore is already destructed, and why client_.is_bound() is true though it's already reset. 

Deleted: failure.txt 8.3 KB

failure.txt 8.3 KB View Download

Deleted: add_logs.diff 5.3 KB

add_logs.diff 5.3 KB Download

Deleted: success.txt 7.2 KB

success.txt 7.2 KB View Download

failure_2.txt shows EWInstance::Start is called twice. This is the reason why OnDetach is called twice. 

Segmentation fault seems to be caused by invalid listners of ServiceWorkerVersion, so I'll take a look tomorrow.

Deleted: failure_2.txt 19.0 KB

failure_2.txt 19.0 KB View Download

Copied from failure_2.txt

#0 0x7fd1f020b1de base::debug::StackTrace::StackTrace()
#1 0x7fd1ecc78b14 content::ServiceWorkerVersion::OnStoppedInternal()
#2 0x7fd1ecbe7c53 content::EmbeddedWorkerInstance::OnDetached()
#3 0x7fd1ecbe7456 content::EmbeddedWorkerInstance::Detach()
#4 0x7fd1f06f603f mojo::InterfaceEndpointClient::NotifyError()
#5 0x7fd1f06fd22a mojo::internal::MultiplexRouter::ProcessNotifyErrorTask()
#6 0x7fd1f06fb1e4 mojo::internal::MultiplexRouter::ProcessTasks()
#7 0x7fd1f06f9d31 mojo::internal::MultiplexRouter::OnPipeConnectionError()
#8 0x7fd1f06efa5d mojo::Connector::HandleError()
#9 0x7fd1f06f0291 mojo::Connector::OnHandleReadyInternal()
#10 0x7fd1f06d3857 mojo::Watcher::OnHandleReady()
#11 0x7fd1f06d392b mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()
#12 0x7fd1f023cdf0 base::MessageLoop::~MessageLoop()
#13 0x7fd1f023a019 base::MessageLoopForUI::~MessageLoopForUI()
#14 0x7fd1f02a54fb base::Thread::ThreadMain()

It looks like that EmbeddedWorkerInstance::Detach() is called on UI thread. This must be called on IO thread. shimazu@, can you confirm the callback is invoked on the appropriate thread?

In addition, it'd be strange to run a callback to detach EmbeddedWorkerInstance during the dtor of MessageLoop. ServiceWorker backend should already be shut down before the dtor of MessageLoop. We might forget to clean up EmbeddedWorkerInstanceClientPtr during SW's shutdown sequence (maybe we should reset EmbeddedWorkerInstance::client_ on the dtor of EmbeddedWorkerInstance??)

Thank you, I didn't realize OnStoppedInternal ran on the UI thread.

I think ~EWInstance removes mojom::EmbeddedWorkerInstanceClientPtr client_ because it's a member. A problem here might be that the dtor of SWVersion isn't called when the MessageLoop gets destructed and SWContextCore has been already destructed. I'm now guessing that SWRegistration is still alive at the time. 

Don't know if it's related but I had a similar issue where ServiceWorkerRegistration destructor was called during shutdown after the message loop was destroyed. I ended up adding this to the dtor:

  // Can be false during shutdown, in which case the DCHECK_CURRENTLY_ON below
  // would cry.
  if (!BrowserThread::IsThreadInitialized(BrowserThread::IO))
    return;

falken: Thanks so much! This might be the cause of this crash. 

I guess this crash is happening as follows:

1. Starts registration of service workers. Writing to DB is pending.
2. Starts the termination procedure. IPC::ChannelProxy::Context::OnChannelClosed is called, and EWInstance::OnDetached is called and InterfacePtr<EWInstanceClient> is destructed.
3. Finish writing to DB (DidStoreRegistration is called). Activation procedure following the registration triggers SWVersion::StartWorker 
   https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_registration.cc?sq=package:chromium&dr=CSs&rcl=1478028288&l=348
4. EWInstance::Start creates a new InterfaceRequest bound to an InterfacePtr<EWInstanceClient> immediately with setting an error callback.
5. ServiceWorkerContextCore, SWHandles and SWRegistrations are getting destructed on the IO thread, but SWVersion is still alive at this time
6. MessageLoopObserver::WillDestroyCurrentMessageLoop triggers the error handler which is set at (4) on the UI thread. At the same time, SWRegistration might be deleted on the IO thread though the dtor of SWRegistration doesn't remove itself from SWVersion::listeners_ (because it's already in shutdown sequence).
7. When running SWVersion::OnStoppedInternal, this test crashes because the listeners_ is null.

If this is correct, this crash can be avoided by moving set_connection_error_handler from the EWInstance::Start to somewhere later. IPC is actually sent at EWInstance::SendMojoStartWorker, which is called after several PostTasks starting from EWInstance::Start, so I think SendMojoServiceWorker might be good location. 

Shutdown is pretty complex. Here are some hints:
- willchan's explanation: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/o05a009KHsQ/m8ly31W5pt4J
- MessageLoop and Thread destruction order https://codereview.chromium.org/2027583002#msg13

I'm thinking now it's a mistake for SWRegistration to give up on destruction and leave SWVersion with a dangling pointer. It should at least cleanup the pointers.

I think this recent patch https://codereview.chromium.org/2180253003 may have eliminated the need for the early return in SWRegistration's dtor.

Yep, I verified that DCHECK_CURRENTLY_ON(BrowserThread::IO) would pass even in the if (!BrowserThread::IsThreadInitialized(BrowserThread::IO)  branch. So we no longer need the early return.

Repro steps:
1) Make a tab controlled by a service worker
2) Edit that service worker so the next update would install a new version.
3) Open another tab controlled by the service worker.
4) Now you should have an active worker and a waiting worker
5) Ctrl+Shift+Q to quit the browser. ServiceWorkerRegistration destructor will be called during shutdown.

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

commit 667f50ff47ad7217f21b80369dde3698e4c758c9
Author: shimazu <shimazu@chromium.org>
Date: Wed Nov 02 07:54:29 2016

ServiceWorker: Let dtor of SWRegistration work in the shutdown sequence

Currently the destructor of SWRegistration just returns without any operations
during shutdown because DCHECK_CURRENTLY_ON(BrowserThread::IO) used not to work
when shutting down. This patch removes the early return because
DCHECK_CURRENTLY_ON is now working thanks to https://crrev.com/2180253003.
Once the dtor works, a pointer to SWRegistration owned by SWVersion::listeners_
will be removed appropreately.

Here is steps to repro:
1) Make a tab controlled by a service worker
2) Edit that service worker so the next update would install a new version.
3) Open another tab controlled by the service worker.
4) Now you should have an active worker and a waiting worker
5) Ctrl+Shift+Q to quit the browser. ServiceWorkerRegistration destructor will
   be called during shutdown.

BUG= 658702 

Review-Url: https://codereview.chromium.org/2469203002
Cr-Commit-Position: refs/heads/master@{#429234}

[modify] https://crrev.com/667f50ff47ad7217f21b80369dde3698e4c758c9/content/browser/service_worker/service_worker_registration.cc

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

commit be675544ee93b8f940230861a8a4ae1dbbf2ebc7
Author: shimazu <shimazu@chromium.org>
Date: Mon Nov 07 01:38:13 2016

ServiceWorker: Add a check if error handler runs on the right thread

As observation in  https://crbug.com/658702 , I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG= 658702 , 604762 

Review-Url: https://codereview.chromium.org/2467223002
Cr-Commit-Position: refs/heads/master@{#430208}

[modify] https://crrev.com/be675544ee93b8f940230861a8a4ae1dbbf2ebc7/content/browser/service_worker/embedded_worker_instance.cc

Not sure whether it has anything to do with your issue, but in case you didn't notice: if you bind a mojo interface pointer to a message pipe on thread X, you can only use/destruct this pointer on thread X (unless you unbind/reset it, then you can rebind it or destroy it on any thread). The error handler can only be set and is later invoked on the bound thread.

Now it looks all green! :)
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests&tests=InstallableManagerBrowserTest

yzshen@:
Thanks. 

> The error handler can only be set and is later invoked on the bound thread

That's also what I expected, but I think the error handler seems to be called on the UI thread when the message loop is destructed, though it's registered on the IO thread. The stack trace commented on c#27 (attached below) is the reason why I guessed so. I'm wondering why the handler is possible to be called directly from the destructor of MessageLoopForUI.

...Ah, I got it while I'm writing this comment! :)
As I mentioned at c#30, EWInstance::Start could be called while the termination procedure is running. This might cause that the error handler gets bound with the UI thread because the IO thread is terminating.

--
#3 0x7fd1ecbe7456 content::EmbeddedWorkerInstance::Detach()
#4 0x7fd1f06f603f mojo::InterfaceEndpointClient::NotifyError()
#5 0x7fd1f06fd22a mojo::internal::MultiplexRouter::ProcessNotifyErrorTask()
#6 0x7fd1f06fb1e4 mojo::internal::MultiplexRouter::ProcessTasks()
#7 0x7fd1f06f9d31 mojo::internal::MultiplexRouter::OnPipeConnectionError()
#8 0x7fd1f06efa5d mojo::Connector::HandleError()
#9 0x7fd1f06f0291 mojo::Connector::OnHandleReadyInternal()
#10 0x7fd1f06d3857 mojo::Watcher::OnHandleReady()
#11 0x7fd1f06d392b mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()
#12 0x7fd1f023cdf0 base::MessageLoop::~MessageLoop()
#13 0x7fd1f023a019 base::MessageLoopForUI::~MessageLoopForUI()
#14 0x7fd1f02a54fb base::Thread::ThreadMain()