Mac: Attach Extension NSViews to the view hierarchy before creating renderers

chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/extensions/extension_popup_controller.h"
 
 #include <algorithm>
 
 #include "base/callback.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ skipping 28 matching lines @@
 
 // Given a value and a rage, clamp the value into the range.
 CGFloat Clamp(CGFloat value, CGFloat min, CGFloat max) {
   return std::max(min, std::min(max, value));
 }
 
 }  // namespace
 
 @interface ExtensionPopupController (Private)
 // Callers should be using the public static method for initialization.
-// NOTE: This takes ownership of |host|.
-- (id)initWithHost:(extensions::ExtensionViewHost*)host
-      parentWindow:(NSWindow*)parentWindow
-        anchoredAt:(NSPoint)anchoredAt
-     arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
-           devMode:(BOOL)devMode;
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                anchoredAt:(NSPoint)anchoredAt
+             arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
+                   devMode:(BOOL)devMode;
+
+// Set the ExtensionViewHost, taking ownership.
+- (void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host;
 
 // Called when the extension's hosted NSView has been resized.
 - (void)extensionViewFrameChanged;
 
 // Called when the extension's size changes.
 - (void)onSizeChanged:(NSSize)newSize;
 
 // Called when the extension view is shown.
 - (void)onViewDidShow;
 
@@ skipping 78 matching lines @@
   // know what it is for notifications, but our ExtensionViewHost may not be
   // valid.
   WebContents* web_contents_;
   base::Callback<void(content::DevToolsAgentHost*, bool)> devtools_callback_;
 };
 
 @implementation ExtensionPopupController
 
 @synthesize extensionId = extensionId_;
 
-- (id)initWithHost:(extensions::ExtensionViewHost*)host
-      parentWindow:(NSWindow*)parentWindow
-        anchoredAt:(NSPoint)anchoredAt
-     arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
-           devMode:(BOOL)devMode {
+- (id)initWithParentWindow:(NSWindow*)parentWindow
+                anchoredAt:(NSPoint)anchoredAt
+             arrowLocation:(info_bubble::BubbleArrowLocation)arrowLocation
+                   devMode:(BOOL)devMode {
   base::scoped_nsobject<InfoBubbleWindow> window([[InfoBubbleWindow alloc]
       initWithContentRect:ui::kWindowSizeDeterminedLater
                 styleMask:NSBorderlessWindowMask
                   backing:NSBackingStoreBuffered
                     defer:YES]);
   if (!window.get())
     return nil;
 
   anchoredAt = [parentWindow convertBaseToScreen:anchoredAt];
   if ((self = [super initWithWindow:window
                        parentWindow:parentWindow
                          anchoredAt:anchoredAt])) {
-    host_.reset(host);
-    extensionId_ = host_->extension_id();
     beingInspected_ = devMode;
     ignoreWindowDidResignKey_ = NO;
-
-    InfoBubbleView* view = self.bubble;
-    [view setArrowLocation:arrowLocation];
-
-    extensionView_ = host->view()->GetNativeView();
-    container_.reset(new ExtensionPopupContainer(self));
-    static_cast<ExtensionViewMac*>(host->view())
-        ->set_container(container_.get());
-
-    NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
-    [center addObserver:self
-               selector:@selector(extensionViewFrameChanged)
-                   name:NSViewFrameDidChangeNotification
-                 object:extensionView_];
-
-    [view addSubview:extensionView_];
-
-    notificationBridge_.reset(new DevtoolsNotificationBridge(self));
-    registrar_.reset(new content::NotificationRegistrar);
-    if (beingInspected_) {
-      // Listen for the extension to finish loading so the dev tools can be
-      // opened.
-      registrar_->Add(notificationBridge_.get(),
-                      extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
-                      content::Source<BrowserContext>(host->browser_context()));
-    }
+    [[self bubble] setArrowLocation:arrowLocation];
   }
   return self;
 }
 
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
   [super dealloc];
 }
 
 - (void)showDevTools {
@@ skipping 68 matching lines @@
                              devMode:(BOOL)devMode {
   DCHECK([NSThread isMainThread]);
   DCHECK(browser);
   if (!browser)
     return nil;
 
   // If we click the browser/page action again, we should close the popup.
   // Make Mac behavior the same with Windows and others.
   if (gPopup) {
     std::string extension_id = url.host();
-    extensions::ExtensionViewHost* host = [gPopup extensionViewHost];
-    if (extension_id == host->extension_id()) {
-      [gPopup close];
+    std::string old_extension_id = [gPopup extensionViewHost]->extension_id();
+    [gPopup close];
+    DCHECK(!gPopup);

[tapted, 2015/02/19 07:38:17]

tl;dr - I had to remove the DCHECK

So this turned out to be a bit more complicated :/

I ran an asan bot in http://crrev.com/940513003 and was surprised to find that
it didn't fail. Then, this DCHECK actually would fail when run here on this CL.

Only, the DCHECK didn't get hit when I tried to reproduce the scenario here in a
spot check in Chromium before uploading. Turns out, this is because clicking the
same button twice didn't actually call showURL again. Instead clicking the same
button twice triggers a close through windowDidResignKey:, and showURL isn't
called. But I did later find out that having two buttons and alternating *does*
go though this path with a failed DCHECK.

So.. this isn't dead code, but what's going on.

I thought [gPopup close] would synchronously call [ExtensionPopupController
windowWillClose:]. The delegate was still set, and my experience has always had
[NSWindow close] calling [delegate windowWillClose:] synchronously.

gPopup is an ExtensionPopupController which inherits from BaseBubbleController
which inherits form NSWindowController, they all override `close` and it will
trickle up to -[NSWindowController close]. However, BaseBubbleController
controls an InfoBubbleWindow which overrides [NSWindow close] and *doesn't* call
[super close]. Instead it starts an animation.

Except... these popups don't animate when closing :'(. It looks like they're
meant to fade out over 0.15 seconds. But currently they just disappear then
close themselves after 0.15 seconds. So, that's a bug. Filed
http://crbug.com/459881

Checking other platforms... They do fade out. But! pressing the same button
twice doesn't toggle properly. So that's another bug (ugh.) Filed
http://crbug.com/459895


So. Back to this.

Overriding [NSWindow close] and not calling [super close] feels a bit weird, but
I guess it works. It just means gPopup won't be set nil until the animation
finishes. So, it wasn't a UAF after all, but InfoBubbleWindow is able to call
close synchronously as well, so better not to assume.

ExtensionPopupController windowWillClose: does get called reliably (just.. not
in tests since they don't wait for the animation, and it's a cross-platform test
so ui::CocoaTest isn't checking for stray windows). So, I'm not worried about
gPopup being non-nil at this point. windowWillClose: also does a `gPopup ==
self` check before doing things, so I think that's all fine w.r.t. accessing the
right things too.

+    if (extension_id == old_extension_id)
       return nil;
-    }
   }
 
-  extensions::ExtensionViewHost* host =
-      extensions::ExtensionViewHostFactory::CreatePopupHost(url, browser);
+  // Create the popup first. This establishes an initially hidden NSWindow so
+  // that the renderer is able to gather correct screen metrics for the initial
+  // paint.
+  gPopup = [[ExtensionPopupController alloc]
+      initWithParentWindow:browser->window()->GetNativeWindow()
+                anchoredAt:anchoredAt
+             arrowLocation:arrowLocation
+                   devMode:devMode];
+
+  scoped_ptr<extensions::ExtensionViewHost> host(
+      extensions::ExtensionViewHostFactory::CreatePopupHost(url, browser));
   DCHECK(host);
-  if (!host)
-    return nil;
-
-  [gPopup close];
-
-  // Takes ownership of |host|. Also will autorelease itself when the popup is
-  // closed, so no need to do that here.
-  gPopup = [[ExtensionPopupController alloc]
-      initWithHost:host
-      parentWindow:browser->window()->GetNativeWindow()
-        anchoredAt:anchoredAt
-     arrowLocation:arrowLocation
-           devMode:devMode];
+  [gPopup setExtensionViewHost:host.Pass()];
   return gPopup;
 }
 
 + (ExtensionPopupController*)popup {
   return gPopup;
 }
 
+- (void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host {
+  DCHECK(!host_);
+  DCHECK(host);
+  host_.swap(host);
+
+  extensionId_ = host_->extension_id();
+  container_.reset(new ExtensionPopupContainer(self));
+  ExtensionViewMac* hostView = static_cast<ExtensionViewMac*>(host_->view());
+  hostView->set_container(container_.get());
+  hostView->CreateWidgetHostViewIn([self bubble]);
+
+  extensionView_ = hostView->GetNativeView();
+
+  NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
+  [center addObserver:self
+             selector:@selector(extensionViewFrameChanged)
+                 name:NSViewFrameDidChangeNotification
+               object:extensionView_];
+
+  notificationBridge_.reset(new DevtoolsNotificationBridge(self));
+  registrar_.reset(new content::NotificationRegistrar);
+  if (beingInspected_) {
+    // Listen for the extension to finish loading so the dev tools can be
+    // opened.
+    registrar_->Add(notificationBridge_.get(),
+                    extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+                    content::Source<BrowserContext>(host_->browser_context()));
+  }
+}
+
 - (void)extensionViewFrameChanged {
   // If there are no changes in the width or height of the frame, then ignore.
   if (NSEqualSizes([extensionView_ frame].size, extensionFrame_.size))
     return;
 
   extensionFrame_ = [extensionView_ frame];
   // Constrain the size of the view.
   [extensionView_ setFrameSize:NSMakeSize(
       Clamp(NSWidth(extensionFrame_),
             ExtensionViewMac::kMinWidth,
@@ skipping 47 matching lines @@
   // the notification is fired (and consequently the view contents have loaded).
   if (![window isVisible]) {
     [self showWindow:self];
   }
 }
 
 - (void)onSizeChanged:(NSSize)newSize {
   // When we update the size, the window will become visible. Stay hidden until
   // the host is loaded.
   pendingSize_ = newSize;
-  if (!host_->did_stop_loading())
+  if (!host_ || !host_->did_stop_loading())
     return;
 
   // No need to use CA here, our caller calls us repeatedly to animate the
   // resizing.
   NSRect frame = [extensionView_ frame];
   frame.size = newSize;
 
   // |new_size| is in pixels. Convert to view units.
   frame.size = [extensionView_ convertSize:frame.size fromView:nil];
 
@@ skipping 37 matching lines @@
   return minSize;
 }
 
 // Private (TestingAPI)
 + (NSSize)maxPopupSize {
   NSSize maxSize = {ExtensionViewMac::kMaxWidth, ExtensionViewMac::kMaxHeight};
   return maxSize;
 }
 
 @end