Add rollback support to DeleteRegKeyWorkItem....

Add rollback support to DeleteRegKeyWorkItem.

BUG=none
TEST=new test added to installer_util_unittests.exe


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76496

[robertshield, 2011-03-01 16:04:43]

LG, comments:

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:26: //   shell lightweight API
funcs, so perhaps it's best to avoid them.
I feel we have insufficient evidence for shlwapi problems, please remove that
part of the comment.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:128: if (result !=
ERROR_SUCCESS) {
do you want to handle ERROR_MORE_DATA here (which would happen if an existing
value was lengthened after teh call to RegQueryInfoKey)?

It feels like we should try to still work in this case (we return false from the
WorkItem's Do() if we return false here).

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:150: NULL, NULL, NULL, NULL);
ditto

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:236: scoped_ptr<RegKeyBackup> 
backup;
nit: 1 space

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.h (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.h:21: // descriptors.
Could simplify the above comment to read:

"Be aware that in the event of rollback the key's values and subkeys are
restored but the key and its subkeys take on their default security
descriptors."

since if failures ARE ignored, then rollback isn't going to happen.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item_unittest.cc:92: // brings them
all back.
Could you also add a test that tests deleting a key with a subkey that you don't
have permission to delete and that the state of the key when done is the same?

[grt (UTC plus 2), 2011-03-01 20:54:01]

Thanks for the comments.  PTAL.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:26: //   shell lightweight API
funcs, so perhaps it's best to avoid them.
On 2011/03/01 16:04:43, robertshield wrote:
> I feel we have insufficient evidence for shlwapi problems, please remove that
> part of the comment.

Done.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:128: if (result !=
ERROR_SUCCESS) {
On 2011/03/01 16:04:43, robertshield wrote:
> do you want to handle ERROR_MORE_DATA here (which would happen if an existing
> value was lengthened after teh call to RegQueryInfoKey)?
> 
> It feels like we should try to still work in this case (we return false from
the
> WorkItem's Do() if we return false here). 

Done.  Also handles the case where values and/or keys are removed between
RegQueryInfoKey and RegEnum*.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:150: NULL, NULL, NULL, NULL);
On 2011/03/01 16:04:43, robertshield wrote:
> ditto

Done.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:236: scoped_ptr<RegKeyBackup> 
backup;
On 2011/03/01 16:04:43, robertshield wrote:
> nit: 1 space

Nice catch.  Done.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.h (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.h:21: // descriptors.
On 2011/03/01 16:04:43, robertshield wrote:
> Could simplify the above comment to read:
> 
> "Be aware that in the event of rollback the key's values and subkeys are
> restored but the key and its subkeys take on their default security
> descriptors."
> 
> since if failures ARE ignored, then rollback isn't going to happen.

Done.

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/6598065/diff/6008/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item_unittest.cc:92: // brings them
all back.
On 2011/03/01 16:04:43, robertshield wrote:
> Could you also add a test that tests deleting a key with a subkey that you
don't
> have permission to delete and that the state of the key when done is the same?

Done.

[robertshield, 2011-03-01 21:05:54]

LG, max_name_len nit below.

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:145: DCHECK(max_name_len == 0
&& name_size != 0||
nit: space between 0 and ||

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:145: DCHECK(max_name_len == 0
&& name_size != 0||
also, it looks to my addled brain liken max_name_len can never be 0

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item_unittest.cc:140:
sec_desc.FromString(L"D:PAI(A;OICI;KR;;;BU)");  // builtin users read
Awesome!

[grt (UTC plus 2), 2011-03-01 21:11:35]

Thanks again.  I'll submit this evening.

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item.cc (right):

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:145: DCHECK(max_name_len == 0
&& name_size != 0||
On 2011/03/01 21:05:54, robertshield wrote:
> nit: space between 0 and ||

Done.

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item.cc:145: DCHECK(max_name_len == 0
&& name_size != 0||
On 2011/03/01 21:05:54, robertshield wrote:
> also, it looks to my addled brain liken max_name_len can never be 0

Done.

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
File chrome/installer/util/delete_reg_key_work_item_unittest.cc (right):

http://codereview.chromium.org/6598065/diff/7003/chrome/installer/util/delete...
chrome/installer/util/delete_reg_key_work_item_unittest.cc:140:
sec_desc.FromString(L"D:PAI(A;OICI;KR;;;BU)");  // builtin users read
On 2011/03/01 21:05:54, robertshield wrote:
> Awesome!

Thanks.

[robert, 2011-03-01 21:20:25]

LGTM