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

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

Extensions with background pages (e.g. Google Cast) currently have a
shorter code path when showing an extension popup that can cause the
renderer to ask for screen metrics before the hosting NSView is added to
the view hierarchy. Since the NSWindow is nil in this case, screen
metrics for the primary screen are used, and they may be the incorrect
screen for the popup.

On non-Mac, toolkit-views platforms get around this problem by deferring
creation until View::ViewHierarchyChanged is triggered. However, there
is no NSView apart from the render view on the extension-side on Mac, so
to do the same we'd either need to add one (and keep it sized
appropriately), or modify the WebContents view itself to override
[NSView viewDid/WillMoveToSuperview] and feed it through to embedders.

Instead, this CL tweaks the ExtensionPopupController initialization to
always create the popup NSWindow (initially hidden), before creating the
ExtensionViewHost.

BUG=324748, 305620
TEST=On a retina mac, plug in a (non-retina) external monitor, move
Chrome there, and open the Chromecast dialog. It should look "nice" (not
as nice as the retina screen, but consistent with other text in Chrome).
See http://crbug.com/324748#c41

Committed: https://crrev.com/836dc7a4b828bffdd351db39cb3738ad36dba332
Cr-Commit-Position: refs/heads/master@{#317011}

[tapted, 2014-10-14 07:05:15]

Patchset #2 (id:20001) has been deleted

[tapted, 2015-02-18 05:48:56]

tapted@chromium.org changed reviewers:
+ finnur@chromium.org, rsesek@chromium.org

[tapted, 2015-02-18 06:00:11]

Hi finnur and robert, please take a look

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (left):

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:189: [view
addSubview:extensionView_];
note this line is now moved to ExtensionViewMac::CreateWidgetHostViewIn

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:186:
hostView->CreateWidgetHostViewIn([self bubble]);
note this line is new

[Finnur, 2015-02-18 09:51:58]

Non-cocoa code: LGTM.

[Robert Sesek, 2015-02-18 14:56:20]

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:177: -
(void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host {
This method should be down around line 320.

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:297: [gPopup
close];
Move this above line 292 and then delete 293.

[tapted, 2015-02-18 22:27:58]

New patchsets have been uploaded after l-g-t-m from finnur@chromium.org

[tapted, 2015-02-18 22:35:28]

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:177: -
(void)setExtensionViewHost:(scoped_ptr<extensions::ExtensionViewHost>)host {
On 2015/02/18 14:56:20, Robert Sesek wrote:
> This method should be down around line 320.

Done.

https://codereview.chromium.org/653843002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:297: [gPopup
close];
On 2015/02/18 14:56:20, Robert Sesek wrote:
> Move this above line 292 and then delete 293.

Done.

[Robert Sesek, 2015-02-18 22:38:02]

https://codereview.chromium.org/653843002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:263: if
(extension_id == host->extension_id())
Actually, will -close also destruct the object and turn this into a UAF? If so,
this should get the extension_id before calling close and then do the
comparison.

[tapted, 2015-02-18 22:46:04]

https://codereview.chromium.org/653843002/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:263: if
(extension_id == host->extension_id())
On 2015/02/18 22:38:02, Robert Sesek wrote:
> Actually, will -close also destruct the object and turn this into a UAF? If
so,
> this should get the extension_id before calling close and then do the
> comparison.

ooh yes! good catch - I'm pretty sure it will. Hopefully an asan bot will see
this.. (nothing unexepected happened when I spot-checked the debug build).

Done. Also added a DCHECK(!gPopup) since that's a bit subtle and I think helps
hint that stuff has been freed.

[Robert Sesek, 2015-02-18 22:48:21]

LGTM

[tapted, 2015-02-19 07:34:37]

New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org

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

https://codereview.chromium.org/653843002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm (right):

https://codereview.chromium.org/653843002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_popup_controller.mm:263:
DCHECK(!gPopup);
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.

[tapted, 2015-02-19 08:14:42]

I think this CL is all good to go, but please yell at me if that's not the case,
or there's anything else I should follow-up on. Thanks!

[tapted, 2015-02-19 08:14:48]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-02-19 08:15:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/653843002/160001

[commit-bot: I haz the power, 2015-02-19 08:39:48]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-02-19 08:40:48]

Patchset 8 (id:??) landed as
https://crrev.com/836dc7a4b828bffdd351db39cb3738ad36dba332
Cr-Commit-Position: refs/heads/master@{#317011}

[Robert Sesek, 2015-02-19 16:14:46]

On 2015/02/19 08:14:42, tapted wrote:
> I think this CL is all good to go, but please yell at me if that's not the
case,
> or there's anything else I should follow-up on. Thanks!

What you wrote sounds fine. Thanks for filing the bugs!