New issue
Advanced search Search tips

Issue 855852 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Remove EmbeddedWorkerInstance::Listener interface

Project Member Reported by leon....@intel.com, Jun 23 2018

Issue description

This comes from https://chromium-review.googlesource.com/c/chromium/src/+/1112904#message-1d234452a64ef3f33718abadc1c7ef94544766a2.

After the removal, we can let EmbeddedWorkerInstance talk directly to its owner ServiceWorkerVersion, then let ServiceWorkerRegisterJob observe the ServiceWorkerVersion it's manipulating to get OnScriptLoaded() notification.
 

Comment 1 by leon....@intel.com, Jun 27 2018

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 28 2018

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

commit c05aa20fa19e4f979f29afc9e4a8efe238356079
Author: Han Leon <leon.han@intel.com>
Date: Thu Jun 28 04:56:37 2018

[ServiceWorker] SWVersion::{Add,Remove}Listener -> {Add,Remove}Observer

This CL renames the interface ServiceWorkerVersion::Listener to
ServiceWorkerVersion::Observer and updates
ServiceWorkerVersion::{Add,Remove}Listener() to
ServiceWorkerVersion::{Add,Remove}Observer().

This renaming is in order to align with ScopedObserver impl which call
{Add,Remove}Observer() rather than {Add,Remove}Listener() on the source
class.
Thus it'll be easier for all observers of ServiceWorkerVersion to
leverage ScopedObserver.

BUG=855852

Change-Id: Id98e78fc90b96267bc68a69e18a731b71623779d
Reviewed-on: https://chromium-review.googlesource.com/1116522
Commit-Queue: Han Leon <leon.han@intel.com>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571029}
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_context_core.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_context_core.h
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_job_unittest.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_object_host.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_object_host.h
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_registration.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_registration.h
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_registration_unittest.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_storage.h
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/c05aa20fa19e4f979f29afc9e4a8efe238356079/content/browser/service_worker/service_worker_version_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 22

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

commit fd6ffa7060dcc92f59736d194141ab6571a5d1c3
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Aug 22 19:39:51 2018

service worker: Use a callback instead of an observer for pause after download.

The observer model was a source of crashes since it had tricky lifetime
as the ServiceWorkerRegisterJob keeps a reference to
ServiceWorkerVersion but tries to observe its EmbeddedWorkerInstance
which can be reset to another instance at any time. We tried to mitigate
it before with OnWillBeDestroyed() but it wasn't sufficient.

Just use a callback instead.

Bug: 876541, 855852
Change-Id: Ic3546716108fa0e7af1e3fada8ca79ec32846a8a
TBR: kinuko
Reviewed-on: https://chromium-review.googlesource.com/1184133
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585202}
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_new_script_loader_unittest.cc
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_register_job.h
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
[modify] https://crrev.com/fd6ffa7060dcc92f59736d194141ab6571a5d1c3/content/common/service_worker/embedded_worker.mojom

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 27

Labels: merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acc43a37c1c73fd651626d12875f2d58d733f4a1

commit acc43a37c1c73fd651626d12875f2d58d733f4a1
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Aug 27 20:46:56 2018

M69: service worker: Use a callback instead of an observer for pause after download.

The observer model was a source of crashes since it had tricky lifetime
as the ServiceWorkerRegisterJob keeps a reference to
ServiceWorkerVersion but tries to observe its EmbeddedWorkerInstance
which can be reset to another instance at any time. We tried to mitigate
it before with OnWillBeDestroyed() but it wasn't sufficient.

Just use a callback instead.

Bug: 876541, 855852
Change-Id: Ic3546716108fa0e7af1e3fada8ca79ec32846a8a
TBR: kinuko
Reviewed-on: https://chromium-review.googlesource.com/1184133
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#585202}(cherry picked from commit fd6ffa7060dcc92f59736d194141ab6571a5d1c3)
Reviewed-on: https://chromium-review.googlesource.com/1191882
Cr-Commit-Position: refs/branch-heads/3497@{#821}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/embedded_worker_instance.h
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_new_script_loader_unittest.cc
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_register_job.cc
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_register_job.h
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_version.cc
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_version.h
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
[modify] https://crrev.com/acc43a37c1c73fd651626d12875f2d58d733f4a1/content/common/service_worker/embedded_worker.mojom

Sign in to add a comment