Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923013006:
[Extensions Sync] Don't apply sync data for unsyncable extensions (Closed)
Description[Extensions Sync] Don't apply sync data for unsyncable extensions
Some extensions are considered "unsyncable" - this includes extensions
that are installed by default (since we don't want to push them to
propogate everywhere). However, we weren't checking if an extension
was syncable before applying the sync data from the server. This meant
that it was possible to get into a bad state where, if a user has an
extension that is default installed on one machine, but not another, the
extension would be synced down, but never updated. In one case, this
can result in the extension always becoming disabled.
Fix this by checking for if an extension is syncable before applying
sync data, and add a couple of tests.
BUG=731824
Review-Url: https://codereview.chromium.org/2923013006
Cr-Commit-Position: refs/heads/master@{#479039}
Committed: https://chromium.googlesource.com/chromium/src/+/2f1ed4c9530785eab233370a9e8429451d987515
Patch Set 1 #Patch Set 2 : testfix #Patch Set 3 : add bug number #
Total comments: 11
Patch Set 4 : karandeepb's #
Total comments: 6
Patch Set 5 : lazyboy's #
Messages
Total messages: 31 (25 generated)
The CQ bit was checked by rdevlin.cronin@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@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.
Description was changed from ========== syncfix BUG= ========== to ========== [Extensions Sync] Don't apply sync data for unsyncable extensions Some extensions are considered "unsyncable" - this includes extensions that are installed by default (since we don't want to push them to propogate everywhere). However, we weren't checking if an extension was syncable before applying the sync data from the server. This meant that it was possible to get into a bad state where, if a user has an extension that is default installed on one machine, but not another, the extension would be synced down, but never updated. In one case, this can result in the extension always becoming disabled. Fix this by checking for if an extension is syncable before applying sync data, and add a couple of tests. BUG=731824 ==========
The CQ bit was checked by rdevlin.cronin@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.
rdevlin.cronin@chromium.org changed reviewers: + karandeepb@chromium.org, lazyboy@chromium.org
Hey folks, mind taking a look? https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (left): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1394: not sure how these crept in, will remove the changes on the next upload.
LGTM with nits. https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/chrome_test_extension_loader.cc (left): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/chrome_test_extension_loader.cc:121: base::FilePath* pem_path_to_use = &fallback_pem_path; Isn't this also correct? This change is just for more clarity? https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/chrome_test_extension_loader.cc:107: if (!temp_dir_.CreateUniqueTempDir()) { How did this work before w/o this? https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:359: EXPECT_EQ(kExtensionId, extension->id()); This seems redundant. Same below. https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:363: new syncer::FakeSyncChangeProcessor; optional nit: Create a unique pointer and then take a weak reference for use in this function. Also same below. https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1519: "http://localhost/autoupdate/updates.xml"); This seems unrelated. I am assuming you also plan to remove this?
The CQ bit was checked by rdevlin.cronin@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.
lgtm https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:372: ExtensionSyncData disable_extension( This pattern: ExtensionSyncData ... SyncChangeList list extension_sync_service()->ProcessSyncChanges(.., list) seems to be repeating enough times to pull this into a method of this test class. (It is also adding noise because of long list of params, mostly bools) https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:375: SyncChangeList enable_list( nit: should this be called disable_list (and the one on line 417 enable_list)? https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:415: ExtensionSyncData enable_extension(*extension, true, 0, false, false, 0->DISABLE_NONE?
The CQ bit was checked by rdevlin.cronin@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...
https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/chrome_test_extension_loader.cc (left): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/chrome_test_extension_loader.cc:121: base::FilePath* pem_path_to_use = &fallback_pem_path; On 2017/06/09 22:53:36, karandeepb wrote: > Isn't this also correct? This change is just for more clarity? It's actually not correct - if the path_to_use isn't empty, the extension creator looks for it (and in this case, we assert it doesn't exist). https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/chrome_test_extension_loader.cc (right): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/chrome_test_extension_loader.cc:107: if (!temp_dir_.CreateUniqueTempDir()) { On 2017/06/09 22:53:36, karandeepb wrote: > How did this work before w/o this? It didn't - turns out, we never exercised this code. https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:359: EXPECT_EQ(kExtensionId, extension->id()); On 2017/06/09 22:53:36, karandeepb wrote: > This seems redundant. Same below. Whoops! This expect was from an earlier version. Definitely unnecessary now. :) Thanks for catching this! https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:363: new syncer::FakeSyncChangeProcessor; On 2017/06/09 22:53:36, karandeepb wrote: > optional nit: Create a unique pointer and then take a weak reference for use in > this function. Also same below. Done. https://codereview.chromium.org/2923013006/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:1519: "http://localhost/autoupdate/updates.xml"); On 2017/06/09 22:53:36, karandeepb wrote: > This seems unrelated. I am assuming you also plan to remove this? Actually, this one's important. We only sync extensions that update from the webstore, so we need to pretend that this one does. I've added a comment explaining this. https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:372: ExtensionSyncData disable_extension( On 2017/06/12 20:59:55, lazyboy wrote: > This pattern: > ExtensionSyncData ... > SyncChangeList list > extension_sync_service()->ProcessSyncChanges(.., list) > seems to be repeating enough times to pull this into a method of this test > class. (It is also adding noise because of long list of params, mostly bools) Good idea! Done. https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:375: SyncChangeList enable_list( On 2017/06/12 20:59:55, lazyboy wrote: > nit: should this be called disable_list (and the one on line 417 enable_list)? Yes, but moot with your suggestion above. :) https://codereview.chromium.org/2923013006/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_service_sync_unittest.cc:415: ExtensionSyncData enable_extension(*extension, true, 0, false, false, On 2017/06/12 20:59:55, lazyboy wrote: > 0->DISABLE_NONE? Done (in the EnableExtensionFromSync method)
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from karandeepb@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2923013006/#ps80001 (title: "lazyboy's")
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": 80001, "attempt_start_ts": 1497370499866660,
"parent_rev": "3f01dde0d132da4cdab03d5ef6b22dc959629276", "commit_rev":
"2f1ed4c9530785eab233370a9e8429451d987515"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions Sync] Don't apply sync data for unsyncable extensions Some extensions are considered "unsyncable" - this includes extensions that are installed by default (since we don't want to push them to propogate everywhere). However, we weren't checking if an extension was syncable before applying the sync data from the server. This meant that it was possible to get into a bad state where, if a user has an extension that is default installed on one machine, but not another, the extension would be synced down, but never updated. In one case, this can result in the extension always becoming disabled. Fix this by checking for if an extension is syncable before applying sync data, and add a couple of tests. BUG=731824 ========== to ========== [Extensions Sync] Don't apply sync data for unsyncable extensions Some extensions are considered "unsyncable" - this includes extensions that are installed by default (since we don't want to push them to propogate everywhere). However, we weren't checking if an extension was syncable before applying the sync data from the server. This meant that it was possible to get into a bad state where, if a user has an extension that is default installed on one machine, but not another, the extension would be synced down, but never updated. In one case, this can result in the extension always becoming disabled. Fix this by checking for if an extension is syncable before applying sync data, and add a couple of tests. BUG=731824 Review-Url: https://codereview.chromium.org/2923013006 Cr-Commit-Position: refs/heads/master@{#479039} Committed: https://chromium.googlesource.com/chromium/src/+/2f1ed4c9530785eab233370a9e84... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2f1ed4c9530785eab233370a9e84... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
