Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181573010:
ServiceWorker: Fix matching registration info missing during shift-reload. (Closed)
|
Created:
5 years, 6 months ago by xiang Modified:
5 years, 6 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Fix matching registration info missing during shift-reload.
Currently the new document generated by shift-reload doesn't retain any
previous matching registration info, then the .ready or .claim() can't
find correct registration through MatchRegistration().
This CL will add all living matching registrations to the new provider host.
The old provider host will keep related registrations alive until provisional
load be committed.
Layout test CL: https://codereview.chromium.org/1184923005/
BUG=462529
Committed: https://crrev.com/4310022a0cb532805f13ba2ca0d6d048795e1949
Cr-Commit-Position: refs/heads/master@{#336127}
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : address #3 & #4 #
Total comments: 4
Patch Set 4 : iterate living registrations #
Total comments: 8
Patch Set 5 : cleanup #
Total comments: 1
Patch Set 6 : cleanup #
Messages
Total messages: 18 (3 generated)
xiang.long@intel.com changed reviewers: + falken@chromium.org, michaeln@chromium.org
PTAL, thanks!
Thanks for working on this so quickly! https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:313: continue; I wonder if we can really be sure an old provider host is found. Would an alternative approach be to just iterate over live registrations and add matching ones? https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:223: // Copy matched registrations from the old one for force-refresh generated "the old one" is vague here... "from the old ServiceWorkerProviderHost" is more clear. https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:79: if (skip_service_worker) { Can we come here for something other than force-refresh, and in that case there may not be an old provider host to copy from. Is that OK?
https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:309: ServiceWorkerProviderHost* host = it->GetProviderHost(); need to compare host process_ids as well https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:313: continue; On 2015/06/17 15:08:35, falken wrote: > I wonder if we can really be sure an old provider host is found. Would an > alternative approach be to just iterate over live registrations and add matching > ones? In the reload case, there will be an old provider, but your question about whether 'skip' might apply to other cases is a good one. Seems like a method to AddMatchingRegistrations() might be more robust?
Thanks for the review. https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:309: ServiceWorkerProviderHost* host = it->GetProviderHost(); On 2015/06/17 21:20:14, michaeln wrote: > need to compare host process_ids as well Done. https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:313: continue; On 2015/06/17 21:20:14, michaeln wrote: > On 2015/06/17 15:08:35, falken wrote: > > I wonder if we can really be sure an old provider host is found. Would an > > alternative approach be to just iterate over live registrations and add > matching > > ones? > > In the reload case, there will be an old provider, but your question about > whether 'skip' might apply to other cases is a good one. Seems like a method to > AddMatchingRegistrations() might be more robust? I feel finding the old SWPH maybe faster than get all living registrations info which contains vector creation and iterate between them. I took a look of 'skip_service_worker', found it should be safe to say when both 'skip' and 'main resource' are true the request should be started by a force reload. Also I added a LOAD_BYPASS_CACHE to make this operation more explicit. https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:223: // Copy matched registrations from the old one for force-refresh generated On 2015/06/17 15:08:35, falken wrote: > "the old one" is vague here... "from the old ServiceWorkerProviderHost" is more > clear. Done.
Thanks for updating. https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:223: // Copy matched registrations from the old ServiceWorkerProviderHost force "force force-refresh" Maybe should be "for a document generated by shift-reload" https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:84: if (request->load_flags() & net::LOAD_BYPASS_CACHE) I'm a bit worried we're adding this if() without being sure of the possible states, which will lead to difficult to maintain code. If |skip_service_worker| without |LOAD_BYPASS_CACHE| is possible, it seems we we still need to add matching registrations for that case. If it's not possible, we should be able to add a DCHECK() and a comment "skip_service_worker means shift-refresh, so copy from the old host" or somehting. But, it looks like |skip_service_worker| can also come form devtool's "Disable cache", which is why we added |skip_service_worker| in the first place: https://chromium.googlesource.com/chromium/src/+/258fc42376fc25bb4fdfbc372ce5.... In which case there may not necessarily be an old provider host to copy from. I think it might just be OK to add a possibly slower AddMatchingRegistrations, and later on optimize it for shift-refresh case if warranted.
https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:313: continue; On 2015/06/19 07:43:24, xiang wrote: > On 2015/06/17 21:20:14, michaeln wrote: > > On 2015/06/17 15:08:35, falken wrote: > > > I wonder if we can really be sure an old provider host is found. Would an > > > alternative approach be to just iterate over live registrations and add > > matching > > > ones? > > > > In the reload case, there will be an old provider, but your question about > > whether 'skip' might apply to other cases is a good one. Seems like a method > to > > AddMatchingRegistrations() might be more robust? > > I feel finding the old SWPH maybe faster than get all living registrations info > which contains vector creation and iterate between them. > > I took a look of 'skip_service_worker', found it should be safe to say when both > 'skip' and 'main resource' are true the request should be started by a force > reload. > > Also I added a LOAD_BYPASS_CACHE to make this operation more explicit. I think agree with falken about doing this as AddMatchingRegistrations() instead of copying from an old host. If we 'skip' service workers for any reason on page load, we'd want to do this, right? About performance, just add these getters to ServiceWorkerContextCore. const std::map<int64, ServiceWorkerRegistration*>& GetLiveRegistrations() const { return live_registrations_; } const std::map<int64, ServiceWorkerVersion*>& GetLiveVersions() const { return live_versions_; }
https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/20001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:313: continue; On 2015/06/22 20:33:52, michaeln wrote: > On 2015/06/19 07:43:24, xiang wrote: > > On 2015/06/17 21:20:14, michaeln wrote: > > > On 2015/06/17 15:08:35, falken wrote: > > > > I wonder if we can really be sure an old provider host is found. Would an > > > > alternative approach be to just iterate over live registrations and add > > > matching > > > > ones? > > > > > > In the reload case, there will be an old provider, but your question about > > > whether 'skip' might apply to other cases is a good one. Seems like a method > > to > > > AddMatchingRegistrations() might be more robust? > > > > I feel finding the old SWPH maybe faster than get all living registrations > info > > which contains vector creation and iterate between them. > > > > I took a look of 'skip_service_worker', found it should be safe to say when > both > > 'skip' and 'main resource' are true the request should be started by a force > > reload. > > > > Also I added a LOAD_BYPASS_CACHE to make this operation more explicit. > > > I think agree with falken about doing this as AddMatchingRegistrations() instead > of copying from an old host. If we 'skip' service workers for any reason on page > load, we'd want to do this, right? > > About performance, just add these getters to ServiceWorkerContextCore. > > const std::map<int64, ServiceWorkerRegistration*>& GetLiveRegistrations() const > { return live_registrations_; } > const std::map<int64, ServiceWorkerVersion*>& GetLiveVersions() const { return > live_versions_; } > Thanks for the suggestion, CL changed. https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:223: // Copy matched registrations from the old ServiceWorkerProviderHost force On 2015/06/22 06:04:02, falken wrote: > "force force-refresh" > > Maybe should be "for a document generated by shift-reload" Done. https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/40001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:84: if (request->load_flags() & net::LOAD_BYPASS_CACHE) On 2015/06/22 06:04:02, falken wrote: > I'm a bit worried we're adding this if() without being sure of the possible > states, which will lead to difficult to maintain code. > > If |skip_service_worker| without |LOAD_BYPASS_CACHE| is possible, it seems we we > still need to add matching registrations for that case. If it's not possible, we > should be able to add a DCHECK() and a comment "skip_service_worker means > shift-refresh, so copy from the old host" or somehting. > > But, it looks like |skip_service_worker| can also come form devtool's "Disable > cache", which is why we added |skip_service_worker| in the first place: > https://chromium.googlesource.com/chromium/src/+/258fc42376fc25bb4fdfbc372ce5.... > In which case there may not necessarily be an old provider host to copy from. > > I think it might just be OK to add a possibly slower AddMatchingRegistrations, > and later on optimize it for shift-refresh case if warranted. I changed to iterate all live registrations and made a DCHECK for LOAD_BYPASS_CACHE flag with new getters in context core. If I understand correctly, devtool "disable cache" will not set skip_service_worker, instead it will set LOAD_BYPASS_CACHE flag. So we will only copy registrations for shift-reload.
lgtm, thanks! This line in the CL description should be updated: "This CL will copy previous provider host matching registration info to the new one, as the previous one will exist till provisional load be committed." https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:314: for (auto& key_registration : registrations) { nit: extra space. (i just always use "git-cl format" before uploading) https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:227: void CopyMatchingRegistrations(); nit: we're not really copying it now, just adding. Maybe AddAllMatchingRegistrations()?
lgtm 2 modulo nits https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:314: for (auto& key_registration : registrations) { does const auto& work? https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:86: DCHECK(request->load_flags() & net::LOAD_BYPASS_CACHE); I'm not sure the dcheck is really appropriate, seems like it could make sense to skip a registered sw w/o busting the http cache? The dcheck implies that its not ok for that to happen, but why?
Thanks for the review. https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:314: for (auto& key_registration : registrations) { On 2015/06/24 02:24:26, falken wrote: > nit: extra space. (i just always use "git-cl format" before uploading) Done. https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:314: for (auto& key_registration : registrations) { On 2015/06/24 19:47:22, michaeln wrote: > does const auto& work? Done. https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:227: void CopyMatchingRegistrations(); On 2015/06/24 02:24:26, falken wrote: > nit: we're not really copying it now, just adding. Maybe > AddAllMatchingRegistrations()? Done. https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:86: DCHECK(request->load_flags() & net::LOAD_BYPASS_CACHE); On 2015/06/24 19:47:22, michaeln wrote: > I'm not sure the dcheck is really appropriate, seems like it could make sense to > skip a registered sw w/o busting the http cache? The dcheck implies that its not > ok for that to happen, but why? The dcheck is on the assumption that when a main resource load happened with skip sw, the load should be triggered by shift-reload. As shift-reload has LOAD_BY_PASS_CACHE specified, we can double check it here. I feel it's OK to remove this dcheck as long as AddMatchingRegistrations doesn't make any bad side effect.
OK with me to remove the DCHECK https://codereview.chromium.org/1181573010/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1181573010/diff/80001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:18: #include "net/base/load_flags.h" nit: no longer need this
On 2015/06/25 05:50:08, falken wrote: > OK with me to remove the DCHECK > > https://codereview.chromium.org/1181573010/diff/80001/content/browser/service... > File content/browser/service_worker/service_worker_request_handler.cc (right): > > https://codereview.chromium.org/1181573010/diff/80001/content/browser/service... > content/browser/service_worker/service_worker_request_handler.cc:18: #include > "net/base/load_flags.h" > nit: no longer need this Thanks, done.
The CQ bit was checked by xiang.long@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/1181573010/#ps100001 (title: "cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181573010/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4310022a0cb532805f13ba2ca0d6d048795e1949 Cr-Commit-Position: refs/heads/master@{#336127} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
