Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12744008:
SyncFS: store disabled origins in DriveMetadataStore (Closed)
|
Created:
7 years, 9 months ago by nhiroki Modified:
7 years, 9 months ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSyncFS: store disabled origins in DriveMetadataStore
Store disabled origins when extensions are disabled. Current implementation stores origins only for installed and enabled apps (i.e. batch sync origins and incremental sync origins). Therefore we cannot access files and metadata related to installed but disabled apps.
This makes a difference between installing/uninstalling and enabling/disabling extensions.
- register an origin when an extension is launched for the first time.
- disable the origin when the extension is disabled (not unregister).
- enable the origin when the extension is enabled.
- unregister the origin when the extension is uninstalled.
BUG=184742
TEST=unit_test
TEST=manual test (launch, reload, restart, disable, enable and uninstall an app)
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189036
Patch Set 1 : fix #
Total comments: 11
Patch Set 2 : unit tests and review fix #
Total comments: 14
Patch Set 3 : review fix #Patch Set 4 : rebase #
Total comments: 28
Patch Set 5 : review fix #
Total comments: 2
Patch Set 6 : review fix #
Total comments: 2
Patch Set 7 : review fix #Patch Set 8 : fix #Patch Set 9 : rebase #Messages
Total messages: 27 (0 generated)
I know this is still rough, but since I'm afraid I'm going the wrong way and bringing a disaster, can you take an early look? I'm going to add some unit tests and do manual tests. Thanks!
Looks good. https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:519: std::map<GURL, std::string>::iterator found; How about doing like this, or making another helper function? found = batch_sync_origins_.find(origin); if (found != batch_sync_origins_) { resource_id = found->socend; batch_sync_origins_.erase(found); } https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.h (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.h:84: bool IsDisabledSyncOrigin(const GURL& origin) const; s/DisabledSyncOrigin/DisabledOrigin/ ? https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.h:95: void EnableSyncOrigin(const GURL& origin, s/EnableSyncOrigin/EnableOriginSync/, s/DisableSyncOrigin/DisableOriginSync/ ?
I think this is fine for now. However I'd really be interested in cleaning this up per my previous suggestions at some point as I myself end up using this class a lot and would like to see it simplified. Because I really need this for the uninstall patch I'm doing, I'm fine with Hiroki just putting in a bunch of TODOs for me, and I will be happy to clean this up later after the M27 branch cut. https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:111: SyncStatusCode UpdateEntry(const FileSystemURL& url, I don't think it's needed now at all, but I think this class would be a lot easier to work with if a lot of these functions were changed to just one general function. Something like UpdateSyncOrigin(GURL, SyncStatusEnum) because all that's essentially happening is that an origin is being moved from one list to another. And we can probably reduce the need to remember the names of all the sync_origin lists if we just made a Map<SyncStatusEnum, ResourceIDMap> If there's agreement to simplify the DriveMetaDatastore in this way, can we add a TODO for this? https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:199: std::string CreateKeyForDisabledSyncOrigin(const GURL& origin) { Same thing with these CreateKeyFor* functions. They can be combined into a single function that takes in a SyncStatusEnum type with a case switch to handle the different prefixes.
Updated, thanks! https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:111: SyncStatusCode UpdateEntry(const FileSystemURL& url, I wouldn't think there are many functions here, but I think it's worth thinking about refactoring them. tzik-san has a big reform plan for the whole SyncFS service, so you can do it with him? Anyway, I added TODO comments. Done. https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:199: std::string CreateKeyForDisabledSyncOrigin(const GURL& origin) { On 2013/03/15 03:37:48, calvinlo wrote: > Same thing with these CreateKeyFor* functions. They can be combined into a > single function that takes in a SyncStatusEnum type with a case switch to handle > the different prefixes. Done. https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:519: std::map<GURL, std::string>::iterator found; On 2013/03/14 12:50:54, tzik wrote: > How about doing like this, or making another helper function? > > found = batch_sync_origins_.find(origin); > if (found != batch_sync_origins_) { > resource_id = found->socend; > batch_sync_origins_.erase(found); > } Done. https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.h (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.h:84: bool IsDisabledSyncOrigin(const GURL& origin) const; On 2013/03/14 12:50:54, tzik wrote: > s/DisabledSyncOrigin/DisabledOrigin/ ? I mean to make it consistent with "IsBatchSyncOrigin" and "IsIncrementalSyncOrigin" since these are coordinative. WDYT? https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.h:95: void EnableSyncOrigin(const GURL& origin, On 2013/03/14 12:50:54, tzik wrote: > s/EnableSyncOrigin/EnableOriginSync/, > s/DisableSyncOrigin/DisableOriginSync/ ? Done.
Remaining tasks (just an implementation memo): - When an origin is disabled, drop MetadataStore entries related to the origin. - Add unit tests for DriveFileSyncService - Do tests
LGTM
https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.h (right): https://codereview.chromium.org/12744008/diff/14001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.h:84: bool IsDisabledSyncOrigin(const GURL& origin) const; On 2013/03/15 12:08:56, nhiroki wrote: > On 2013/03/14 12:50:54, tzik wrote: > > s/DisabledSyncOrigin/DisabledOrigin/ ? > > I mean to make it consistent with "IsBatchSyncOrigin" and > "IsIncrementalSyncOrigin" since these are coordinative. WDYT? I think they are for 'batch sync' and 'incremental sync', and I don't think they are necessarily matched here.
https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/drive_file_sync_service.cc:459: origin_to_changes_map_.erase(found); I'm afraid I'm lost here... do we call origin_to_changes_map_.erase(found) over and over? https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/drive_metadata_store.cc:117: // in just one map like "Map<SyncStatusEnum, ResourceIDMap>". Can we file a bug for this? https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/drive_metadata_store.cc:130: ResourceIDMap* disabled_sync_origins); Hmm, ok so we seem to use the term 'SyncOrigin' a lot in this class. Then the name 'EnableOriginSync' and 'DisableOriginSync' may look a bit inconsistent... and returning disabled ones for the method 'GetSyncOrigins' feels a bit weird. I feel we could just drop 'Sync' from most method names and can rename them to: UpdateSyncOriginAs... -> UpdateOriginAs...Sync GetSyncOrigins -> GetOrigins EnableOriginSync -> EnableOrigin DisableOriginSync -> DisableOrigin IsDisabledSyncOrigin -> IsOriginDisabled These would match better with following MetadataStore method names: MetadataStore::GetEnabledOrigins() MetadataStore::GetDisabledOrigins() wdyt? https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/drive_metadata_store.cc:501: DCHECK(!IsIncrementalSyncOrigin(origin)); Nit-picky comment: If we had a crash after an extension is disabled (and the status change is persisted in extension system) but before the changes are successfully written into the metadata store, this method may get called without meeting these conditions, am I correct? DCHECKs aren't hit in release build but if something goes wrong enabling the extension may keep failing in debug build? https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/drive_metadata_store.cc:522: DCHECK(!IsDisabledSyncOrigin(origin)); Ditto. https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/sync_file_system_service.cc:403: switch (type) { Can you add a comment about when each event is delivered? https://chromiumcodereview.appspot.com/12744008/diff/22001/chrome/browser/syn... chrome/browser/sync_file_system/sync_file_system_service.cc:471: base::Bind(&DidHandleOriginForExtensionEvent, type, app_origin)); Looking into the table in https://code.google.com/p/chromium/issues/detail?id=184742#c5, should we call local_file_service_->SetOriginEnabled(app_origin, true) here and remove HandleExtensionLoaded method? Also while you're there could you remove the following line in LocalFileSyncService::OriginChangeMap::SetOriginEnabled? IIUC looks like ENABLED could be called for origins that are not in the in-memory map (e.g. when an extension is enabled after chrome relaunch). line 85: DCHECK(ContainsKey(disabled_origins_, origin)); This map is only there to suppress local sync notifications, so I think just removing this line'd work (or tests would tell).
Thank you for reviewing! Updated. I made sure that this works well in my local. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:459: origin_to_changes_map_.erase(found); On 2013/03/16 23:16:51, kinuko wrote: > I'm afraid I'm lost here... do we call origin_to_changes_map_.erase(found) over > and over? Thanks for pointing out! I mean to erase pending changes... Fixed. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_metadata_store.cc (right): https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:117: // in just one map like "Map<SyncStatusEnum, ResourceIDMap>". On 2013/03/16 23:16:51, kinuko wrote: > Can we file a bug for this? Done. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:130: ResourceIDMap* disabled_sync_origins); On 2013/03/16 23:16:51, kinuko wrote: > Hmm, ok so we seem to use the term 'SyncOrigin' a lot in this class. Then the > name 'EnableOriginSync' and 'DisableOriginSync' may look a bit inconsistent... > and returning disabled ones for the method 'GetSyncOrigins' feels a bit weird. > > I feel we could just drop 'Sync' from most method names and can rename them to: > > UpdateSyncOriginAs... -> UpdateOriginAs...Sync > GetSyncOrigins -> GetOrigins > EnableOriginSync -> EnableOrigin > DisableOriginSync -> DisableOrigin > IsDisabledSyncOrigin -> IsOriginDisabled > > These would match better with following MetadataStore method names: > > MetadataStore::GetEnabledOrigins() > MetadataStore::GetDisabledOrigins() > > wdyt? Looks good. (hopefully) I replaced them without any omission. Done. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:501: DCHECK(!IsIncrementalSyncOrigin(origin)); On 2013/03/16 23:16:51, kinuko wrote: > Nit-picky comment: If we had a crash after an extension is disabled (and the > status change is persisted in extension system) but before the changes are > successfully written into the metadata store, this method may get called without > meeting these conditions, am I correct? DCHECKs aren't hit in release build but > if something goes wrong enabling the extension may keep failing in debug build? You are right. I think it's not needed to check them strictly here. I changed these checks to simple if-statements. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_metadata_store.cc:522: DCHECK(!IsDisabledSyncOrigin(origin)); On 2013/03/16 23:16:51, kinuko wrote: > Ditto. Done. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:403: switch (type) { On 2013/03/16 23:16:51, kinuko wrote: > Can you add a comment about when each event is delivered? Done. https://codereview.chromium.org/12744008/diff/22001/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:471: base::Bind(&DidHandleOriginForExtensionEvent, type, app_origin)); On 2013/03/16 23:16:51, kinuko wrote: > Looking into the table in > https://code.google.com/p/chromium/issues/detail?id=184742#c5, should we call > local_file_service_->SetOriginEnabled(app_origin, true) here and remove > HandleExtensionLoaded method? > > Also while you're there could you remove the following line in > LocalFileSyncService::OriginChangeMap::SetOriginEnabled? IIUC looks like > ENABLED could be called for origins that are not in the in-memory map (e.g. when > an extension is enabled after chrome relaunch). > > line 85: DCHECK(ContainsKey(disabled_origins_, origin)); > > This map is only there to suppress local sync notifications, so I think just > removing this line'd work (or tests would tell). Removed HandleExtensionLoaded and DCHECK from LocalFileSyncService. Done.
https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:431: const SyncStatusCallback& callback) { I think we can likely early-return here if the origin's not in disabled list? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:448: const SyncStatusCallback& callback) { ditto. I guess we can likely early-return here if the origin's not in incremental_sync or batch_sync? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:987: } Hmm, having two loops with similar body looks a bit awkward.. but I hope we can clean this up when we migrate to use type enum. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1042: Can we have another DCHECK to check if the origin's not disabled? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { If we came here from RegisterOrigin I think we need to re-enable the origin. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1096: if (!metadata_store_->IsOriginDisabled(origin)) { We've gotten token in GetDriveDirectoryForOrigin and there we've checked if origin is disabled or not, haven't we? Could this happen? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1175: !metadata_store_->IsOriginDisabled(origin) && Could this (origin is batch-sync and disabled) happen? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:2071: !metadata_store_->IsOriginDisabled(url.origin()) && ditto. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/local_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/local_file_sync_service.cc:91: } nit: can you also remove these { } ? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:61: DCHECK(chrome::NOTIFICATION_EXTENSION_UNLOADED == type); type == UNLOADED || type == ENABLED? https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:63: (chrome::NOTIFICATION_EXTENSION_UNLOADED == type) ? "UNLOAD" : "LOAD"; "LOAD" -> "ENABLE" ?
Updated, thanks! https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:431: const SyncStatusCallback& callback) { On 2013/03/18 18:09:43, kinuko wrote: > I think we can likely early-return here if the origin's not in disabled list? Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:448: const SyncStatusCallback& callback) { On 2013/03/18 18:09:43, kinuko wrote: > ditto. I guess we can likely early-return here if the origin's not in > incremental_sync or batch_sync? Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:987: } On 2013/03/18 18:09:43, kinuko wrote: > Hmm, having two loops with similar body looks a bit awkward.. but I hope we can > clean this up when we migrate to use type enum. Okay, for now just add TODO comment. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1042: On 2013/03/18 18:09:43, kinuko wrote: > Can we have another DCHECK to check if the origin's not disabled? Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/18 18:09:43, kinuko wrote: > If we came here from RegisterOrigin I think we need to re-enable the origin. hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to any origin maps (batch, incremental and disabled) yet, so I think we can pass here. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1096: if (!metadata_store_->IsOriginDisabled(origin)) { On 2013/03/18 18:09:43, kinuko wrote: > We've gotten token in GetDriveDirectoryForOrigin and there we've checked if > origin is disabled or not, haven't we? Could this happen? Yes, we've checked it on line 1067, so we don't have to do here. Removed. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1175: !metadata_store_->IsOriginDisabled(origin) && On 2013/03/18 18:09:43, kinuko wrote: > Could this (origin is batch-sync and disabled) happen? I thought this may happen in situations like you mentioned... "If we had a crash after an extension is disabled (and the status change is persisted in extension system) but before the changes are successfully written into the metadata store, ..." , but now I'm thinking this never happen since enabled(batch or incremental)/disabled are completely toggled by EnableOrigin()/DisableOrigin() (i.e. there is no intermediate state), so I think we don't have to check it here. Removed. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:2071: !metadata_store_->IsOriginDisabled(url.origin()) && On 2013/03/18 18:09:43, kinuko wrote: > ditto. Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/local_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/local_file_sync_service.cc:91: } On 2013/03/18 18:09:43, kinuko wrote: > nit: can you also remove these { } ? Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/sync_file_system_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:61: DCHECK(chrome::NOTIFICATION_EXTENSION_UNLOADED == type); On 2013/03/18 18:09:43, kinuko wrote: > type == UNLOADED || type == ENABLED? Done. https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/sync_file_system_service.cc:63: (chrome::NOTIFICATION_EXTENSION_UNLOADED == type) ? "UNLOAD" : "LOAD"; On 2013/03/18 18:09:43, kinuko wrote: > "LOAD" -> "ENABLE" ? Done.
https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/19 02:35:42, nhiroki wrote: > On 2013/03/18 18:09:43, kinuko wrote: > > If we came here from RegisterOrigin I think we need to re-enable the origin. > > hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to any > origin maps (batch, incremental and disabled) yet, so I think we can pass here. That sounds right... wait, couldn't we be in a situation like: an app is disabled, and then enabled, chrome crashes before we change the db, and the app is launched after restart https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1175: !metadata_store_->IsOriginDisabled(origin) && On 2013/03/19 02:35:42, nhiroki wrote: > On 2013/03/18 18:09:43, kinuko wrote: > > Could this (origin is batch-sync and disabled) happen? > > I thought this may happen in situations like you mentioned... > > "If we had a crash after an extension is disabled (and the > status change is persisted in extension system) but before the changes are > successfully written into the metadata store, ..." > > , but now I'm thinking this never happen since enabled(batch or > incremental)/disabled are completely toggled by EnableOrigin()/DisableOrigin() > (i.e. there is no intermediate state), so I think we don't have to check it > here. > > Removed. Yup, as far as I understand the only possible inconsistency we need to care is the state inconsistency between the extension service and our metadata store.
https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/19 02:44:44, kinuko wrote: > On 2013/03/19 02:35:42, nhiroki wrote: > > On 2013/03/18 18:09:43, kinuko wrote: > > > If we came here from RegisterOrigin I think we need to re-enable the origin. > > > > hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to any > > origin maps (batch, incremental and disabled) yet, so I think we can pass > here. > > That sounds right... wait, couldn't we be in a situation like: an app is > disabled, and then enabled, chrome crashes before we change the db, and the app > is launched after restart > I see... in your situation, an origin is disabled until an app is reloaded or disabled/enabled. I'm not sure whether a true-positive case (extension and origin are disabled) can reach here. If such case exists, I think we cannot enable an origin easily (we need to know an actual extension state?). Anyway, I'll take a look into it (sorry, I won't be able to use a network a few hours.).
https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/19 03:43:52, nhiroki wrote: > On 2013/03/19 02:44:44, kinuko wrote: > > On 2013/03/19 02:35:42, nhiroki wrote: > > > On 2013/03/18 18:09:43, kinuko wrote: > > > > If we came here from RegisterOrigin I think we need to re-enable the > origin. > > > > > > hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to any > > > origin maps (batch, incremental and disabled) yet, so I think we can pass > > here. > > > > That sounds right... wait, couldn't we be in a situation like: an app is > > disabled, and then enabled, chrome crashes before we change the db, and the > app > > is launched after restart > > > > I see... in your situation, an origin is disabled until an app is reloaded or > disabled/enabled. Nope, I just realized that we do perform the enabled/disabled check at startup in UpdateRegisteredOrigins, so I guess we don't need to worry about this case. > I'm not sure whether a true-positive case (extension and origin are disabled) > can reach here. If such case exists, I think we cannot enable an origin easily > (we need to know an actual extension state?). > > Anyway, I'll take a look into it (sorry, I won't be able to use a network a few > hours.).
ok, let me stamp this lgtm. (It's great if tzik@ could take another look too)
https://codereview.chromium.org/12744008/diff/50001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/50001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:470: } nit: it may look nicer if we define a sub method (like ForgetRemoteChangesForOrigin) to do this and call the method here and in UnregsterOriginForTrackingChanges.
Updated https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/19 03:51:39, kinuko wrote: > On 2013/03/19 03:43:52, nhiroki wrote: > > On 2013/03/19 02:44:44, kinuko wrote: > > > On 2013/03/19 02:35:42, nhiroki wrote: > > > > On 2013/03/18 18:09:43, kinuko wrote: > > > > > If we came here from RegisterOrigin I think we need to re-enable the > > origin. > > > > > > > > hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to > any > > > > origin maps (batch, incremental and disabled) yet, so I think we can pass > > > here. > > > > > > That sounds right... wait, couldn't we be in a situation like: an app is > > > disabled, and then enabled, chrome crashes before we change the db, and the > > app > > > is launched after restart > > > > > > > I see... in your situation, an origin is disabled until an app is reloaded or > > disabled/enabled. > > Nope, I just realized that we do perform the enabled/disabled check at startup > in UpdateRegisteredOrigins, so I guess we don't need to worry about this case. > > > I'm not sure whether a true-positive case (extension and origin are disabled) > > can reach here. If such case exists, I think we cannot enable an origin easily > > (we need to know an actual extension state?). > > > > Anyway, I'll take a look into it (sorry, I won't be able to use a network a > few > > hours.). > Thanks! Done.
lgtm https://codereview.chromium.org/12744008/diff/55001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/55001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:956: if (extension_service->GetInstalledExtension(extension_id) == NULL) { s/== NULL// ?
Updated, thanks! https://codereview.chromium.org/12744008/diff/50001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/50001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:470: } On 2013/03/19 04:01:49, kinuko wrote: > nit: it may look nicer if we define a sub method (like > ForgetRemoteChangesForOrigin) to do this and call the method here and in > UnregsterOriginForTrackingChanges. Added TODO comment. https://codereview.chromium.org/12744008/diff/55001/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/55001/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:956: if (extension_service->GetInstalledExtension(extension_id) == NULL) { On 2013/03/19 09:24:03, tzik wrote: > s/== NULL// ? Done.
https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/12744008/diff/43002/chrome/browser/sync_file_... chrome/browser/sync_file_system/drive_file_sync_service.cc:1067: metadata_store_->IsOriginDisabled(origin)) { On 2013/03/19 08:47:38, nhiroki wrote: > On 2013/03/19 03:51:39, kinuko wrote: > > On 2013/03/19 03:43:52, nhiroki wrote: > > > On 2013/03/19 02:44:44, kinuko wrote: > > > > On 2013/03/19 02:35:42, nhiroki wrote: > > > > > On 2013/03/18 18:09:43, kinuko wrote: > > > > > > If we came here from RegisterOrigin I think we need to re-enable the > > > origin. > > > > > > > > > > hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to > > any > > > > > origin maps (batch, incremental and disabled) yet, so I think we can > pass > > > > here. > > > > > > > > That sounds right... wait, couldn't we be in a situation like: an app is > > > > disabled, and then enabled, chrome crashes before we change the db, and > the > > > app > > > > is launched after restart > > > > > > > > > > I see... in your situation, an origin is disabled until an app is reloaded > or > > > disabled/enabled. > > > > Nope, I just realized that we do perform the enabled/disabled check at startup > > in UpdateRegisteredOrigins, so I guess we don't need to worry about this case. > > > > > I'm not sure whether a true-positive case (extension and origin are > disabled) > > > can reach here. If such case exists, I think we cannot enable an origin > easily > > > (we need to know an actual extension state?). > > > > > > Anyway, I'll take a look into it (sorry, I won't be able to use a network a > > few > > > hours.). > > > > Thanks! Done. I meant I don't think we need to worry about disabled origin case here either, since if it's disabled in metadata but extension service says it's enabled we enable them in UpdateRegisteredOrigins. Can you check the behavior again?
On 2013/03/19 09:56:30, kinuko wrote: > > I meant I don't think we need to worry about disabled origin case here either, > since if it's disabled in metadata but extension service says it's enabled we > enable them in UpdateRegisteredOrigins. Can you check the behavior again? Sorry, I misread... I made sure that disabled origin doesn't reach here, so removed again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/12744008/47012
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/12744008/75002
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/12744008/75002
Message was sent while issue was closed.
Change committed as 189036 |
