Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790483002:
Fixes History page keeps on loading when signed in with Passphrase (Closed)
|
Created:
3 years, 8 months ago by ramyasharma Modified:
3 years, 8 months ago CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sync-reviews_chromium.org, sdefresne+watch_chromium.org, skym, gracie Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes History page keeps on loading when signed in with Passphrase
enabled account.
This bug was exposed as a result of https://codereview.chromium.org/2589313002.
There are two types of data types for sync:
Preferred type - The sync service will try to make active exactly these types.
Active: a preferred datatype that is actively being synchronized.
On iOS sync service, when fetching the current state of a data type, we were
looking up preferred data type, instead of active. Hence even though the
passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this
stage, it was being returned as true, since it was still a part of the preferred
data types.
TEST=Test with an account with secondary passphrase. The history view must
only load local history when passphrase is enabled. Otherwise load synced
history from across devices enabled
BUG=679347
TBR=skym@chromium.org
Review-Url: https://codereview.chromium.org/2790483002
Cr-Commit-Position: refs/heads/master@{#461642}
Committed: https://chromium.googlesource.com/chromium/src/+/54106f055fcaed85a4cfd34d749eaf7ebf107137
Patch Set 1 : a #
Total comments: 4
Patch Set 2 : a #Patch Set 3 : a #
Messages
Total messages: 51 (39 generated)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ramyasharma@chromium.org changed reviewers: + pinkerton@chromium.org, sczs@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for tackling this ramyasharma@ lgtm with a couple "TODO" format fixes https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... ios/chrome/browser/ui/history/history_collection_view_controller.mm:333: // TODO(ramyasharma): Remove this check when sync bug is fixed. Please use format TODO(crbug.com/####): as in https://sites.google.com/a/google.com/bling/engineering/workflow/style-guide?... https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... File ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm (right): https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm:124: // TODO(ramyasharma): Remove dependency on SecondaryPassphrase once Same as the other TODO
I'm not sure we can fix the issue where history isn't synced if there is a passphrase (due to the very nature of how it works), but we should have it not spin endlessly and return nothing immediately. Shouldn't the fix be in whatever service is telling us to keep waiting for data (that will never come)? That would then help all platforms (such as android who is right now implementing native history) instead of just patching for iOS. Can you check if ChromeAndroid has this problem too?
On 2017/03/31 16:10:44, pink wrote: > I'm not sure we can fix the issue where history isn't synced if there is a > passphrase (due to the very nature of how it works), but we should have it not > spin endlessly and return nothing immediately. > > Shouldn't the fix be in whatever service is telling us to keep waiting for data > (that will never come)? That would then help all platforms (such as android who > is right now implementing native history) instead of just patching for iOS. Can > you check if ChromeAndroid has this problem too? Thank sczs@ and pink. I had an offline discussion about this with skym@, and the issue may be in the iOS sync_setup_service. Hence Android and other platforms are not affected by this. iOS we're calling ProfileSyncService::GetPreferedDataTypes() https://cs.chromium.org/chromium/src/ios/chrome/browser/sync/sync_setup_servi... on desktop we call ProfileSyncService::GetActiveDataTypes https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_servi... While we disable HISTORY_DELETE_DIRECTIVES, when passphrases are enabled since we query for GetPreferedDataTypes, it is returned enabled, so that it isn't active, but it is still "preferred". I will look into this approach on Monday and see if this is a more permanent fix, than the patch fix in the view controller.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/01 01:13:49, ramyasharma wrote: > On 2017/03/31 16:10:44, pink wrote: > > I'm not sure we can fix the issue where history isn't synced if there is a > > passphrase (due to the very nature of how it works), but we should have it not > > spin endlessly and return nothing immediately. > > > > Shouldn't the fix be in whatever service is telling us to keep waiting for > data > > (that will never come)? That would then help all platforms (such as android > who > > is right now implementing native history) instead of just patching for iOS. > Can > > you check if ChromeAndroid has this problem too? > > Thank sczs@ and pink. I had an offline discussion about this with skym@, and the > issue may be in the iOS sync_setup_service. Hence Android and other platforms > are not affected by this. iOS we're calling > ProfileSyncService::GetPreferedDataTypes() > https://cs.chromium.org/chromium/src/ios/chrome/browser/sync/sync_setup_servi... > on desktop we call ProfileSyncService::GetActiveDataTypes > https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_servi... > While we disable HISTORY_DELETE_DIRECTIVES, when passphrases are enabled > since we query for GetPreferedDataTypes, it is returned enabled, so that it > isn't active, > but it is still "preferred". > I will look into this approach on Monday and see if this is a more permanent > fix, > than the patch fix in the view controller. PTAL everyone? I have made the change in sync_setup_service, where GetEnabledDataTypes looks up GetActiveDataTypes and not GetPreferedDataTypes, to get the correct enabled state.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
pkl@chromium.org changed reviewers: + pkl@chromium.org
LGTM, but please update the CL description since it is still talking about the temporary solution. The latest patch is the more "permanent" solution, isn't it? A TEST= line will help the testers as well. Please add TEST=Test with an account with secondary passphrase.
The CQ bit was checked by ramyasharma@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This is temporary fix, where in we check if the explicit user passphrase is enabled, and if yes, then we simply load the local device history only. This bug was exposed as a result of https://codereview.chromium.org/2589313002. Long term fix would be to fix the sync bug, so that History is synced when the passphrase is enabled. This is a quick fix to improve user experience, and not load indefinitely. BUG=679347 ========== to ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices. enabled BUG=679347 ==========
On 2017/04/03 20:20:03, pklpkl wrote: > LGTM, but please update the CL description since it is > still talking about the temporary solution. The latest > patch is the more "permanent" solution, isn't it? > > A TEST= line will help the testers as well. Please add > TEST=Test with an account with secondary passphrase. Thanks pink. Done.
Description was changed from ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices. enabled BUG=679347 ========== to ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 ==========
ramyasharma@chromium.org changed reviewers: + skym@chromium.org
Adding skym@ for sync. https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... File ios/chrome/browser/ui/history/history_collection_view_controller.mm (right): https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... ios/chrome/browser/ui/history/history_collection_view_controller.mm:333: // TODO(ramyasharma): Remove this check when sync bug is fixed. On 2017/03/31 15:58:01, sczs wrote: > Please use format TODO(crbug.com/####): as in > https://sites.google.com/a/google.com/bling/engineering/workflow/style-guide?... Done. https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... File ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm (right): https://codereview.chromium.org/2790483002/diff/80001/ios/chrome/browser/ui/h... ios/chrome/browser/ui/history/history_collection_view_controller_unittest.mm:124: // TODO(ramyasharma): Remove dependency on SecondaryPassphrase once On 2017/03/31 15:58:01, sczs wrote: > Same as the other TODO Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 ========== to ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 TBR=skym@chromium.org ==========
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sczs@chromium.org, pinkerton@chromium.org, pkl@chromium.org Link to the patchset: https://codereview.chromium.org/2790483002/#ps120001 (title: "a")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491285659672080,
"parent_rev": "976a6d5640fe93c23e6aec62d2bed2230a8fdc9b", "commit_rev":
"54106f055fcaed85a4cfd34d749eaf7ebf107137"}
Message was sent while issue was closed.
Description was changed from ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 TBR=skym@chromium.org ========== to ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 TBR=skym@chromium.org Review-Url: https://codereview.chromium.org/2790483002 Cr-Commit-Position: refs/heads/master@{#461642} Committed: https://chromium.googlesource.com/chromium/src/+/54106f055fcaed85a4cfd34d749e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001) as https://chromium.googlesource.com/chromium/src/+/54106f055fcaed85a4cfd34d749e...
Message was sent while issue was closed.
sync change lgtm!
Message was sent while issue was closed.
Description was changed from ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 TBR=skym@chromium.org Review-Url: https://codereview.chromium.org/2790483002 Cr-Commit-Position: refs/heads/master@{#461642} Committed: https://chromium.googlesource.com/chromium/src/+/54106f055fcaed85a4cfd34d749e... ========== to ========== Fixes History page keeps on loading when signed in with Passphrase enabled account. This bug was exposed as a result of https://codereview.chromium.org/2589313002. There are two types of data types for sync: Preferred type - The sync service will try to make active exactly these types. Active: a preferred datatype that is actively being synchronized. On iOS sync service, when fetching the current state of a data type, we were looking up preferred data type, instead of active. Hence even though the passphrase is enabled, and HISTORY_DELETE_DIRECTIVES is disabled in this stage, it was being returned as true, since it was still a part of the preferred data types. TEST=Test with an account with secondary passphrase. The history view must only load local history when passphrase is enabled. Otherwise load synced history from across devices enabled BUG=679347 TBR=skym@chromium.org Review-Url: https://codereview.chromium.org/2790483002 Cr-Commit-Position: refs/heads/master@{#461642} Committed: https://chromium.googlesource.com/chromium/src/+/54106f055fcaed85a4cfd34d749e... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
