ServiceWorker: better error reporting for script streaming [chromium/src : master]
Makoto Shimazu (Gerrit)
Makoto Shimazu uploaded patch set #5 to this change.
ServiceWorker: better error reporting for script streaming
This patch is to pass a couple of content_browsertests for testing disk cache
errors. In the current implementation, a service worker is shutted down if the
disk cache is broken, and as a result, |start_worker_status_| in
ServiceWorkerVersion is recorded as SERVICE_WORKER_ERROR_ABORT. This patch is to
shutdown the worker on the browser side and remove the ServiceWorkerVersion.
It's the same behavior with ServiceWorkerReadFromCacheJob.
Test: content_browsertests -f --gtest_filter="ServiceWorkerVersionBrowserTest.ReadResourceFailure*" --enable-features=ServiceWorkerScriptStreaming
Bug: 683037, 754139
Change-Id: Ibde6743434ddd88bec46721decec0f67ab0698bf
---
M content/browser/service_worker/service_worker_installed_scripts_sender.cc
M content/renderer/service_worker/thread_safe_script_container.cc
M content/renderer/service_worker/thread_safe_script_container.h
M content/renderer/service_worker/thread_safe_script_container_unittest.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl_unittest.cc
M third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerInstalledScriptsManager.h
7 files changed, 121 insertions(+), 55 deletions(-)
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Makoto Shimazu (Gerrit)
Sorry for spamming!
I think it's now ready to review.
Patch set 6:Commit-Queue +1
Makoto Shimazu (Gerrit)
Makoto Shimazu would like Hiroki Nakagawa to review this change.
ServiceWorker: better error reporting for script streaming
This patch is to pass a couple of content_browsertests for testing disk cache
errors. In the current implementation, a service worker is shutted down if the
disk cache is broken, and as a result, |start_worker_status_| in
ServiceWorkerVersion is recorded as SERVICE_WORKER_ERROR_ABORT. This patch is to
shutdown the worker on the browser side and remove the ServiceWorkerVersion.
It's the same behavior with ServiceWorkerReadFromCacheJob.
Test: content_browsertests -f --gtest_filter="ServiceWorkerVersionBrowserTest.ReadResourceFailure*" --enable-features=ServiceWorkerScriptStreaming
Bug: 683037, 754139
Change-Id: Ibde6743434ddd88bec46721decec0f67ab0698bf
---
M content/browser/service_worker/service_worker_installed_scripts_sender.cc
M content/renderer/service_worker/thread_safe_script_container.cc
M content/renderer/service_worker/thread_safe_script_container.h
M content/renderer/service_worker/thread_safe_script_container_unittest.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl_unittest.cc
M third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerInstalledScriptsManager.h
7 files changed, 120 insertions(+), 55 deletions(-)
Hiroki Nakagawa (Gerrit)
4 comments:
Patch Set #6, Line 10:
errors. In the current implementation, a service worker is shutted down if theshutted -> shut
Patch Set #6, Line 13:
shutdown the worker on the browser side and remove the ServiceWorkerVersion.to shutdown -> to shut down
File content/browser/service_worker/service_worker_installed_scripts_sender.cc:
Patch Set #6, Line 419:
// This ends up with destructing |this|.If this destroys |this|, should we return at line 421 to avoid further operations on |this|?
File content/renderer/service_worker/thread_safe_script_container.h:
Patch Set #6, Line 81:
std::map<GURL, ScriptStatus> script_statuses_;From my personal experience, multiple maps sometimes cause inconsistent state because of code changes, so how about unifying these maps? For example,
struct Entry {
ScriptStatus status;
std::unique_ptr<Data> data;
};
std::map<GURL, Entry> script_data_;By the way, do we really need to have a separate field for |status|? Data class has |is_valid_| field, so I wonder if it's available for distinguishing the status instead of |status|.
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Makoto Shimazu (Gerrit)
Makoto Shimazu uploaded patch set #8 to this change.
ServiceWorker: better error reporting for script streaming
This patch is to pass a couple of content_browsertests for testing disk cache
errors. In the current implementation, a service worker is shut down if the
disk cache is broken, and as a result, |start_worker_status_| in
ServiceWorkerVersion is recorded as SERVICE_WORKER_ERROR_ABORT. This patch is to
shut down the worker on the browser side and remove the ServiceWorkerVersion.
It's the same behavior with ServiceWorkerReadFromCacheJob.
Test: content_browsertests -f --gtest_filter="ServiceWorkerVersionBrowserTest.ReadResourceFailure*" --enable-features=ServiceWorkerScriptStreaming
Bug: 683037, 754139
Change-Id: Ibde6743434ddd88bec46721decec0f67ab0698bf
---
M content/browser/service_worker/service_worker_installed_scripts_sender.cc
M content/renderer/service_worker/thread_safe_script_container.cc
M content/renderer/service_worker/thread_safe_script_container.h
M content/renderer/service_worker/thread_safe_script_container_unittest.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl_unittest.cc
M third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerInstalledScriptsManager.h
7 files changed, 119 insertions(+), 54 deletions(-)
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Makoto Shimazu (Gerrit)
Uploaded ps8. PTAnL.
Patch set 8:Commit-Queue +1
2 comments:
Patch Set #6, Line 419:
// This ends up with destructing |this|.If this destroys |this|, should we return at line 421 to avoid further operations on |this|?
Thanks, that makes sense.
File content/renderer/service_worker/thread_safe_script_container.h:
Patch Set #6, Line 81:
GURL waiting_url_;From my personal experience, multiple maps sometimes cause inconsistent state because of code change […]
Fair enough. Tried to remove the map and used IsInvalid() instead.
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
LGTM
3 comments:
File content/renderer/service_worker/thread_safe_script_container.cc:
Patch Set #8, Line 23:
void ThreadSafeScriptContainer::MarkFailedOnIOThread(const GURL& url) {AddOnIOThread(url, Data::CreateInvalidInstance()); here?
or
keeping this as is and adding DCHECK(data->IsValid()) in AddOnIOThread()?
Patch Set #8, Line 37:
// If the instance is invalid, return |kFailed|.Hmm... I feel separating state management from |Data| would make the code slightly more readable. Can you consider it later?
File content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:
Patch Set #8, Line 273:
DCHECK_EQ(ThreadSafeScriptContainer::ScriptStatus::kSuccess, status);How about changing these if-statements to switch-cases?
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Hiroki Nakagawa (Gerrit)
Patch set 8:Code-Review +1
Hiroki Nakagawa (Gerrit)
1 comment:
File content/renderer/service_worker/thread_safe_script_container.cc:
Patch Set #8, Line 37:
// If the instance is invalid, return |kFailed|.Hmm... I feel separating state management from |Data| would make the code slightly more readable. […]
Looks complicated...
No entry -> kPending
Entry exists, but data is null -> kSuccess(Taken)
Entry exists, but data is invalid -> kFailed
Entry exists, and data is valid -> kSuccess(NotTaken)
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Makoto Shimazu (Gerrit)
Patch set 10:Commit-Queue +2
Makoto Shimazu (Gerrit)
3 comments:
File content/renderer/service_worker/thread_safe_script_container.cc:
Patch Set #8, Line 23:
ThreadSafeScriptContainer::ScriptStatusAddOnIOThread(url, Data::CreateInvalidInstance()); here? […]
Done
Patch Set #8, Line 37:
DCHECK(waiting_url_.is_empty())Looks complicated... […]
Thanks. I'll upload a patch to simplify them soon. Added TODO comment for now.
File content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc:
Patch Set #8, Line 273:
return RawScriptData::CreateInvalidInstance();
How about changing these if-statements to switch-cases?
I think switch-cases isn't good here since it needs to fall through.
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Add another comment" https://chromium-review.googlesource.com/c/616540/10
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/616540/10
Bot data: {"action": "start", "triggered_at": "2017-08-18T08:09:27.0Z", "cq_cfg_revision": "5668c9aae8391d95373bdc16d23f5833b4e5ff37", "revision": "262ac8c9edd77c9bfa51a022fc65cb943dad6dfb"}
Hiroki Nakagawa (Gerrit)
1 comment:
Patch Set #8, Line 273:
return RawScriptData::CreateInvalidInstance();
I think switch-cases isn't good here since it needs to fall through.
Ah, I see. |status| can be updated at line 267. I missed the line. Acked.
To view, visit change 616540. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
ServiceWorker: better error reporting for script streaming
This patch is to pass a couple of content_browsertests for testing disk cache
errors. In the current implementation, a service worker is shut down if the
disk cache is broken, and as a result, |start_worker_status_| in
ServiceWorkerVersion is recorded as SERVICE_WORKER_ERROR_ABORT. This patch is to
shut down the worker on the browser side and remove the ServiceWorkerVersion.
It's the same behavior with ServiceWorkerReadFromCacheJob.
Test: content_browsertests -f --gtest_filter="ServiceWorkerVersionBrowserTest.ReadResourceFailure*" --enable-features=ServiceWorkerScriptStreaming
Bug: 683037, 754139
Change-Id: Ibde6743434ddd88bec46721decec0f67ab0698bf
Reviewed-on: https://chromium-review.googlesource.com/616540
Commit-Queue: Makoto Shimazu <shi...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495522}
---
M content/browser/service_worker/service_worker_installed_scripts_sender.cc
M content/renderer/service_worker/thread_safe_script_container.cc
M content/renderer/service_worker/thread_safe_script_container.h
M content/renderer/service_worker/thread_safe_script_container_unittest.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl.cc
M content/renderer/service_worker/web_service_worker_installed_scripts_manager_impl_unittest.cc
M third_party/WebKit/public/platform/modules/serviceworker/WebServiceWorkerInstalledScriptsManager.h
7 files changed, 109 insertions(+), 52 deletions(-)