Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
892103003:
PlatformKeysService: Process state accessing operations sequentially. (Closed)
|
Created:
5 years, 10 months ago by pneubeck (no reviews) Modified:
5 years, 10 months ago Reviewers:
kaliamoorthi CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cert_impl_sign Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlatformKeysService: Process state accessing operations sequentially.
To prevent concurrent updates and reading old state, operations have to executed in sequence if they read or modify an extension's StateStore.
This change is not affecting/modifying the crypto operations but only changes the processing order of the operations.
BUG=450167
Committed: https://crrev.com/76cf3b94584a622a9017c1e3b31f82c18e764804
Cr-Commit-Position: refs/heads/master@{#316411}
Patch Set 1 : Rebased. #Patch Set 2 : Minor cleanup. #
Total comments: 8
Patch Set 3 : Removed NOT_STARTED state. #
Total comments: 8
Patch Set 4 : Addressed comments. #
Total comments: 4
Patch Set 5 : Rebased. #Patch Set 6 : Addressed comments #
Messages
Total messages: 26 (13 generated)
Patchset #1 (id:1) has been deleted
pneubeck@chromium.org changed reviewers: + tnagel@chromium.org
-Bartosz, +Thiemo
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
pneubeck@chromium.org changed reviewers: + kaliamoorthi@chromium.org - tnagel@chromium.org
-Thiemo Prabhu, ptal.
Patchset #2 (id:100001) has been deleted
Patchset #3 (id:140001) has been deleted
Some high level comments first. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:52: WRITE_UPDATE, Not sure if you need so many states. There is just a single asynchronous call that is handled by this class. When the asynchronous call is complete, the callback makes the state machine transition through WRITE_UPDATE and DONE. NOT_STARTED is not that useful either. Makes one wonder if the state machine is even needed here. It bloats the code for sure. A succinct implementation can reduce the LOC in this class by a factor of 2. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:221: permission_update_.reset(new PermissionUpdateTask( Combining SignTask with PermissionUpdateTask can result in a class that justifies the use of a state machine. A clever implementation can make PermissionUpdateTask be trigger-able by mentioning the end state alone since both tasks execute PermissionUpdateTask as the first step. It would possibly also help in constructor argument sharing. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right): https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:124: // This ensures to start at most one task at a time. |task| https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:154: // Callback used by |GeneratedKey|. Expand this comment.
Patchset #3 (id:160001) has been deleted
ptal https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:52: WRITE_UPDATE, On 2015/02/09 10:44:01, kaliamoorthi wrote: > Not sure if you need so many states. There is just a single asynchronous call > that is handled by this class. When the asynchronous call is complete, the > callback makes the state machine transition through WRITE_UPDATE and DONE. > NOT_STARTED is not that useful either. Makes one wonder if the state machine is > even needed here. It bloats the code for sure. A succinct implementation can > reduce the LOC in this class by a factor of 2. dropped the NOT_STARTED state (which was meant to more clearly document what the _current_ state is). But yeah, the additional bloat might not be worth it. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:221: permission_update_.reset(new PermissionUpdateTask( On 2015/02/09 10:44:01, kaliamoorthi wrote: > Combining SignTask with PermissionUpdateTask can result in a class that > justifies the use of a state machine. A clever implementation can make > PermissionUpdateTask be trigger-able by mentioning the end state alone since > both tasks execute PermissionUpdateTask as the first step. It would possibly > also help in constructor argument sharing. PermissionUpdate will also be used independent of Signing. I'd find it confusing if that would be too tightly coupled with Signing. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right): https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:124: // This ensures to start at most one task at a time. On 2015/02/09 10:44:01, kaliamoorthi wrote: > |task| Done. https://codereview.chromium.org/892103003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:154: // Callback used by |GeneratedKey|. On 2015/02/09 10:44:01, kaliamoorthi wrote: > Expand this comment. Done.
https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:55: PermissionUpdateTask(const bool new_permission_value, Document https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:300: tasks_.push(make_linked_ptr(task.release())); Is there are reason why linked_ptr is used here rather than scoped_ptr? linked_ptr is said to incur more overhead and can get you into trouble when casting to and back from raw pointers. https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:308: while (!tasks_.empty() && tasks_.front()->IsDone()) Can multiple tasks execute at the same time? Either the documentation in the header is wrong or the code is redundant. https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:351: callback.Run(public_key_spki_der, std::string() /* no error */); Why is this parameter needed if it is empty? If it will be filled in later, a TODO?
https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:55: PermissionUpdateTask(const bool new_permission_value, On 2015/02/09 15:13:15, kaliamoorthi wrote: > Document Done. https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:300: tasks_.push(make_linked_ptr(task.release())); On 2015/02/09 15:13:15, kaliamoorthi wrote: > Is there are reason why linked_ptr is used here rather than scoped_ptr? > linked_ptr is said to incur more overhead and can get you into trouble when > casting to and back from raw pointers. scoped_ptrs don't work in containers as it internally copies. The only scoped container that we have in chrome that I know of is scoped_vector. For other containers the linked_ptr is usually used. https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:308: while (!tasks_.empty() && tasks_.front()->IsDone()) On 2015/02/09 15:13:15, kaliamoorthi wrote: > Can multiple tasks execute at the same time? Either the documentation in the > header is wrong or the code is redundant. At most one task is running at a time. Do you suggest code like if (tasks_.front()->IsDone()) tasks_.pop(); instead? I chose the while loop to be more robust (less assumptions for this function), in case there happen to be more tasks in the queue which are already finished. Added a comment for clarification. https://codereview.chromium.org/892103003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:351: callback.Run(public_key_spki_der, std::string() /* no error */); On 2015/02/09 15:13:15, kaliamoorthi wrote: > Why is this parameter needed if it is empty? If it will be filled in later, a > TODO? which parameter? the error parameter? See GeneratedKey, there a non-empty error is passed to the same callback.
LGTM with nits https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:357: void PlatformKeysService::DidRegisterGeneratedKey( nit: How about RegisteredGeneratedKey? I see other callbacks are named this way and this one is inconsistent. https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right): https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:124: // Starts |task| eventually. To ensures that at most one |Task| is running at nit: ensures -> ensure
New patchsets have been uploaded after l-g-t-m from kaliamoorthi@chromium.org
The CQ bit was checked by pneubeck@chromium.org
https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.cc (right): https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.cc:357: void PlatformKeysService::DidRegisterGeneratedKey( On 2015/02/09 16:02:10, kaliamoorthi wrote: > nit: How about RegisteredGeneratedKey? I see other callbacks are named this way > and this one is inconsistent. Done. https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... File chrome/browser/chromeos/platform_keys/platform_keys_service.h (right): https://codereview.chromium.org/892103003/diff/200001/chrome/browser/chromeos... chrome/browser/chromeos/platform_keys/platform_keys_service.h:124: // Starts |task| eventually. To ensures that at most one |Task| is running at On 2015/02/09 16:02:10, kaliamoorthi wrote: > nit: ensures -> ensure Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892103003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/892103003/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/76cf3b94584a622a9017c1e3b31f82c18e764804 Cr-Commit-Position: refs/heads/master@{#316411} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
