Issue metadata
Sign in to add a comment
|
Parts of Window not redrawn after theme change |
||||||||||||||||||||||
Issue descriptionChrome Version: 57, 58, ToT OS: Ubuntu 14.04 What steps will reproduce the problem? (1) Open a Chrome window (2) Use gnome-tweak-tool to change the gtk theme What is the expected result? The Chrome window should be redrawn with the new theme What happens instead? Only parts of the window are redrawn. I've bisected this to: https://chromium.googlesource.com/chromium/src/+/f7c7217dd882eac40d5d07e441db5ef9fe3c94a4 Assigning to ananta@ I see that the above commit was also merged to M57 and M58, so the fix for this issue must be merged as well.
,
Mar 10 2017
Just to update, Able to reproduce the issue on the latest Stable/Beta(57.0.2987.98) and M-59(59.0.3036.0). Somehow couldn't repro this on the Windows and Mac using the High Contrast Extension from the CWS. Confirming the bisect points to the same suspect as in C#0 and with hasbisect-per-revision. Regressed in M-58. Last good build: 58.0.3017.0 First bad build: 58.0.3018.0 Changelog: ========== https://chromium.googlesource.com/chromium/src/+log/6b9501df29d056a886b6d30ebb3b5a86daa8685d..f7c7217dd882eac40d5d07e441db5ef9fe3c94a4 ananta@: Could you please take a look at this issue. Note: The Window renders/redraws correctly when Chrome is focused.
,
Mar 10 2017
[Bulk Edit] Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP. Thank you! Know that this issue shouldn't block the release? Remove the ReleaseBlock-Stable label or move to M58.
,
Mar 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1 commit 3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1 Author: ananta <ananta@chromium.org> Date: Tue Mar 14 02:22:05 2017 Fix theme change paint issue on linux. We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in this patch https://codereview.chromium.org/2703933002. This was to work around a crash we were seeing on Windows where views would be added/removed while the rootview was propagating the theme changed notification to child views effectively making the iterators invalid. The recursion killer check in Widget::FrameTypeChanged() causes a painting bug on Linux. The callstack thanks to thomasanderson is here. #2 0x7fdda6f06c83 views::Widget::FrameTypeChanged() #3 0x55801d8ad66c BrowserView::UserChangedTheme() #4 0x55801d8b0aef BrowserView::OnNativeThemeChanged() #5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged() #6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged() #7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged() #8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated() #9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers() #10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle() #11 0x7fdda419532d libgtkui::OnThemeChanged() The BrowserView::UserChangedTheme() function also has a recursion killer which is not set to true here, which indicates that this stack is legitimate. Fixes as below: 1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After calling the base class we call the newly added function NativeThemeUpdated() in the BrowserView which ensures that we don't nuke the views while the iteration is going on in View::PropagateNativeThemeChanged() 2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen window map after we call PerformDwmTransition(). This internally at times causes a window pos changed to treat the now non fullscreen window as fullscreen leading to a DCHECK. Fixed by moving this code above. I verified that this does not regress the previous theme fix http://crbug.com/681525 on Windows. BUG= 700135 Review-Url: https://codereview.chromium.org/2743793003 Cr-Commit-Position: refs/heads/master@{#456580} [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/chrome/browser/ui/views/frame/browser_frame.h [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/ui/views/widget/widget.cc [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/ui/views/widget/widget.h [modify] https://crrev.com/3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1/ui/views/win/hwnd_message_handler.cc
,
Mar 14 2017
Fix from C#4 missed today's M-59(59.0.3041.0 cut @456562). Will verify the fix in tomorrow's M-59 build.
,
Mar 14 2017
ananta@, could you please merge this change to Canary branch #3041 so we can trigger new Canary to verify this fix. Thank you. Note: Merge approval is not needed as this merge request is for Canary branch.
,
Mar 14 2017
Per chat with ananta@, we decided not to merge this change to branch #3014 (wait until tonight's canary instead) as this patch won't have any impact on Stability. Test team please perform below verification on tonight's canary: * Verify this bug (700135) on Linux * Verify 681525 on Windows * Perform some theme testing Thank you.
,
Mar 14 2017
,
Mar 14 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-57; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-57 label, otherwise remove Merge-TBD label. Thanks.
,
Mar 14 2017
,
Mar 15 2017
Verified the fix on the latest M-59(59.0.3042.0) on Linux Ubuntu 14.04 as per test steps in C#0 and didn't observe any Window rendering issue while changing theme. Adding the screen-cast of the same and verified label for M-59.
,
Mar 15 2017
With respect to comment#7 rechecked issues 700135 on Linux & 681525 on Windows Below are some theme testing performed: 1. Changed GTK+ theme to Ambiance/Radiance/High Contrast. Added external themes from webstore Applied Dark themes Checked tab-strip Downloads bar Fav-icons of Bookmarked pages/opened pages on tab-strip Fav-icons of added extensions in browser containment area Checked Omni-box content by hitting ctrl+e[Search google] Checked overlay focuses [ Filed Issue 701737 / Regressed in M-59] 2. Changed Cursor to all available in Tweek tool and checked. Compared with previous stable and beta builds 3. Covered all probable combinations of all GTK+/cursor/window/Icons 4. Added external High Contrast extension from webstore and checked how it effects all GTK+ themes present in Tweek Tool In all the scenarios no new issues or side-effect's were observed.
,
Mar 15 2017
,
Mar 15 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 15 2017
This bug requires manual review: Request affecting a post-stable build 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
,
Mar 15 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 15 2017
,
Mar 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b58fa9d8b8e60498a56101a16987d85d9ffecab commit 9b58fa9d8b8e60498a56101a16987d85d9ffecab Author: ananta <ananta@chromium.org> Date: Wed Mar 15 20:24:27 2017 Fix theme change paint issue on linux. Merging to M58 We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in this patch https://codereview.chromium.org/2703933002. This was to work around a crash we were seeing on Windows where views would be added/removed while the rootview was propagating the theme changed notification to child views effectively making the iterators invalid. The recursion killer check in Widget::FrameTypeChanged() causes a painting bug on Linux. The callstack thanks to thomasanderson is here. The BrowserView::UserChangedTheme() function also has a recursion killer which is not set to true here, which indicates that this stack is legitimate. Fixes as below: 1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After calling the base class we call the newly added function NativeThemeUpdated() in the BrowserView which ensures that we don't nuke the views while the iteration is going on in View::PropagateNativeThemeChanged() 2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen window map after we call PerformDwmTransition(). This internally at times causes a window pos changed to treat the now non fullscreen window as fullscreen leading to a DCHECK. Fixed by moving this code above. I verified that this does not regress the previous theme fix http://crbug.com/681525 on Windows. BUG= 700135 TBR=sky NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2743793003 Cr-Commit-Position: refs/heads/master@{#456580} (cherry picked from commit 3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1) Review-Url: https://codereview.chromium.org/2753813003 Cr-Commit-Position: refs/branch-heads/3029@{#214} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/chrome/browser/ui/views/frame/browser_frame.h [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/ui/views/widget/widget.cc [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/ui/views/widget/widget.h [modify] https://crrev.com/9b58fa9d8b8e60498a56101a16987d85d9ffecab/ui/views/win/hwnd_message_handler.cc
,
Mar 15 2017
Approving merge to M57 branch 2987 based on comment #11, #12 and testing performed by Prudhvi. Please merge ASAP. Thank you.
,
Mar 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66178fb1d289ca375868890ec6ff7af4cc9af0ad commit 66178fb1d289ca375868890ec6ff7af4cc9af0ad Author: ananta <ananta@chromium.org> Date: Wed Mar 15 21:33:58 2017 Fix theme change paint issue on linux. Merging to M57 We had added a recursion guard in the Widget::OnNativeThemeUpdated() function in this patch https://codereview.chromium.org/2703933002. This was to work around a crash we were seeing on Windows where views would be added/removed while the rootview was propagating the theme changed notification to child views effectively making the iterators invalid. The recursion killer check in Widget::FrameTypeChanged() causes a painting bug on Linux. The callstack thanks to thomasanderson is here. The BrowserView::UserChangedTheme() function also has a recursion killer which is not set to true here, which indicates that this stack is legitimate. Fixes as below: 1. BrowserFrame now overrides the Widget::OnNativeThemeUpdated() function. After calling the base class we call the newly added function NativeThemeUpdated() in the BrowserView which ensures that we don't nuke the views while the iteration is going on in View::PropagateNativeThemeChanged() 2. In the HWNDMessageHandler::SetFullscreen method we update the fullscreen window map after we call PerformDwmTransition(). This internally at times causes a window pos changed to treat the now non fullscreen window as fullscreen leading to a DCHECK. Fixed by moving this code above. I verified that this does not regress the previous theme fix http://crbug.com/681525 on Windows. BUG= 700135 TBR=sky NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2743793003 Cr-Commit-Position: refs/heads/master@{#456580} (cherry picked from commit 3d0d1cdfad3fd70d8d579dd03004b4e09d01f2f1) Review-Url: https://codereview.chromium.org/2753933003 Cr-Commit-Position: refs/branch-heads/2987@{#831} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/chrome/browser/ui/views/frame/browser_frame.h [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/ui/views/widget/widget.cc [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/ui/views/widget/widget.h [modify] https://crrev.com/66178fb1d289ca375868890ec6ff7af4cc9af0ad/ui/views/win/hwnd_message_handler.cc
,
Mar 15 2017
Requesting a postmortem for this (please see go/chrome-postmortems for the process to follow). Thank you.
,
Mar 16 2017
Verified this issue on Ubuntu 14.04 using chrome latest stable M57-57.0.2987.110 by following steps mentioned in the original comment. Observed no rendering issues while changing the theme using gnome-tweak-tool. Hence adding TE-Verified label. Thanks!
,
Mar 22 2017
Tested the issue on Linux Ubuntu 14.04 using chrome version 58.0.3029.33.Not observed any rendering issue while changing the themes using gnome-tweak-tool. Please find the attached screen cast for the same. Adding TE-Verified labels. Thanks, |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by thomasanderson@chromium.org
, Mar 10 2017I've gathered the stack trace you requested with this patch: diff --git a/ui/views/widget/widget.cc b/ui/views/widget/widget.cc index 38ade374c65c..dfe9c18f13e3 100644 --- a/ui/views/widget/widget.cc +++ b/ui/views/widget/widget.cc @@ -919,8 +919,10 @@ void Widget::DebugToggleFrameType() { } void Widget::FrameTypeChanged() { - if (processing_theme_changed_) + if (processing_theme_changed_) { + base::debug::StackTrace().Print(); return; + } native_widget_->FrameTypeChanged(); } #0 0x7fddb23d5bcb base::debug::StackTrace::StackTrace() #1 0x7fddb23d425c base::debug::StackTrace::StackTrace() #2 0x7fdda6f06c83 views::Widget::FrameTypeChanged() #3 0x55801d8ad66c BrowserView::UserChangedTheme() #4 0x55801d8b0aef BrowserView::OnNativeThemeChanged() #5 0x7fdda6edbdf6 views::View::PropagateNativeThemeChanged() #6 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged() #7 0x7fdda6edbdc1 views::View::PropagateNativeThemeChanged() #8 0x7fdda6f09275 views::Widget::OnNativeThemeUpdated() #9 0x7fdda6aa7493 ui::NativeTheme::NotifyObservers() #10 0x7fdda4195366 libgtkui::GtkUi::ResetStyle() #11 0x7fdda419532d libgtkui::OnThemeChanged() #12 0x7fdda16353b8 g_closure_invoke #13 0x7fdda1646fd1 <unknown> #14 0x7fdda164ea29 g_signal_emit_valist #15 0x7fdda164ece2 g_signal_emit #16 0x7fdda1639725 <unknown> #17 0x7fdda163bceb g_object_notify #18 0x7fdd9b39d965 gtk_main_do_event #19 0x7fdda41916f9 libgtkui::Gtk2EventLoop::DispatchGdkEvent() #20 0x7fdd9afa0c22 <unknown> #21 0x7fdda1364e04 g_main_context_dispatch #22 0x7fdda1365048 <unknown> #23 0x7fdda13650ec g_main_context_iteration #24 0x7fddb2483fcf base::MessagePumpGlib::Run() #25 0x7fddb246b467 base::MessageLoop::RunHandler() #26 0x7fddb251b55a base::RunLoop::Run() #27 0x55801b741a1e ChromeBrowserMainParts::MainMessageLoopRun() #28 0x7fddab9e2358 content::BrowserMainLoop::RunMainMessageLoopParts() #29 0x7fddab9f0876 content::BrowserMainRunnerImpl::Run() #30 0x7fddab9db858 content::BrowserMain() #31 0x7fddad24a096 content::RunNamedProcessTypeMain() #32 0x7fddad24c43c content::ContentMainRunnerImpl::Run() #33 0x7fddad248fc2 content::ContentMain() #34 0x558019dec2fa ChromeMain #35 0x558019dec222 main #36 0x7fdd9e915f45 __libc_start_main #37 0x558019dec125 <unknown>