Issue metadata
Sign in to add a comment
|
Preferences API incorrectly firing events in incognito contexts |
||||||||||||||||||||||
Issue descriptionRevision 1324ea47d5870af00fccd488d06bb1bf68d00947 accidentally changed the criteria under which we dispatch an event to a listening extension for preference changes. The desired behavior is that: // If the extension is in incognito split mode, // a) incognito pref changes are visible only to the incognito tabs // b) regular pref changes are visible only to the incognito tabs if the // incognito pref has not already been set [1] Revision 1324ea47d5870af00fccd488d06bb1bf68d00947 made these events dispatch in incognito contexts regardless of if the incognito pref had been set. [2] In theory, we have a test that *should* have caught this ('changeDefaultOnly' from ExtensionPreferenceApiTest.OnChangeSplit) [3], but the test is racy, and thus flaky [4]. [1] https://chromium.googlesource.com/chromium/src/+/49ee788d3bf82d80613edb98bc1f5d1e4c5e0e93/chrome/browser/extensions/api/preference/preference_helpers.cc#111 [2] https://chromium.googlesource.com/chromium/src/+/1324ea47d5870af00fccd488d06bb1bf68d00947%5E%21/#F1 [3] https://chromium.googlesource.com/chromium/src/+/93f209d5e8adbdce5a229385752eec2570cc4713/chrome/test/data/extensions/api_test/preference/onchange_split/test.js#101 [4] http://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyMwsSBUZsYWtlIihFeHRlbnNpb25QcmVmZXJlbmNlQXBpVGVzdC5PbkNoYW5nZVNwbGl0DA Unfortunately, since revision 1324ea47d5870af00fccd488d06bb1bf68d00947 was merged to M65 (in revision 39d21bd986d55d316f8374ad7d6a5667f817885f), we'll have to merge this fix, as well.
,
Feb 8 2018
,
Feb 8 2018
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 9 2018
Before we approve merge to M65, could you pls confirm followings? Is this M65 regression and critical to merge? Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge? Any other imp details to justify the merge. Please note M65 is already promoted to Beta so merge bar is very high. Thank you.
,
Feb 9 2018
> Is this M65 regression and critical to merge? It is a regression that was introduced in M65, and fixed in M66. I'd like to merge to M65 to avoid the regression ever hitting stable. It is not absolutely critical (it is not a security bug, nor a crasher), but we don't know how many items this may affect. > Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge? It's baked for a bit over a day, but we can wait 'til tomorrow to give it a bit more time. It does have automated tests. > Any other imp details to justify the merge. The production code change is extremely small [1]. This boils down to a single character change (removing an ! operator), so merging it back to avoid issues ever being present on stable is desirable IMO. [1] https://chromium.googlesource.com/chromium/src.git/+/28d0c01e6b4cdfc5f144fafb8d480cb5043fdafb%5E%21/#F1
,
Feb 9 2018
Thank you rdevlin.cronin@. Approving merge to M65 branch 3325 based on comment #5. Pls merge tomorrow after little more baking in canary.
,
Feb 9 2018
Will do, thanks! :)
,
Feb 9 2018
Thank you!
,
Feb 9 2018
The NextAction date has arrived: 2018-02-09
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/67cafaf4f1076a3addd71334f19035a5f92f5748 commit 67cafaf4f1076a3addd71334f19035a5f92f5748 Author: Devlin Cronin <rdevlin.cronin@chromium.org> Date: Fri Feb 09 18:28:44 2018 [M65 Merge] Fix incognito event dispatching for the preferences API A preference changed event should be fired to an incognito extension if and only if the incognito preference has not already been set. This regressed recently, and the tests that exercised it were racy. The raciness came from callers using pass(sendMessage(...)) as an API callback, when in fact they should have been using pass(function() { sendMessage(...) }). The former ends up sending the message immediately, thus not properly waiting for the original API call to complete. Fix the bug, as well as fixing the flakiness in the tests. TBR=rdevlin.cronin@chromium.org (cherry picked from commit 28d0c01e6b4cdfc5f144fafb8d480cb5043fdafb) Bug: 810048 Change-Id: If94f9c7fb17a4c7544f7e42a4f83d225a83064cb Reviewed-on: https://chromium-review.googlesource.com/907115 Reviewed-by: Toni Barzic <tbarzic@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#535189} Reviewed-on: https://chromium-review.googlesource.com/911898 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#408} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/67cafaf4f1076a3addd71334f19035a5f92f5748/chrome/browser/extensions/api/preference/preference_apitest.cc [modify] https://crrev.com/67cafaf4f1076a3addd71334f19035a5f92f5748/chrome/browser/extensions/api/preference/preference_helpers.cc [modify] https://crrev.com/67cafaf4f1076a3addd71334f19035a5f92f5748/chrome/test/data/extensions/api_test/preference/onchange_split/test.js |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Feb 7 2018