Issue metadata
Sign in to add a comment
|
Regression: User zoom levels reset
Reported by
d...@unabridgedsoftware.com,
Aug 15 2017
|
||||||||||||||||||||||
Issue descriptionChrome Version : 61.0.3163.39 (Official Build) beta (64-bit) OS : Linux (Ubuntu 16.04.03 LTS) URLs (if applicable) : n/a Other browsers tested: n/a What steps will reproduce the problem? (1) Upgrade Chrome beta from 60 to 61 (2) Restart browser What is the expected result? Previously set zoom levels per-website are persisted and automatically applied when re-visiting the sites. What happens instead? All website have the zoom set to the default of 100%. No previously set zoom level is used. Please provide any additional information below. Attach a screenshot if possible. I think this is pretty self-explanatory and straightforward. This is obviously not a huge deal-breaker but it's a bit annoying to have happen.
,
Aug 16 2017
,
Aug 16 2017
ranjitkan@, pls try to repro and bisect if possible. Thank you.
,
Aug 17 2017
Able to reproduce the issue using chrome version 61.0.3163.49 & Canary build 62.0.3187.0 on Ubuntu 14.04. Issue is broken in M61 and below are the bisect details for the same: Bisect info: ============ 61.0.3145.0 - Good build 61.0.3146.0 - Bad build Change Log: https://chromium.googlesource.com/chromium/src/+log/61.0.3145.0..61.0.3146.0?pretty=fuller&n=10000 Unable to narrow down using bisect script as we need to update chrome over the existing saved settings. Manually searching suspecting below changes could be a possible culprit: https://chromium.googlesource.com/chromium/src/+/f3b6e00fe2826406218b15bd2f317cfebe835696 (or) https://chromium.googlesource.com/chromium/src/+/7c44da721a59e6aa0b9fdcddb314175cb1e0123f @patricialor: Assigning to you, kindly take a look into it. Please help us to find an owner if not with respect to your change. @rune: CC'ing you incase if it is related to your change. Issue is not observed on Windows and MAC OS.
,
Aug 17 2017
Adding blocker label, please remove if not the case. Thanks.!
,
Aug 17 2017
Almost certainly not mine.
,
Aug 17 2017
I'm not sure this is me, though I also don't think it's rune@'s change either. It might have something to do with this patch? https://chromium.googlesource.com/chromium/src/+/08a0e3ee7ad01c643155daf1c95a70954fe05c00 I could be wrong - tbansal@, any chance your change could have deleted some of the zoom prefs for whatever reason?
,
Aug 17 2017
At first glance, it does not seem like it was because of my patch (linked in #7). My patch was not OS-specific or zoom-level specific.
,
Aug 18 2017
,
Aug 20 2017
+ pbommana, could you pls bisect this?
,
Aug 21 2017
thomasanderson@ ranjit from our team has already bisected the issue and posted the bisect results in comment#4.
,
Aug 21 2017
[Bulk Edit] URGENT - PTAL. M61 Stable promotion is coming soon and your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M62. Thank you! Note: We will only have 2 beta releases before Stable promotion. Plan is to cut M61 Stable RC on 08/31/17.
,
Aug 21 2017
c#11: That bisect has many revisions. Using tools/bisect_builds.py, I was able to bisect it down to just 9 revisions: https://chromium.googlesource.com/chromium/src/+log/172d5dce95c34577fc45230fa8d5fd18f2143364..4c513d4ad2b4353ab41d3c7666c0bad7ae043379 Based on that, I'd guess this issue is caused by switching chrome to libc++ (my cl): 39fcd6af4d8c9ddcf909b8d489c6ba684d046157 I'll see if I can find a root cause
,
Aug 21 2017
Thanks very much for doing a narrower bisect thomasanderson@!
,
Aug 21 2017
Update: Confirmed the symptom is caused by 39fcd6af4d8c9ddcf909b8d489c6ba684d046157 Details: In our user preference file, the per-host-zoom-level is stored as a key-value pair. The key is the hash of some string, calculated here: https://cs.chromium.org/chromium/src/chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc?rcl=e41aeaed051c8cc401e0bce2a6d57dcc330808e6&l=53 The hash function ends up plumbing into std::hash<std::string>(). Since the libstdc++ hash algorithm is different from the libc++ one, Chrome creates new zoom preferences for the new hash, effectively resetting the zoom preferences for all hosts. There are some options: (1) Change the key to be deterministic, which would fix the issue in the long run, but with a one-time reset of everyone's zoom preferences on all platforms. (2) WontFix the issue, with this issue resurfacing whenever the standard library changes. (3) Create a hack on each platform that copies the system behavior of std::hash<std::string>(). Unless anyone has any objections, I'm going to move forward with (1).
,
Aug 22 2017
I agree that (1) seems the best ... but is it possible to make the fixed key the same as the old key, to perhaps minimize the number of people who see even one reset? Or has that boat already sailed ...
,
Aug 22 2017
,
Aug 22 2017
c#16 Now that you mention it, I think it is possible to preserve everyone's zoom settings. On initialization, we could just check using both the new and the old keys, and if (# of elements for new key) < (# of elements for old key), then migrate the settings to the new key. This should preserve Mac and Windows user's settings. The Linux case would still be broken though, unless we try to mimic libstdc++'s std::hash<std::string>. Lastly, we would remove the migration code after a few stable releases. wdyt?
,
Aug 22 2017
Sounds good. How hard would mimicing libstdc++'s thing for the migration code be?
,
Aug 22 2017
The libstdc++ std::hash<std::string> impl uses their _Hash_bytes function, and the documentation for that says: // The actual hash algorithm is not guaranteed to stay // the same from release to release -- it may be updated or tuned to // improve hash quality or speed. https://cs.chromium.org/chromium/src/third_party/perl/c/lib/gcc/i686-w64-mingw32/4.6.3/include/c%2B%2B/bits/hash_bytes.h?q=%22hash+function+implementation+for+the+nontrivial%22&sq=package:chromium&l=41 So I probably can't just copy whatever the libstdc++ does on my machine into Chromium code. Perhaps we could dlopen() libstdc++.so.6 and dlsym() _Hash_bytes, and if the library and symbol are available, try to use it?
,
Aug 24 2017
Requesting merge for https://crrev.com/b2be8c44f3383dc069d3b298b2d978d6592af08b
,
Aug 24 2017
This bug requires manual review: We are only 11 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 24 2017
As per offline discussion with govind@, we agreed to pass on the M61 merge. The change (https://crrev.com/b2be8c44f3383dc069d3b298b2d978d6592af08b) has not been tested in beta and the issue is not serious enough to warrant a merge of such a risky change. Users will have to wait until M62 to see the fix. There will not be a similar reset of zoom settings on the upgrade from M61 to M62, and per-site zoom settings from pre-M61 will even be retroactively restored when M62 is released.
,
Aug 24 2017
Rejecting merge to M61 based on comment #23.
,
Aug 24 2017
,
Aug 24 2017
,
Aug 25 2017
Rechecked this issue on Chrome version 62.0.3196.0. Fix is working as intended. Installed first chrome version 62.0.3192.0 and from chrome://setting changed the page zoom level to 125%. Over installed Chrome version 62.0.3196.0 and navigated to chrome setting and observed that chrome retained the page zoom percentage. Adding TE-Verified labels for M62. Thanks.! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by jainabhi...@chromium.org
, Aug 16 2017Labels: -Type-Bug -Pri-3 Hotlist-ConOps-Channel-Beta Hotlist-ConOps M-61 Hotlist-ConOps-Source-Forum OS-Linux Pri-1 Type-Bug-Regression