Revise ScopedRedrawLock and the caption button workaround.

Additionally guard against operations on destroyed widgets.

This CL incorporates and builds on changes from r101033.
That original CL was reverted to ease merging this into M15.

Add two guards against unlocking after Widget destruction.
ScopedRedrawLock will check HWND validity with IsWindow().
DefWindowProcWithRedrawLock will check for destructor calls.
Unify ScopedRedrawLock + DefWindowProc codepaths.

BUG=84563,89820,95582,95727,93530
TEST=none

Review URL: http://codereview.chromium.org/7926019

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@101642 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 84 insertions, 46 deletions

diff --git a/views/widget/native_widget_win.cc b/views/widget/native_widget_win.cc
index 2f823ee..5385ec4 100644
--- a/views/widget/native_widget_win.cc
+++ b/views/widget/native_widget_win.cc
@@ -327,6 +327,10 @@ bool NativeWidgetWin::screen_reader_active_ = false;
 // associated Window's lock and unlock functions as it is created and destroyed.
 // See documentation in those methods for the technique used.
 //
+// The lock only has an effect if the window was visible upon lock creation, as
+// it doesn't guard against direct visiblility changes, and multiple locks may
+// exist simultaneously to handle certain nested Windows messages.
+//
 // IMPORTANT: Do not use this scoping object for large scopes or periods of
 //            time! IT WILL PREVENT THE WINDOW FROM BEING REDRAWN! (duh).
 //
@@ -334,17 +338,32 @@ bool NativeWidgetWin::screen_reader_active_ = false;
 // list of other messages that this applies to ;-)
 class NativeWidgetWin::ScopedRedrawLock {
  public:
-  explicit ScopedRedrawLock(NativeWidgetWin* widget) : widget_(widget) {
-    widget_->LockUpdates();
+  explicit ScopedRedrawLock(NativeWidgetWin* widget)
+    : widget_(widget),
+      native_view_(widget_->GetNativeView()),
+      was_visible_(widget_->IsVisible()),
+      cancel_unlock_(false) {
+    if (was_visible_ && ::IsWindow(native_view_))
+      widget_->LockUpdates();
   }
 
   ~ScopedRedrawLock() {
-    widget_->UnlockUpdates();
+    if (!cancel_unlock_ && was_visible_ && ::IsWindow(native_view_))
+      widget_->UnlockUpdates();
   }
 
+  // Cancel the unlock operation, call this if the Widget is being destroyed.
+  void CancelUnlockOperation() { cancel_unlock_ = true; }
+
  private:
   // The widget having its style changed.
   NativeWidgetWin* widget_;
+  // The widget's native view, cached to avoid action after window destruction.
+  gfx::NativeView native_view_;
+  // Records the widget visibility at the time of creation.
+  bool was_visible_;
+  // A flag indicating that the unlock operation was canceled.
+  bool cancel_unlock_;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -365,16 +384,20 @@ NativeWidgetWin::NativeWidgetWin(internal::NativeWidgetDelegate* delegate)
       previous_cursor_(NULL),
       fullscreen_(false),
       force_hidden_count_(0),
-      lock_updates_(false),
+      lock_updates_count_(0),
       saved_window_style_(0),
       ignore_window_pos_changes_(false),
       ignore_pos_changes_factory_(this),
       last_monitor_(NULL),
       is_right_mouse_pressed_on_caption_(false),
-      restored_enabled_(false) {
+      restored_enabled_(false),
+      destroyed_(NULL) {
 }
 
 NativeWidgetWin::~NativeWidgetWin() {
+  if (destroyed_ != NULL)
+    *destroyed_ = true;
+
   if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
     delete delegate_;
   else
@@ -1158,7 +1181,7 @@ void NativeWidgetWin::OnActivateApp(BOOL active, DWORD thread_id) {
     delegate_->EnableInactiveRendering();
     // Update the native frame too, since it could be rendering the non-client
     // area.
-    CallDefaultNCActivateHandler(FALSE);
+    DefWindowProcWithRedrawLock(WM_NCACTIVATE, FALSE, 0);
   }
 }
 
@@ -1535,15 +1558,13 @@ LRESULT NativeWidgetWin::OnMouseRange(UINT message,
 
   bool handled = delegate_->OnMouseEvent(event);
 
-  if (!handled && message == WM_NCLBUTTONDOWN) {
+  if (!handled && message == WM_NCLBUTTONDOWN && w_param != HTSYSMENU &&
+      !GetWidget()->ShouldUseNativeFrame()) {
     // TODO(msw): Eliminate undesired painting, or re-evaluate this workaround.
     // DefWindowProc for WM_NCLBUTTONDOWN does weird non-client painting, so we
-    // need to call it directly here inside a ScopedRedrawLock. This may cause
-    // other negative side-effects (ex/ stifling non-client mouse releases).
-    ScopedRedrawLock lock(this);
-    DefWindowProc(GetNativeView(), message, w_param, l_param);
-    // Update the saved window style, which may change (maximized to restored).
-    saved_window_style_ = GetWindowLong(GWL_STYLE);
+    // need to call it inside a ScopedRedrawLock. This may cause other negative
+    // side-effects (ex/ stifling non-client mouse releases).
+    DefWindowProcWithRedrawLock(message, w_param, l_param);
     handled = true;
   }
 
@@ -1596,7 +1617,8 @@ LRESULT NativeWidgetWin::OnNCActivate(BOOL active) {
   if (IsActive())
     delegate_->EnableInactiveRendering();
 
-  return CallDefaultNCActivateHandler(inactive_rendering_disabled || active);
+  return DefWindowProcWithRedrawLock(WM_NCACTIVATE,
+                                     inactive_rendering_disabled || active, 0);
 }
 
 LRESULT NativeWidgetWin::OnNCCalcSize(BOOL mode, LPARAM l_param) {
@@ -1860,8 +1882,7 @@ LRESULT NativeWidgetWin::OnSetCursor(UINT message,
                                      WPARAM w_param,
                                      LPARAM l_param) {
   // This shouldn't hurt even if we're using the native frame.
-  ScopedRedrawLock lock(this);
-  return DefWindowProc(GetNativeView(), message, w_param, l_param);
+  return DefWindowProcWithRedrawLock(message, w_param, l_param);
 }
 
 void NativeWidgetWin::OnSetFocus(HWND focused_window) {
@@ -1874,16 +1895,14 @@ void NativeWidgetWin::OnSetFocus(HWND focused_window) {
 
 LRESULT NativeWidgetWin::OnSetIcon(UINT size_type, HICON new_icon) {
   // This shouldn't hurt even if we're using the native frame.
-  ScopedRedrawLock lock(this);
-  return DefWindowProc(GetNativeView(), WM_SETICON, size_type,
-                       reinterpret_cast<LPARAM>(new_icon));
+  return DefWindowProcWithRedrawLock(WM_SETICON, size_type,
+                                     reinterpret_cast<LPARAM>(new_icon));
 }
 
 LRESULT NativeWidgetWin::OnSetText(const wchar_t* text) {
   // This shouldn't hurt even if we're using the native frame.
-  ScopedRedrawLock lock(this);
-  return DefWindowProc(GetNativeView(), WM_SETTEXT, NULL,
-                       reinterpret_cast<LPARAM>(text));
+  return DefWindowProcWithRedrawLock(WM_SETTEXT, NULL,
+                                     reinterpret_cast<LPARAM>(text));
 }
 
 void NativeWidgetWin::OnSettingChange(UINT flags, const wchar_t* section) {
@@ -1929,11 +1948,10 @@ void NativeWidgetWin::OnSysCommand(UINT notification_code, CPoint click) {
       GetWidget()->non_client_view()->ResetWindowControls();
     } else if ((notification_code & sc_mask) == SC_MOVE ||
                (notification_code & sc_mask) == SC_SIZE) {
-      if (lock_updates_) {
-        // We were locked, before entering a resize or move modal loop. Now that
-        // we've begun to move the window, we need to unlock updates so that the
-        // sizing/moving feedback can be continuous.
-        UnlockUpdates();
+      if (!IsVisible()) {
+        // Circumvent ScopedRedrawLocks and force visibility before entering a
+        // resize or move modal loop to get continuous sizing/moving feedback.
+        SetWindowLong(GWL_STYLE, GetWindowLong(GWL_STYLE) | WS_VISIBLE);
       }
     }
   }
@@ -2288,24 +2306,28 @@ void NativeWidgetWin::RedrawLayeredWindowContents() {
 }
 
 void NativeWidgetWin::LockUpdates() {
-  lock_updates_ = true;
-
   // We skip locked updates when Aero is on for two reasons:
   // 1. Because it isn't necessary
   // 2. Because toggling the WS_VISIBLE flag may occur while the GPU process is
   //    attempting to present a child window's backbuffer onscreen. When these
   //    two actions race with one another, the child window will either flicker
   //    or will simply stop updating entirely.
-  if (!IsAeroGlassEnabled()) {
-    saved_window_style_ = GetWindowLong(GWL_STYLE);
-    SetWindowLong(GWL_STYLE, saved_window_style_ & ~WS_VISIBLE);
+  if (!IsAeroGlassEnabled() && ++lock_updates_count_ == 1) {
+    SetWindowLong(GWL_STYLE, GetWindowLong(GWL_STYLE) & ~WS_VISIBLE);
   }
+  // TODO(msw): Remove nested LockUpdates VLOG info for crbug.com/93530.
+  VLOG_IF(1, (lock_updates_count_ > 1)) << "Nested LockUpdates call: "
+      << lock_updates_count_ << " locks for widget " << this;
 }
 
 void NativeWidgetWin::UnlockUpdates() {
-  if (!IsAeroGlassEnabled())
-    SetWindowLong(GWL_STYLE, saved_window_style_);
-  lock_updates_ = false;
+  // TODO(msw): Remove nested LockUpdates VLOG info for crbug.com/93530.
+  VLOG_IF(1, (lock_updates_count_ > 1)) << "Nested UnlockUpdates call: "
+      << lock_updates_count_ << " locks for widget " << this;
+  if (!IsAeroGlassEnabled() && --lock_updates_count_ <= 0) {
+    SetWindowLong(GWL_STYLE, GetWindowLong(GWL_STYLE) | WS_VISIBLE);
+    lock_updates_count_ = 0;
+  }
 }
 
 bool NativeWidgetWin::WidgetSizeIsClientSize() const {
@@ -2373,12 +2395,20 @@ void NativeWidgetWin::ResetWindowRegion(bool force) {
   DeleteObject(current_rgn);
 }
 
-LRESULT NativeWidgetWin::CallDefaultNCActivateHandler(BOOL active) {
-  // The DefWindowProc handling for WM_NCACTIVATE renders the classic-look
-  // window title bar directly, so we need to use a redraw lock here to prevent
-  // it from doing so.
+LRESULT NativeWidgetWin::DefWindowProcWithRedrawLock(UINT message,
+                                                     WPARAM w_param,
+                                                     LPARAM l_param) {
   ScopedRedrawLock lock(this);
-  return DefWindowProc(GetNativeView(), WM_NCACTIVATE, active, 0);
+  // The Widget and HWND can be destroyed in the call to DefWindowProc, so use
+  // the |destroyed_| flag to avoid unlocking (and crashing) after destruction.
+  bool destroyed = false;
+  destroyed_ = &destroyed;
+  LRESULT result = DefWindowProc(GetNativeView(), message, w_param, l_param);
+  if (destroyed)
+    lock.CancelUnlockOperation();
+  else
+    destroyed_ = NULL;
+  return result;
 }
 
 void NativeWidgetWin::RestoreEnabledIfNecessary() {
diff --git a/views/widget/native_widget_win.h b/views/widget/native_widget_win.h
index dd1158b..cb635f5 100644
--- a/views/widget/native_widget_win.h
+++ b/views/widget/native_widget_win.h
@@ -513,10 +513,12 @@ class VIEWS_EXPORT NativeWidgetWin : public ui::WindowImpl,
   // frame windows.
   void ResetWindowRegion(bool force);
 
-  // Calls the default WM_NCACTIVATE handler with the specified activation
-  // value, safely wrapping the call in a ScopedRedrawLock to prevent frame
-  // flicker.
-  LRESULT CallDefaultNCActivateHandler(BOOL active);
+  // Calls DefWindowProc, safely wrapping the call in a ScopedRedrawLock to
+  // prevent frame flicker. DefWindowProc handling can otherwise render the
+  // classic-look window title bar directly.
+  LRESULT DefWindowProcWithRedrawLock(UINT message,
+                                      WPARAM w_param,
+                                      LPARAM l_param);
 
   // Stops ignoring SetWindowPos() requests (see below).
   void StopIgnoringPosChanges() { ignore_window_pos_changes_ = false; }
@@ -619,8 +621,9 @@ class VIEWS_EXPORT NativeWidgetWin : public ui::WindowImpl,
   DWORD drag_frame_saved_window_style_;
   DWORD drag_frame_saved_window_ex_style_;
 
-  // True if updates to this window are currently locked.
-  bool lock_updates_;
+  // Represents the number of ScopedRedrawLocks active against this widget.
+  // If this is greater than zero, the widget should be locked against updates.
+  int lock_updates_count_;
 
   // The window styles of the window before updates were locked.
   DWORD saved_window_style_;
@@ -650,6 +653,11 @@ class VIEWS_EXPORT NativeWidgetWin : public ui::WindowImpl,
   // The compositor for accelerated drawing.
   scoped_refptr<ui::Compositor> compositor_;
 
+  // This flag can be initialized and checked after certain operations (such as
+  // DefWindowProc) to avoid stack-controlled NativeWidgetWin operations (such
+  // as unlocking the Window with a ScopedRedrawLock) after Widget destruction.
+  bool* destroyed_;
+
   DISALLOW_COPY_AND_ASSIGN(NativeWidgetWin);
 };