Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1283273002:
Service Worker: Change last update check location and HTTP cache bypass rule (2/2) (Closed)
|
Created:
5 years, 4 months ago by jungkees Modified:
5 years, 2 months ago CC:
chromium-reviews, horo, jam, kinuko+serviceworker, kinuko+watch, serviceworker-reviews, tzik Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionService Worker: Change last update check location and HTTP cache bypass rule (2/2)
As per the last f2f and the follow-up discussion, the registration's last update
check time should be updated only when the script load request actually accessed
network and received response without a network error. The time stamp should be
updated regardless of whether the installation of the fetched resource succeeds
or fails.
This patch involves the following behavior changes:
1. Before: The last update check time was not updated when the installation of
the fetched resouce fails.
After: The last update check time is updated regardless of the result of the
installation.
2. Before: registration.update() always bypassed HTTP cache.
After: registration.update() no longer forces bypassing the HTTP cache but
bypasses it only when 24 hours have passed since the last update check, as
per spec.
Spec: https://github.com/slightlyoff/ServiceWorker/commit/569863f3acd0f5d79beab9d8c63210b8286ee507
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718
Companion CL (Blink layout test): https://codereview.chromium.org/1288653002/
BUG=520068
Patch Set 1 #Patch Set 2 : Update comments. #
Total comments: 5
Patch Set 3 : Do not store max-age to database. #
Total comments: 10
Patch Set 4 : Address the latest spec change. #
Total comments: 12
Patch Set 5 : Use |request->response_info().network_accessed| to check network access. #
Total comments: 2
Patch Set 6 : Pass a callback to UpdateScript(). #
Total comments: 4
Patch Set 7 : Add comments. #
Total comments: 8
Messages
Total messages: 37 (7 generated)
jungkee.song@samsung.com changed reviewers: + jinho.bang@samsung.com, l.gombos@samsung.com
PTAL.
falken@chromium.org changed reviewers: + falken@chromium.org
This is surprising to me, is it really what we decided? I commented on https://github.com/slightlyoff/ServiceWorker/issues/514
I left a comment in the spec issue. Depending on the decision, I'll apply the necessary change.
peer review looks good to me
jungkee.song@samsung.com changed reviewers: + kinuko@chromium.org, michaeln@chromium.org, nhiroki@chromium.org, tkent@chromium.org
It has been reviewed from the spec perspective too: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373. OWNERs, PTAL.
tkent@chromium.org changed reviewers: - tkent@chromium.org
Thank you for working on this. I wonder if we don't have to store max-age value in ServiceWorkerDatabase because the HTTP Cache stores it and the network stack appropriately handles a request depending on the cache freshness. I made comments from that point of view. https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_request_handler.cc (right): https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_request_handler.cc:72: if (time_since_last_check > version_->max_age() || The original code (time_since_last_check > FromHours(24)) could work for the latest spec because the max-age is recorded in the HTTP Cache and a network request can bypass the cache regardless of the bypass flag when the max-age passed. https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:350: if (time_since_last_check > new_version()->max_age() || We could use EmbeddedWorkerInstance::network_accessed_for_script_ instead of this calculation to check whether the cache was bypassed.
Thanks for the good point. I'm surely opened to a way not storing max-age value to database if possible. Please review my comment in service_worker_version.cc:2220 regarding the need of service worker's max age concept for scheduled updates. https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_context_request_handler.cc (right): https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_context_request_handler.cc:72: if (time_since_last_check > version_->max_age() || Agreed on your point. Having double-checked the logic, the original code should have been attained the same result. So, depending on the need/decision of the service worker's max age concept for scheduled updates, let me apply a relevant code for this part. https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:350: if (time_since_last_check > new_version()->max_age() || I got it. Thanks for the point. https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1283273002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_version.cc:2220: if (time_since_last_check > max_age()) I think this line of codes seems still need the concept of the service worker's max age for scheduled updates? In my understanding, we now consider min(Cache-Control: max-age's value, 86400) as the desired lifetime of the service worker. I'm not sure if we still have other ways to attain this instead of storing/using max-age.
On 2015/08/20 01:52:56, jungkees wrote: > In my understanding, we now consider > min(Cache-Control: max-age's value, 86400) as the desired lifetime of the > service worker. On second thought, I think we better clarify whether we'd really want to use min(Cache-Control: max-age's value, 86400) for scheduled updates too. And also it seems we need to spec it more clearly. (Now Soft Update only says "The user agent may call this as often as it likes to check for updates.") I'll leave a comment in the spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
It has been clarified that the spec doesn't want to require any scheduled update based on min(max-age, 86400). "The user agent may call Soft Update as often as it likes to check for updates." would be sufficient. I uploaded a new snapshot having addressed nhiroki@'s comments as such. PTAL.
Sorry for my late reply. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:143: void set_network_accessed_for_script(bool network_accessed) { nit: Can you remove this? In general, it'd be preferable not to add a function until we actually need it. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:441: // last update. Probably this comment should be moved to service_worker_context_core.h and only mention 86400 seconds (24-hours) update because the Cache-Control header is actually handled in the lower network layer like a regular request and SW does not have to take care of it. For example, "Updates the service worker. If |force_bypass_cache| is true or 86400 seconds (24 hours) have passed since the last update, bypasses the browser cache." https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:348: if (new_version()->embedded_worker()->network_accessed_for_script() || This could break some content_unittests? https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:349: new_version()->force_bypass_cache_for_scripts()) { This second condition seems no longer necessary because the first condition contains it.
Thanks for the review. I'll upload a snapshot once I sort out the related question (about the last update (check) time in the inline comment) here. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/embedded_worker_instance.h (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/embedded_worker_instance.h:143: void set_network_accessed_for_script(bool network_accessed) { Absolutely. I'll remove it. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:441: // last update. Agreed. I'll move the suggested comment to service_worker_context_core.h. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:348: if (new_version()->embedded_worker()->network_accessed_for_script() || Ah yes.. I found ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime needs some changes. I'll work on that. BTW, I noticed there seems to be a discrepancy in using the last update time between the spec and the implementation. Currently the spec does not update the registration's last update time internal slot when the UA hits the network bypassing HTTP cache but the script is byte-by-byte identical (i.e. 304 case.) But this line of code updates the value in this case. I think it needs to be sorted out to update the code. https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:349: new_version()->force_bypass_cache_for_scripts()) { Yes. I'll remove this second condition.
https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:348: if (new_version()->embedded_worker()->network_accessed_for_script() || On 2015/08/25 09:36:51, jungkees wrote: > Ah yes.. I found ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime needs some > changes. I'll work on that. > > BTW, I noticed there seems to be a discrepancy in using the last update time > between the spec and the implementation. Currently the spec does not update the > registration's last update time internal slot when the UA hits the network > bypassing HTTP cache but the script is byte-by-byte identical (i.e. 304 case.) > But this line of code updates the value in this case. I think it needs to be > sorted out to update the code. Thank you for clarifying the spec. The latest spec looks good[1]. I'll take another look when this CL is ready to review :) [1] https://github.com/slightlyoff/ServiceWorker/commit/569863f3acd0f5d79beab9d8c...
On 2015/08/28 09:19:55, nhiroki wrote: > Thank you for clarifying the spec. The latest spec looks good[1]. I'll take > another look when this CL is ready to review :) > > [1] > https://github.com/slightlyoff/ServiceWorker/commit/569863f3acd0f5d79beab9d8c... Thank you for the confirmation. I'll request a review when I'm ready with a new snapshot :)
Patchset #4 (id:100001) has been deleted
I uploaded a new snapshot having addressed the latest spec change upon our discussion: - Updating the |last_update_check_| has been moved to ServiceWorkerWriteToCacheJob::OnResponseStarted - Removed ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime and created ServiceWorkerWriteToCacheJobTest.Update_BumpLastUpdateCheckTime instead. PTAL https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_register_job.cc (right): https://codereview.chromium.org/1283273002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_register_job.cc:348: if (new_version()->embedded_worker()->network_accessed_for_script() || As per the latest spec change, I moved this part to ServiceWorkerWriteToCacheJob::OnResponseStarted (service_worker_write_to_cache_job.cc), and also removed ServiceWorkerJobTest.Update_BumpLastUpdateCheckTime and created ServiceWorkerWriteToCacheJobTest.Update_BumpLastUpdateCheckTime.
https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:186: // Updates the service worker. If |force_bypass_cache| is true or 86400 nit: Can you insert a blank line as a separator before this line? (and before line 181?) https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:393: false /* force_bypass_cache */); This should be true because this function is called from a 'update' button on DevTools which expects that the cache is bypassed (sorry, we should have added a comment about it...) To clarify this behavior, probably SWContextWrapper::UpdateRegistration() should receive |force_bypass_cache| flag as a parameter and convey it up to here. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:650: if (version_->embedded_worker()->network_accessed_for_script()) { This seems always false because |OnNetworkAccessedForScriptLoad| is called after this line (line 695). Maybe we can replace |network_accessed_for_script()| with |net_request_->response_info().network_accessed| (see line 694) and remove the function. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:92: "\0"; nit: These lines could be squashed into line 90. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:394: new_version->embedded_worker()->OnNetworkAccessedForScriptLoad(); This might not be necessary after incorporating my comment on service_worker_write_to_cache_job.cc ??
Thanks for the review! I uploaded a new snapshot having addressed the comments. PTAL https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:186: // Updates the service worker. If |force_bypass_cache| is true or 86400 On 2015/09/07 07:05:59, nhiroki wrote: > nit: Can you insert a blank line as a separator before this line? (and before > line 181?) Done. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:393: false /* force_bypass_cache */); On 2015/09/07 07:05:59, nhiroki wrote: > This should be true because this function is called from a 'update' button on > DevTools which expects that the cache is bypassed (sorry, we should have added a > comment about it...) > I wanted to have your comment on this indeed. So, I set it to true as suggested. > To clarify this behavior, probably SWContextWrapper::UpdateRegistration() should > receive |force_bypass_cache| flag as a parameter and convey it up to here. Is it better to include that plumbing as part of this patch or a separate one? Please let me know. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:650: if (version_->embedded_worker()->network_accessed_for_script()) { On 2015/09/07 07:05:59, nhiroki wrote: > This seems always false because |OnNetworkAccessedForScriptLoad| is called after > this line (line 695). Maybe we can replace |network_accessed_for_script()| with > |net_request_->response_info().network_accessed| (see line 694) and remove the > function. Thanks for spotting this! I replaced the condition with |net_request_->response_info().network_accessed| and removed |network_accessed_for_script()|, and updated the service_worker_write_to_cache_job_unittest.cc to satisfy this change. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:92: "\0"; On 2015/09/07 07:05:59, nhiroki wrote: > nit: These lines could be squashed into line 90. Done. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:394: new_version->embedded_worker()->OnNetworkAccessedForScriptLoad(); On 2015/09/07 07:05:59, nhiroki wrote: > This might not be necessary after incorporating my comment on > service_worker_write_to_cache_job.cc ?? Yes, this part has been removed.
Almost there. https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:393: false /* force_bypass_cache */); On 2015/09/08 16:01:13, jungkees wrote: > On 2015/09/07 07:05:59, nhiroki wrote: > > This should be true because this function is called from a 'update' button on > > DevTools which expects that the cache is bypassed (sorry, we should have added > a > > comment about it...) > > > > I wanted to have your comment on this indeed. So, I set it to true as suggested. > > > To clarify this behavior, probably SWContextWrapper::UpdateRegistration() > should > > receive |force_bypass_cache| flag as a parameter and convey it up to here. > > Is it better to include that plumbing as part of this patch or a separate one? > Please let me know. A separate CL should be OK. Can you leave a TODO comment here? https://codereview.chromium.org/1283273002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:405: bool network_accessed = true) { nit: Passing multiple booleans could be error-prone. How about passing a callback to be set in |mock_protocol_handler_| instead? UpdateScript(base::Bind(&CreateCachedResponseJob, response)); Or, creating separate functions? scoped_refptr<SWV> UpdateScriptWithNetworkResponse(response) { mock_protocol_handler_->SetCreateJobCallback( base::Bind(&CreateNetworkResponseJob, response)); UpdateScriptInternal(); } scoped_refptr<SWV> UpdateScriptWithCachedResponse(response) { mock_protocol_handler_->SetCreateJobCallback( base::Bind(&CreateCachedResponseJob, response)); UpdateScriptInternal(); } scoped_refptr<SWV> UpdateScriptWithErrorResponse(response) { mock_protocol_handler_->SetCreateJobCallback( base::Bind(&CreateErrorResponseJob, response)); UpdateScriptInternal(); }
Addressed the comment and uploaded a new snapshot. PTAL https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:393: false /* force_bypass_cache */); On 2015/09/09 08:27:48, nhiroki wrote: > On 2015/09/08 16:01:13, jungkees wrote: > > On 2015/09/07 07:05:59, nhiroki wrote: > > > This should be true because this function is called from a 'update' button > on > > > DevTools which expects that the cache is bypassed (sorry, we should have > added > > a > > > comment about it...) > > > > > > > I wanted to have your comment on this indeed. So, I set it to true as > suggested. > > > > > To clarify this behavior, probably SWContextWrapper::UpdateRegistration() > > should > > > receive |force_bypass_cache| flag as a parameter and convey it up to here. > > > > Is it better to include that plumbing as part of this patch or a separate one? > > Please let me know. > > A separate CL should be OK. Can you leave a TODO comment here? Done. https://codereview.chromium.org/1283273002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:405: bool network_accessed = true) { Agreed. I did it with the passing a callback option. I think it improves the readability too.
LGTM! Since the update check is important feature from the aspect of security and availability, I asked falken@ to double check this CL. Please wait for his check. Thank you for patiently working on this for a month!
I think this looks good. Can you make the CL description more explicit about the exact behavior change? If I understand correctly, the implementation before this patch already does "last update check time should be updated only when the script load request actually accessed network". This patch just moves the code from SWRegisterJob to SWWriteToCacheJob, is that correct? If so, could you explain the motivation for this move? I see the behavior change that registration.update() no longer forces bypassing the cache, as per spec. https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:392: // TODO(jungkees): Plumb |force_bypass_cache| through to UpdateRegistration(). Does not doing this TODO cause a bug? Can you explain in the TODO why we need to do this? https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:401: // to the script |response|. Returns the new version. Cann you add documentation for |callback|?
Thanks for the review! I updated the CL description and just uploaded a new snapshot having addressed the comments. PTAL https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:392: // TODO(jungkees): Plumb |force_bypass_cache| through to UpdateRegistration(). On 2015/09/10 01:29:24, falken wrote: > Does not doing this TODO cause a bug? It doesn't. It's just to provide options to callers in general since the other update paths do not force it. > Can you explain in the TODO why we need to > do this? Done. https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc (right): https://codereview.chromium.org/1283273002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc:401: // to the script |response|. Returns the new version. On 2015/09/10 01:29:24, falken wrote: > Cann you add documentation for |callback|? Done.
a drive by comment https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:655: registration->set_last_update_check(base::Time::Now()); I think this logic that applies to the register/update job as whole would probably fit more naturally in the ServiceWorkerRegisterJob class. This class is about downloading and writing a single resource to disk. https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:656: context_->storage()->UpdateLastUpdateCheckTime(registration); I'm not sure i understand this change? 1) The writing of the resources hasn't yet succeeded at this time. If it eventually fails we'll have a new update check time stored but no new script will have been downloaded or completely written to disk or run or ... 2) A different ServiceWorkerWriteToCacheJob is used for the main script and all imports. The check time will be updated multiple times as the main script and each import is loaded. 3) This can occur prior to a registration being 'stored' for the very first time. It'll be trying to updating something that does not yet exist in the db? Looks like that won't cause any problem but we'll be reading when we don't have to and getting errors. RegistrationData registration; status = ReadRegistrationData(registration_id, origin, ®istration); if (status != STATUS_OK) return status;
Thanks for the review. I left my answers and questions inline below. https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:655: registration->set_last_update_check(base::Time::Now()); On 2015/09/10 20:26:45, michaeln wrote: > I think this logic that applies to the register/update job as whole would > probably fit more naturally in the ServiceWorkerRegisterJob class. This class is > about downloading and writing a single resource to disk. I see. The reason I put this code here is to update the time stamp before checking any errors such as the MIME type and the script location. This is because the spec defines the last update check as purely the most recent time the UA accessed the network to check the script resource. If we'd want to move this to ServiceWorkerRegisterJob class, the spec may correspondingly need to move the Update algorithm step 4.13 to step 4.24, which would mean the MIME type error and the script location violation prevent it from updating the time stamp. I'd like to hear some opinions on this. /cc michaeln@, nhiroki@, falken@ https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:656: context_->storage()->UpdateLastUpdateCheckTime(registration); On 2015/09/10 20:26:45, michaeln wrote: > I'm not sure i understand this change? > > 1) The writing of the resources hasn't yet succeeded at this time. If it > eventually fails we'll have a new update check time stored but no new script > will have been downloaded or completely written to disk or run or ... > This is expected behavior as per the current spec text. > 2) A different ServiceWorkerWriteToCacheJob is used for the main script and all > imports. The check time will be updated multiple times as the main script and > each import is loaded. > Yeah, this is a problem. I wanted to update it only for the main script. > 3) This can occur prior to a registration being 'stored' for the very first > time. It'll be trying to updating something that does not yet exist in the db? > Looks like that won't cause any problem but we'll be reading when we don't have > to and getting errors. > > RegistrationData registration; > status = ReadRegistrationData(registration_id, origin, ®istration); > if (status != STATUS_OK) > return status; > This seems a bit different from the spec where the registration is stored for the first time before getting to the update phase. If we decide to move the code to ServiceWorkerRegisterJob class, I think we'd still have to put it in the beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished method. Then, that'd be still before ServiceWorkerStorage::StoreRegistration has been executed for the first registration attempt.
On 2015/09/11 04:28:14, jungkees wrote: > Thanks for the review. I left my answers and questions inline below. > > https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... > File content/browser/service_worker/service_worker_write_to_cache_job.cc > (right): > > https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... > content/browser/service_worker/service_worker_write_to_cache_job.cc:655: > registration->set_last_update_check(base::Time::Now()); > On 2015/09/10 20:26:45, michaeln wrote: > > I think this logic that applies to the register/update job as whole would > > probably fit more naturally in the ServiceWorkerRegisterJob class. This class > is > > about downloading and writing a single resource to disk. > > I see. The reason I put this code here is to update the time stamp before > checking any errors such as the MIME type and the script location. This is > because the spec defines the last update check as purely the most recent time > the UA accessed the network to check the script resource. > > If we'd want to move this to ServiceWorkerRegisterJob class, the spec may > correspondingly need to move the Update algorithm step 4.13 to step 4.24, which > would mean the MIME type error and the script location violation prevent it from > updating the time stamp. I'd like to hear some opinions on this. /cc michaeln@, > nhiroki@, falken@ First, Michael thanks for pointing out things we missed. Updating the timestamp even when install fails (whether due to script error, or disk error, or MIME header) as the current spec mandates sounds OK with me, quoting slightlyoff "the simpler rule which doesn't pay attention to install success or failure doesn't strike me as being problematic. A site with a busted SW is the reason to have the backstop and potentially DoSing a server for which there's a failed SW install seems like a bad idea" https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135596131 A more intelligent impl might retry writing on disk error but I suspect it's not so important, you'll retry again after 24 hours. Jungkee, would it make sense to split the update() behavior change and timestamp behavior change into two patches? The update() change seems ready now. I don't see test coverage for the timestamp change yet. > > https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... > content/browser/service_worker/service_worker_write_to_cache_job.cc:656: > context_->storage()->UpdateLastUpdateCheckTime(registration); > On 2015/09/10 20:26:45, michaeln wrote: > > I'm not sure i understand this change? > > > > 1) The writing of the resources hasn't yet succeeded at this time. If it > > eventually fails we'll have a new update check time stored but no new script > > will have been downloaded or completely written to disk or run or ... > > > > This is expected behavior as per the current spec text. > > > 2) A different ServiceWorkerWriteToCacheJob is used for the main script and > all > > imports. The check time will be updated multiple times as the main script and > > each import is loaded. > > > > Yeah, this is a problem. I wanted to update it only for the main script. > > > 3) This can occur prior to a registration being 'stored' for the very first > > time. It'll be trying to updating something that does not yet exist in the db? > > Looks like that won't cause any problem but we'll be reading when we don't > have > > to and getting errors. > > > > RegistrationData registration; > > status = ReadRegistrationData(registration_id, origin, ®istration); > > if (status != STATUS_OK) > > return status; > > > > This seems a bit different from the spec where the registration is stored for > the first time before getting to the update phase. If we decide to move the code > to ServiceWorkerRegisterJob class, I think we'd still have to put it in the > beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished method. Then, > that'd be still before ServiceWorkerStorage::StoreRegistration has been executed > for the first registration attempt.
On 2015/09/11 05:10:21, falken wrote: > On 2015/09/11 04:28:14, jungkees wrote: > > Thanks for the review. I left my answers and questions inline below. > > > > > https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... > > File content/browser/service_worker/service_worker_write_to_cache_job.cc > > (right): > > > > > https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... > > content/browser/service_worker/service_worker_write_to_cache_job.cc:655: > > registration->set_last_update_check(base::Time::Now()); > > On 2015/09/10 20:26:45, michaeln wrote: > > > I think this logic that applies to the register/update job as whole would > > > probably fit more naturally in the ServiceWorkerRegisterJob class. This > class > > is > > > about downloading and writing a single resource to disk. > > > > I see. The reason I put this code here is to update the time stamp before > > checking any errors such as the MIME type and the script location. This is > > because the spec defines the last update check as purely the most recent time > > the UA accessed the network to check the script resource. > > > > If we'd want to move this to ServiceWorkerRegisterJob class, the spec may > > correspondingly need to move the Update algorithm step 4.13 to step 4.24, > which > > would mean the MIME type error and the script location violation prevent it > from > > updating the time stamp. I'd like to hear some opinions on this. /cc > michaeln@, > > nhiroki@, falken@ > > First, Michael thanks for pointing out things we missed. > > Updating the timestamp even when install fails (whether due to script error, or > disk error, or MIME header) as the current spec mandates sounds OK with me, > quoting slightlyoff "the simpler rule which doesn't pay attention to install > success or failure doesn't strike me as being problematic. A site with a busted > SW is the reason to have the backstop and potentially DoSing a server for which > there's a failed SW install seems like a bad idea" > https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135596131 > > A more intelligent impl might retry writing on disk error but I suspect it's not > so important, you'll retry again after 24 hours. > > Jungkee, would it make sense to split the update() behavior change and timestamp > behavior change into two patches? The update() change seems ready now. I don't > see test coverage for the timestamp change yet. > Agreed. Re the CL management, which option would be the best practice in general? 1) Use this CL for timestamp change and create a new CL for update() change 2) The other way around 3) Create two new CLs that point to this CL.
And is it better to split the BUG entries too?
On 2015/09/11 05:47:44, jungkees wrote: > And is it better to split the BUG entries too? Personally I'd make two new CLs and use separate BUGs.
https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:650: if (request->response_info().network_accessed) { I'm not sure this matters, but... I don't think network_accessed necessarily means the resource was retrieved from the network. I think this flag will be true in cases where the resource in the http cache gets validated (requiring a server resposne) but the resource is not necessarily downloaded from the server. https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:656: context_->storage()->UpdateLastUpdateCheckTime(registration); On 2015/09/11 04:28:14, jungkees wrote: > On 2015/09/10 20:26:45, michaeln wrote: > > I'm not sure i understand this change? > > > > 1) The writing of the resources hasn't yet succeeded at this time. If it > > eventually fails we'll have a new update check time stored but no new script > > will have been downloaded or completely written to disk or run or ... > > > > This is expected behavior as per the current spec text. Seems odd to update the time even if the job is aborted (browser is quit) prior to finishing the script resource download, but ok. > > 2) A different ServiceWorkerWriteToCacheJob is used for the main script and > all > > imports. The check time will be updated multiple times as the main script and > > each import is loaded. > > > > Yeah, this is a problem. I wanted to update it only for the main script. > > > 3) This can occur prior to a registration being 'stored' for the very first > > time. It'll be trying to updating something that does not yet exist in the db? > > Looks like that won't cause any problem but we'll be reading when we don't > have > > to and getting errors. > > > > RegistrationData registration; > > status = ReadRegistrationData(registration_id, origin, ®istration); > > if (status != STATUS_OK) > > return status; > > > > This seems a bit different from the spec where the registration is stored for > the first time before getting to the update phase. If we decide to move the code > to ServiceWorkerRegisterJob class, I think we'd still have to put it in the > beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished method. Then, > that'd be still before ServiceWorkerStorage::StoreRegistration has been executed > for the first registration attempt. you have to squint a little when directly comparing a spec with an impl iirc, the spec didn't make a distinction between being 'stored' in the map of scopes to registrations and being persisted to disk. In chrome, registrations are 'stored' in the map when a job commences and if it get far enough along to warrant being written to disk, that happens. I think updating the value of the reg instance corresponds to 'storing' the new time in the language of the spec. When that gets blasted to disk is an impl detail.
Sorry for coming back late. Thanks for the review, michaeln@. I just left comments. https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_write_to_cache_job.cc (right): https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:650: if (request->response_info().network_accessed) { On 2015/09/11 18:59:26, michaeln wrote: > I'm not sure this matters, but... > > I don't think network_accessed necessarily means the resource was retrieved from > the network. I think this flag will be true in cases where the resource in the > http cache gets validated (requiring a server resposne) but the resource is not > necessarily downloaded from the server. Sorry for getting back late. You are right. What this line of code wanted to do is to check whether the resource was served from the server not from the http cache. Having looked at the code, I presume |was_cached| of HttpResponseInfo object can help here. How about doing the following in ServiceWorkerWriteToCacheJob::OnResponseStarted() if (net_request_->response_info().network_accessed && !(net_request_->response_info().was_cached)) // i.e add this condition version_->embedded_worker()->OnNetworkAccessedForScriptLoad(); And check the EmbeddedWorkerInstance's |network_accessed_for_script_| in the beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished() like the following? void ServiceWorkerRegisterJob::OnStartWorkerFinished( ServiceWorkerStatusCode status) { if (new_version()->embedded_worker()->network_accessed_for_script() || new_version()->force_bypass_cache_for_scripts()) { // set_last_update_check } if (status == SERVICE_WORKER_OK) { ... For this change, the Update algorithm step 4.13 needs to move to 4.24. I think that'd be fine if this change makes sense. WDYT? https://codereview.chromium.org/1283273002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_write_to_cache_job.cc:656: context_->storage()->UpdateLastUpdateCheckTime(registration); On 2015/09/11 18:59:26, michaeln wrote: > On 2015/09/11 04:28:14, jungkees wrote: > > On 2015/09/10 20:26:45, michaeln wrote: > > > I'm not sure i understand this change? > > > > > > 1) The writing of the resources hasn't yet succeeded at this time. If it > > > eventually fails we'll have a new update check time stored but no new script > > > will have been downloaded or completely written to disk or run or ... > > > > > > > This is expected behavior as per the current spec text. > > Seems odd to update the time even if the job is aborted (browser is quit) prior > to finishing the script resource download, but ok. > > > > 2) A different ServiceWorkerWriteToCacheJob is used for the main script and > > all > > > imports. The check time will be updated multiple times as the main script > and > > > each import is loaded. > > > > > > > Yeah, this is a problem. I wanted to update it only for the main script. > > > > > 3) This can occur prior to a registration being 'stored' for the very first > > > time. It'll be trying to updating something that does not yet exist in the > db? > > > Looks like that won't cause any problem but we'll be reading when we don't > > have > > > to and getting errors. > > > > > > RegistrationData registration; > > > status = ReadRegistrationData(registration_id, origin, ®istration); > > > if (status != STATUS_OK) > > > return status; > > > > > > > This seems a bit different from the spec where the registration is stored for > > the first time before getting to the update phase. If we decide to move the > code > > to ServiceWorkerRegisterJob class, I think we'd still have to put it in the > > beginning of ServiceWorkerRegisterJob::OnStartWorkerFinished method. Then, > > that'd be still before ServiceWorkerStorage::StoreRegistration has been > executed > > for the first registration attempt. > > you have to squint a little when directly comparing a spec with an impl > > iirc, the spec didn't make a distinction between being 'stored' in the map of > scopes to registrations and being persisted to disk. In chrome, registrations > are 'stored' in the map when a job commences and if it get far enough along to > warrant being written to disk, that happens. I think updating the value of the > reg instance corresponds to 'storing' the new time in the language of the spec. > When that gets blasted to disk is an impl detail. Indeed.
Message was sent while issue was closed.
Closing this CL in favor of: https://codereview.chromium.org/1380323002/ (Landed) https://codereview.chromium.org/1381153004/ (WIP) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
