Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16232019:
Fix for finding a disabled origin after it was already moved to enabled. (Closed)
|
Created:
7 years, 6 months ago by calvinlo Modified:
7 years, 6 months ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix for finding a disabled origin after it was already moved to enabled.
BUG=245268
TEST=
DriveFileSyncServiceTest.DisableOriginForTrackingChangesPendingOrigin
DriveFileSyncServiceTest.DisableOriginForTrackingChangesIncrementalOrigin
DriveFileSyncServiceTest.ReenableOriginForTrackingChanges
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203927
Patch Set 1 #Patch Set 2 : For manual diff check #Patch Set 3 : Revision after diff #
Total comments: 20
Patch Set 4 : Kinuko and Hiroki review #1wq #Patch Set 5 : Revert Reenable* -> Enable* #Patch Set 6 : Hiroki review #2 #
Messages
Total messages: 19 (0 generated)
Can one of you please take a look at this. It's just a one line change. Unfortunately I previously made a mistake in the call order here. I have to find the origin's resource ID from the disabled list BEFORE I re-enable the origin in DriveMetadataStore.
looks good, can we / should we add a small test for EnableOriginForTrackingChanges (and other *OriginForTrackingChanges that have no tests yet)?
looks good, too. On 2013/05/31 14:36:22, kinuko wrote: > looks good, can we / should we add a small test for > EnableOriginForTrackingChanges (and other *OriginForTrackingChanges that have no > tests yet)? +1
On 2013/06/03 02:25:07, nhiroki wrote: > looks good, too. > > On 2013/05/31 14:36:22, kinuko wrote: > > looks good, can we / should we add a small test for > > EnableOriginForTrackingChanges (and other *OriginForTrackingChanges that have > no > > tests yet)? > > +1 I added a bug issue https://code.google.com/p/chromium/issues/detail?id=246078 to make note of the missing tests that would have caught this bug. I'll work on them this afternoon. Do you want to put in the one liner fix first or wait for the automated tests to be combined with fix?
On 2013/06/03 02:41:42, calvinlo wrote: > On 2013/06/03 02:25:07, nhiroki wrote: > > looks good, too. > > > > On 2013/05/31 14:36:22, kinuko wrote: > > > looks good, can we / should we add a small test for > > > EnableOriginForTrackingChanges (and other *OriginForTrackingChanges that > have > > no > > > tests yet)? > > > > +1 > > I added a bug issue https://code.google.com/p/chromium/issues/detail?id=246078 > to make note of the missing tests that would have caught this bug. I'll work on > them this afternoon. > > Do you want to put in the one liner fix first or wait for the automated tests to > be combined with fix? Adding those small tests doesn't look very difficult, can you just add the tests in this change together?
On 2013/06/03 02:48:44, kinuko wrote: > On 2013/06/03 02:41:42, calvinlo wrote: > > On 2013/06/03 02:25:07, nhiroki wrote: > > > looks good, too. > > > > > > On 2013/05/31 14:36:22, kinuko wrote: > > > > looks good, can we / should we add a small test for > > > > EnableOriginForTrackingChanges (and other *OriginForTrackingChanges that > > have > > > no > > > > tests yet)? > > > > > > +1 > > > > I added a bug issue https://code.google.com/p/chromium/issues/detail?id=246078 > > to make note of the missing tests that would have caught this bug. I'll work > on > > them this afternoon. > > > > Do you want to put in the one liner fix first or wait for the automated tests > to > > be combined with fix? > > Adding those small tests doesn't look very difficult, can you just add the tests > in this change together? Tests added. Please take a look at the bug issue for the complete testing coverage map.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); Doesn't this change the current behavior? On the callsite (i.e. SyncFileSystemService::HandleExtensionEnabled) it appears we call EnableOriginForTrackingChanges unconditionally whenever we observe Extension enabled event.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); On 2013/06/03 08:48:33, kinuko wrote: > Doesn't this change the current behavior? > On the callsite (i.e. SyncFileSystemService::HandleExtensionEnabled) it appears > we call EnableOriginForTrackingChanges unconditionally whenever we observe > Extension enabled event. Based on Hiroki's notes on lines 461-468, NOTIFICATION_EXTENSION_ENABLED should only be called if the extension has already been disabled. The old code actually doesn't make sense to me anyway because if the origin wasn't in the disabled list and we return SYNC_STATUS_OK, I actually have no idea what the origin sync status is. i.e. What happened to it? Where did it go.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); On 2013/06/03 08:54:26, calvinlo wrote: > On 2013/06/03 08:48:33, kinuko wrote: > > Doesn't this change the current behavior? > > On the callsite (i.e. SyncFileSystemService::HandleExtensionEnabled) it > appears > > we call EnableOriginForTrackingChanges unconditionally whenever we observe > > Extension enabled event. > > Based on Hiroki's notes on lines 461-468, NOTIFICATION_EXTENSION_ENABLED should > only be called if the extension has already been disabled. The old code actually > doesn't make sense to me anyway because if the origin wasn't in the disabled > list and we return SYNC_STATUS_OK, I actually have no idea what the origin sync > status is. i.e. What happened to it? Where did it go. I mean, couldn't this method be called for apps/origins that have never used syncFS API, could it? We wouldn't be interested in showing statuses for such origins either.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); On 2013/06/03 08:54:26, calvinlo wrote: > On 2013/06/03 08:48:33, kinuko wrote: > > Doesn't this change the current behavior? > > On the callsite (i.e. SyncFileSystemService::HandleExtensionEnabled) it > appears > > we call EnableOriginForTrackingChanges unconditionally whenever we observe > > Extension enabled event. > > Based on Hiroki's notes on lines 461-468, NOTIFICATION_EXTENSION_ENABLED should > only be called if the extension has already been disabled. The old code actually > doesn't make sense to me anyway because if the origin wasn't in the disabled > list and we return SYNC_STATUS_OK, I actually have no idea what the origin sync > status is. i.e. What happened to it? Where did it go. I'm not sure what happened on the old code, too. Anyway I think this function can be called for unregistered (not enabled nor disabled) origin like below: 1. Open chrome 2. Install a syncfs app, and then disabled it immediately. 3. Start syncfs service (i.e. launch other syncfs app). 4. Enable the syncfs app. On #4 the app does not belong to the disabled origins, so hit the DCHECK. Can you try this?
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); On 2013/06/03 09:06:03, nhiroki wrote: > I'm not sure what happened on the old code, too. Sorry, in this case an app will be registered when launched.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:72: Just a question: Why did you add this here? https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:104: // Prints which counts don't match up if any. This comment would be somewhat verbose. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:110: // simply adding ASSERT_TRUE on the call to this function. s/ASSERT_TRUE/EXPECT_TRUE https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:167: const GURL origin1("chrome-extension://app1"); nit: "1" suffix looks unnecessary here and somewhere else since each test has only one origin. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:171: ASSERT_TRUE(VerifyOriginStatusCount(1u, 0u, 0u)); s/ASSERT_TRUE/EXPECT_TRUE/ ? https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:177: ASSERT_TRUE(VerifyOriginStatusCount(0u, 0u, 0u)); ditto.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:504: DCHECK(metadata_store_->IsOriginDisabled(origin)); On 2013/06/03 09:11:45, nhiroki wrote: > On 2013/06/03 09:06:03, nhiroki wrote: > > I'm not sure what happened on the old code, too. > > Sorry, in this case an app will be registered when launched. Sorry, I was under the impression that we only got events for extensions that declared SyncFS permission in their manifest. However I just did some testing and we really do get extension messages for every extension. I'll revert these lines and add a note similar to the one I just spotted on Line 536. I really wish we had an OriginManager to prevent the code from reaching this point however. It's a bit confusing to me that the disabled list check here is really being used as a check to see if the origin uses SyncFS.
https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:72: On 2013/06/03 09:41:44, nhiroki wrote: > Just a question: Why did you add this here? If I don't do this here, then DriveFileSyncService::Initialize() doesn't get activated until after I've tried to put in the first pending batch sync origin in my tests below at which time the DCHECK on L375 DidInitializeMetadataStore() gets hit. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:104: // Prints which counts don't match up if any. On 2013/06/03 09:41:44, nhiroki wrote: > This comment would be somewhat verbose. The reason I wrote this is because I wanted to make it clear why I'm doing both the EXPECT_EQ checks here and manually below again. Without doing this, I lose the line number where the call to VerifyOriginStatusCount() comes from making this function less useful. In the end I want it to print both which counts didn't match, and where the verify* call was made in the tests. Otherwise the expect just tells me the failure happened at line 105-107 in this function which isn't as useful. I thought about taking in a FROM_HERE macro into this call to get the line number but that seemed more cumbersome. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:110: // simply adding ASSERT_TRUE on the call to this function. On 2013/06/03 09:41:44, nhiroki wrote: > s/ASSERT_TRUE/EXPECT_TRUE I actually want this to be an assert. If this is wrong, the tests should immediately fail because it doesn't make sense to continue on. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:167: const GURL origin1("chrome-extension://app1"); On 2013/06/03 09:41:44, nhiroki wrote: > nit: "1" suffix looks unnecessary here and somewhere else since each test has > only one origin. Done. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:171: ASSERT_TRUE(VerifyOriginStatusCount(1u, 0u, 0u)); On 2013/06/03 09:41:44, nhiroki wrote: > s/ASSERT_TRUE/EXPECT_TRUE/ ? Replied with the reason above. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:177: ASSERT_TRUE(VerifyOriginStatusCount(0u, 0u, 0u)); On 2013/06/03 09:41:44, nhiroki wrote: > ditto. Replied with the reason above.
lgtm https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc (right): https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:104: // Prints which counts don't match up if any. On 2013/06/03 10:10:20, calvinlo wrote: > On 2013/06/03 09:41:44, nhiroki wrote: > > This comment would be somewhat verbose. > > The reason I wrote this is because I wanted to make it clear why I'm doing both > the EXPECT_EQ checks here and manually below again. Without doing this, I lose > the line number where the call to VerifyOriginStatusCount() comes from making > this function less useful. > > In the end I want it to print both which counts didn't match, and where the > verify* call was made in the tests. Otherwise the expect just tells me the > failure happened at line 105-107 in this function which isn't as useful. > > I thought about taking in a FROM_HERE macro into this call to get the line > number but that seemed more cumbersome. I know what you mean, and I think we could get it from comments on line 109-110. Anyway it's okay to put this here. https://codereview.chromium.org/16232019/diff/23001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc:110: // simply adding ASSERT_TRUE on the call to this function. On 2013/06/03 10:10:20, calvinlo wrote: > On 2013/06/03 09:41:44, nhiroki wrote: > > s/ASSERT_TRUE/EXPECT_TRUE > > I actually want this to be an assert. If this is wrong, the tests should > immediately fail because it doesn't make sense to continue on. Okay, please keep them.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calvinlo@chromium.org/16232019/20009
Message was sent while issue was closed.
Change committed as 203927 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
