Issue metadata
Sign in to add a comment
|
Variations are not applied from master_preferences during first run
Reported by
bor...@gmail.com,
Nov 21
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 YaBrowser/18.11.1.282 (beta) Yowser/2.5 Safari/537.36 Steps to reproduce the problem: 1. You need browser with master_preferences filed with variations_compressed_seed 2. Install the browser 3. Check chrome://version there are no variations What is the expected behavior? 3. Check chrome://version there are variations What went wrong? Variations have to be moved from master_preferences to Local State during first run before they is read from Local State by VariationsService. Did this work before? N/A Chrome version: 72.0.3613.0 Channel: n/a OS Version: OS X 10.13.6 Flash Version: Shockwave Flash 31.0 r0
,
Nov 21
,
Nov 21
Thanks for filing and working on the fix (https://chromium-review.googlesource.com/c/chromium/src/+/1344097). For Chrome, I think this affects Windows primarily. Adding appropriate label. Given this breaks first run experiments, marking as release blocker for M71 stable. I can't assign owner to boriay@yandex-team.ru even though they're working on this. Assigning owner to myself as a proxy as I'm reviewing the fix. We'll want it merged to M71 after it's submitted.
,
Nov 21
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Nov 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18483d6acf26f1c21194b3425013902166b51dd4 commit 18483d6acf26f1c21194b3425013902166b51dd4 Author: Boris Yusupov <boriay@yandex-team.ru> Date: Fri Nov 23 16:44:43 2018 Read master preferences before create variations to allow to migrate variations from master preferences to Local State Change-Id: I25cca8a8265e2fae57d5d0a13b053e577a9bba11 Bug: 907434 Change-Id: I25cca8a8265e2fae57d5d0a13b053e577a9bba11 Reviewed-on: https://chromium-review.googlesource.com/c/1344097 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#610635} [modify] https://crrev.com/18483d6acf26f1c21194b3425013902166b51dd4/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/18483d6acf26f1c21194b3425013902166b51dd4/chrome/browser/chrome_browser_main.h [modify] https://crrev.com/18483d6acf26f1c21194b3425013902166b51dd4/chrome/browser/first_run/first_run_browsertest.cc [modify] https://crrev.com/18483d6acf26f1c21194b3425013902166b51dd4/chrome/browser/metrics/chrome_feature_list_creator.cc [modify] https://crrev.com/18483d6acf26f1c21194b3425013902166b51dd4/chrome/browser/metrics/chrome_feature_list_creator.h
,
Nov 23
CL is landed. Will request merge on Monday if there are no issues over the weekend in Canary.
,
Nov 25
Thank you, pls request a merge to tomorrow morning if change looks good in canary.
,
Nov 25
Just to update, CL from C#5 has introduced a big spike in browser crashes on Win canary tracked in Issue 908259.
,
Nov 25
Revert of CL is in cq. We'll need to modify the approach to resolve the crash issue. I think we might be able to split the first run import in two: chrome_feature_list_creator.cc can still apply the first run variations prefs as done by the CL, but the other work done by that function could be done after resourcebundle initialization.
,
Nov 25
Basically, I think we can just move this code:
first_run::ProcessMasterPreferencesResult pmp_result =
first_run::ProcessMasterPreferences(user_data_dir, master_prefs_.get());
if (pmp_result == first_run::EULA_EXIT_NOW)
return chrome::RESULT_CODE_EULA_REFUSED;
Back to chrome_browser_main.cc. The rest of the CL can stay as-is. I believe that should work. I can prepare that change on Monday. It requires an official Chrome build so probably I have to do it since boriay@ can't build a GOOGLE_CHROME_BUILD - which is the codepath that's crashing - in CreateProfilePrefStoreManager(). I'll also add a CHECK on HasSharedInstance() to that function outside of the ifdef so this sort of thing can be caught by regular builds and not just official ones.
,
Nov 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d55a40d24fe9cb351fd91b8538754e8bb24314d5 commit d55a40d24fe9cb351fd91b8538754e8bb24314d5 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Sun Nov 25 19:18:18 2018 Revert "Read master preferences before create variations" This reverts commit 18483d6acf26f1c21194b3425013902166b51dd4. Reason for revert: Crash in official build due to resourcebundle not being initialized: crbug.com/908259 Original change's description: > Read master preferences before create variations > to allow to migrate variations from master preferences to Local State > > Change-Id: I25cca8a8265e2fae57d5d0a13b053e577a9bba11 > > Bug: 907434 > Change-Id: I25cca8a8265e2fae57d5d0a13b053e577a9bba11 > Reviewed-on: https://chromium-review.googlesource.com/c/1344097 > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Jochen Eisinger <jochen@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#610635} TBR=sky@chromium.org,asvitkine@chromium.org,hanxi@chromium.org,jochen@chromium.org,boriay@yandex-team.ru # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 907434 Change-Id: Ic48accb1c33bb5206dac265596628d8225ee2683 Reviewed-on: https://chromium-review.googlesource.com/c/1350201 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#610737} [modify] https://crrev.com/d55a40d24fe9cb351fd91b8538754e8bb24314d5/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/d55a40d24fe9cb351fd91b8538754e8bb24314d5/chrome/browser/chrome_browser_main.h [modify] https://crrev.com/d55a40d24fe9cb351fd91b8538754e8bb24314d5/chrome/browser/first_run/first_run_browsertest.cc [modify] https://crrev.com/d55a40d24fe9cb351fd91b8538754e8bb24314d5/chrome/browser/metrics/chrome_feature_list_creator.cc [modify] https://crrev.com/d55a40d24fe9cb351fd91b8538754e8bb24314d5/chrome/browser/metrics/chrome_feature_list_creator.h
,
Nov 26
The NextAction date has arrived: 2018-11-26
,
Nov 26
,
Nov 26
CL got reverted at #11, when do we expect safe cl to land in trunk? We're planning to cut M71 stable RC tomorrow @ 1:00 PM PT.
,
Nov 26
I'm working on a fix today. I don't think we should be shipping M71 stable without this fix and the fix for crbug.com/908114 , since it will cause breakage for experiments. Happy to discuss in more detail if you'd like.
,
Nov 26
Pls land the fix to trunk ASAP and request a merge to M71 once change looks good in canary. We can delay stable RC cut by few days to Thursday/Friday morning if needed. Thank you.
,
Nov 26
Sent re-land w/ fix for review: https://chromium-review.googlesource.com/c/chromium/src/+/1351090
,
Nov 26
Pls land the change to trunk ASAP so it gets picked up by tonight's canary.
,
Nov 26
Waiting on LGTMs. Pinged all the reviewers.
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06 commit 1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Mon Nov 26 23:27:13 2018 Fix first run variations support. This is a partial re-land of: https://chromium-review.googlesource.com/c/1344097 "Read master preferences before create variations to allow to migrate variations from master preferences to Local State" That CL's diff is in patchset 1. However, that CL caused a crash on first run in official builds due to official build first run logic relying on ResourceBundle being initialized. That CL tried to move all first run master prefs import logic to be earlier. This CL partially reverts that and only moves the logic specific to variations' first run (which does not depend on ResourceBundle) earlier, while keeping the rest of first run import in the same place as before. Confirmed that this fixes the crash on official builds while still fixing variations first run. Also adds a CHECK about ResourceBundle being initialized in the non-official build codepath too, to match what official build does. TBR=cpu@chromium.org Bug: 907434 Change-Id: I6ae4be40e376d61dd043e37fc4e35bce59473af0 Reviewed-on: https://chromium-review.googlesource.com/c/1351090 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Xi Han <hanxi@chromium.org> Cr-Commit-Position: refs/heads/master@{#610973} [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/chrome_browser_main.h [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/first_run/first_run.cc [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/first_run/first_run.h [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/first_run/first_run_browsertest.cc [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/first_run/first_run_unittest.cc [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/metrics/chrome_feature_list_creator.cc [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/metrics/chrome_feature_list_creator.h [modify] https://crrev.com/1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06/chrome/browser/prefs/chrome_pref_service_factory.cc
,
Nov 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c52155f062cf65270868e7c278d61389cf7b0b65 commit c52155f062cf65270868e7c278d61389cf7b0b65 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Tue Nov 27 05:54:19 2018 Fix first run variations support. This is a partial re-land of: https://chromium-review.googlesource.com/c/1344097 "Read master preferences before create variations to allow to migrate variations from master preferences to Local State" That CL's diff is in patchset 1. However, that CL caused a crash on first run in official builds due to official build first run logic relying on ResourceBundle being initialized. That CL tried to move all first run master prefs import logic to be earlier. This CL partially reverts that and only moves the logic specific to variations' first run (which does not depend on ResourceBundle) earlier, while keeping the rest of first run import in the same place as before. Confirmed that this fixes the crash on official builds while still fixing variations first run. Also adds a CHECK about ResourceBundle being initialized in the non-official build codepath too, to match what official build does. TBR=cpu@chromium.org Bug: 907434 Change-Id: I6ae4be40e376d61dd043e37fc4e35bce59473af0 Reviewed-on: https://chromium-review.googlesource.com/c/1351090 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Xi Han <hanxi@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#610973}(cherry picked from commit 1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06) Reviewed-on: https://chromium-review.googlesource.com/c/1352029 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Commit-Position: refs/branch-heads/3622@{#4} Cr-Branched-From: bcc24571cb9986398513d54297a9a769d9d60ba3-refs/heads/master@{#610746} [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/chrome_browser_main.h [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/first_run/first_run.cc [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/first_run/first_run.h [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/first_run/first_run_browsertest.cc [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/first_run/first_run_unittest.cc [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/metrics/chrome_feature_list_creator.cc [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/metrics/chrome_feature_list_creator.h [modify] https://crrev.com/c52155f062cf65270868e7c278d61389cf7b0b65/chrome/browser/prefs/chrome_pref_service_factory.cc
,
Nov 27
Today's 8:00 PM PT canary didn't get trigger, see bug 908730 for more details. So I merged the change listed at #20 to canary branch 3622 and triggering new canary from same branch to have canary coverage, Thank you.
,
Nov 27
Bug 908730 is fixed and triggered new canary #72.0.3623.0 from ToT. Pls verify this bug on canary version 72.0.3623.0 tomorrow.
,
Nov 27
The NextAction date has arrived: 2018-11-27
,
Nov 27
Not seeing any new crash due to the CL from C#21 on Windows canary 72.0.3623.0 live for 1 hr, however there is one other blocker crash Issue 908791 due to CL from Issue 908114 .
,
Nov 27
Is this bug only applicable to Windows and Mac or any other OSs too?
,
Nov 27
For Google Chrome, this applies to Windows specifically. (Even though Yandex browser also uses this functionality on Mac.)
,
Nov 27
Adding Merge-Request-71 label. I do not believe the crashes from the other canary bug should mask anything for this one. Those only affect a subset of users and I haven't seen any stability issues from this CL yet.
,
Nov 27
This bug requires manual review: We are only 6 days from stable. Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 27
Approving merge to M71 branch 3578 based on comments #25 and #28. Please merge ASAP so we can pick it up for tomorrow's last beta release, cutting RC soon. Thank you. Pls mark this bug as fixed after the merge if nothing else is pending.
,
Nov 27
Unfortunately the patch does not apply cleanly since it is based on the patch in https://bugs.chromium.org/p/chromium/issues/detail?id=908114. Since the other one is also RBS that we plan to resolve for M71 stable, suggest we merge both together after we land the follow-up fix to that patch and confirm no issues.
,
Nov 27
OK, sounds good.
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5cdf46280b9eb60fd30d35972caeae3a77e62c0 commit a5cdf46280b9eb60fd30d35972caeae3a77e62c0 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Thu Nov 29 13:38:35 2018 Fix first run variations support. This is a partial re-land of: https://chromium-review.googlesource.com/c/1344097 "Read master preferences before create variations to allow to migrate variations from master preferences to Local State" That CL's diff is in patchset 1. However, that CL caused a crash on first run in official builds due to official build first run logic relying on ResourceBundle being initialized. That CL tried to move all first run master prefs import logic to be earlier. This CL partially reverts that and only moves the logic specific to variations' first run (which does not depend on ResourceBundle) earlier, while keeping the rest of first run import in the same place as before. Confirmed that this fixes the crash on official builds while still fixing variations first run. Also adds a CHECK about ResourceBundle being initialized in the non-official build codepath too, to match what official build does. TBR=asvitkine@chromium.org, cpu@chromium.org (cherry picked from commit c52155f062cf65270868e7c278d61389cf7b0b65) Bug: 907434 Change-Id: I6ae4be40e376d61dd043e37fc4e35bce59473af0 Reviewed-on: https://chromium-review.googlesource.com/c/1351090 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Xi Han <hanxi@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#610973}(cherry picked from commit 1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06) Reviewed-on: https://chromium-review.googlesource.com/c/1352029 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/3622@{#4} Cr-Original-Branched-From: bcc24571cb9986398513d54297a9a769d9d60ba3-refs/heads/master@{#610746} Reviewed-on: https://chromium-review.googlesource.com/c/1354269 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#847} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/chrome_browser_main.h [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/first_run/first_run.cc [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/first_run/first_run.h [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/first_run/first_run_browsertest.cc [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/first_run/first_run_unittest.cc [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/metrics/chrome_feature_list_creator.cc [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/metrics/chrome_feature_list_creator.h [modify] https://crrev.com/a5cdf46280b9eb60fd30d35972caeae3a77e62c0/chrome/browser/prefs/chrome_pref_service_factory.cc
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5cdf46280b9eb60fd30d35972caeae3a77e62c0 Commit: a5cdf46280b9eb60fd30d35972caeae3a77e62c0 Author: asvitkine@chromium.org Commiter: asvitkine@chromium.org Date: 2018-11-29 13:38:35 +0000 UTC Fix first run variations support. This is a partial re-land of: https://chromium-review.googlesource.com/c/1344097 "Read master preferences before create variations to allow to migrate variations from master preferences to Local State" That CL's diff is in patchset 1. However, that CL caused a crash on first run in official builds due to official build first run logic relying on ResourceBundle being initialized. That CL tried to move all first run master prefs import logic to be earlier. This CL partially reverts that and only moves the logic specific to variations' first run (which does not depend on ResourceBundle) earlier, while keeping the rest of first run import in the same place as before. Confirmed that this fixes the crash on official builds while still fixing variations first run. Also adds a CHECK about ResourceBundle being initialized in the non-official build codepath too, to match what official build does. TBR=asvitkine@chromium.org, cpu@chromium.org (cherry picked from commit c52155f062cf65270868e7c278d61389cf7b0b65) Bug: 907434 Change-Id: I6ae4be40e376d61dd043e37fc4e35bce59473af0 Reviewed-on: https://chromium-review.googlesource.com/c/1351090 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Xi Han <hanxi@chromium.org> Cr-Original-Original-Commit-Position: refs/heads/master@{#610973}(cherry picked from commit 1ac2b95d87558cb9f13af9eaf11b81b3fc98fb06) Reviewed-on: https://chromium-review.googlesource.com/c/1352029 Reviewed-by: Krishna Govind <govind@chromium.org> Cr-Original-Commit-Position: refs/branch-heads/3622@{#4} Cr-Original-Branched-From: bcc24571cb9986398513d54297a9a769d9d60ba3-refs/heads/master@{#610746} Reviewed-on: https://chromium-review.googlesource.com/c/1354269 Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#847} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Nov 29
Merged to M71. Marking as Fixed. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by bor...@gmail.com
, Nov 21