out/component/browser_tests --gtest_filter=UnloadTest.BrowserCloseNoUnloadListeners

The test code says

  void LoadUrlAndQuitBrowser(const char* html_content,
                             const char* expected_title) {
    NavigateToDataURL(html_content, expected_title);
    content::WindowedNotificationObserver window_observer(
        chrome::NOTIFICATION_BROWSER_CLOSED,
        content::NotificationService::AllSources());
    chrome::CloseWindow(browser());
    window_observer.Wait();
  }

From brwoser_commands.cc:

void CloseWindow(Browser* browser) {
  base::RecordAction(UserMetricsAction("CloseWindow"));
  browser->window()->Close();
}


In browser_window_cocoa.mm:

void BrowserWindowCocoa::Close() {
...
        [window() orderOut:nil];
        [window() close];
...
}

and I confirmed window() is non-null. chrome::NOTIFICATION_BROWSER_CLOSED only gets sent in browser_list.cc's BrowserList::RemoveBrowser(), which only gets called from browser.cc's Browser::~Browser(). I confirmed that the destructor isn't being called, so the test is waiting forever (although the window does go offscreen).

That means the BrowserWindowController isn't getting released, since that owns the Browser and BrowserWindow. Which means this may be related to (AppKit-induced?) over-retained windows, which we think may be responsible for  issue 748518  and  issue 749176 .

Right after the [window() close] I tried adding a bunch of [window() release]es (it started out as just one), and I didn't get a test completion or a crash. It seems like the window is being retained in some special way vs. just an unbalanced -retain somewhere in the AppKit.

/sub :)

(I'll try out the unit test in  Issue 748518  under Instruments today.. finally got our High Sierra machine updated with all the bits yesterday )

tl;dr I poked a bit, but I think this needs an ObjC/AutoreleasePool expert to take the next look.


The unit test in  Issue 748518  was a bust - that issue seems to have been fixed by Apple.

Starting up a browser test in Instruments was a lot more painful but I managed it in the end.

Attaching a list of what seems to show the problem. I'm not really sure how to interpret it though. It looks like a whole bunch of calls to autorelease are never actually getting drained. 

The suspicious actor seems to be 

   0 AppKit -[NSWindowController release]
   1 libobjc.A.dylib (anonymous namespace)::AutoreleasePoolPage::pop(void*)
   2 AppKit -[NSView(NSInternal) _recursive:displayRectIgnoringOpacity:inGraphicsContext:shouldChangeFontReferenceColor:]
   3 AppKit -[NSView(NSInternal) _recursive:displayRectIgnoringOpacity:inContext:shouldChangeFontReferenceColor:]
   4 AppKit __46-[NSView(NSLayerKitGlue) drawLayer:inContext:]_block_invoke
   5 AppKit -[NSView(NSLayerKitGlue) _drawViewBackingLayer:inContext:drawingHandler:]
   6 AppKit -[NSView(NSLayerKitGlue) drawLayer:inContext:]
   7 QuartzCore CABackingStoreUpdate_
   8 QuartzCore invocation function for block in CA::Layer::display_()
   9 QuartzCore x_blame_allocations
  10 QuartzCore -[CALayer _display]
  11 AppKit _NSBackingLayerDisplay
  12 AppKit -[_NSViewBackingLayer display]
  13 QuartzCore CA::Layer::display_if_needed(CA::Transaction*)
  14 QuartzCore CA::Layer::layout_and_display_if_needed(CA::Transaction*)
  15 QuartzCore CA::Context::commit_transaction(CA::Transaction*)
  16 QuartzCore CA::Transaction::commit()
  17 QuartzCore CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
  18 CoreFoundation __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__


One time this is invoked, it releases some of the autoreleased objects retained since the prior drain, but nowhere near enough. Specifically, after event #4011, that call only drains to a refcount of 63 and never seems to recover. Leaving BrowserWindowController with 56 references, and no call to dealloc at the end.



To repro this, I'm doing

$ ./interactive_ui_tests --gtest_filter=PermissionBubbleInteractiveUITest.CmdWClosesWindow/Cocoa --single_process --test-launcher-timeout=-1

and, in browswer_window_factory_cocoa.mm, doing

BrowserWindow* BrowserWindow::CreateBrowserWindow(Browser* browser,
                                                  bool user_gesture) {
  //base::debug::BreakDebugger();
  char c;
  DLOG(INFO) << "Input plz";
  std::cin.get(c);
  BrowserWindowController* controller =
      [[BrowserWindowController alloc] initWithBrowser:browser];
  return [controller browserWindow];
}

to pause the process and give Instruments a chance to connect.

Deleted: BrowserWindowController-allocations.txt 96.1 KB

BrowserWindowController-allocations.txt 96.1 KB View Download

Temporarily -> tapted@ for filing rdar w/Apple.

Filed rdar://33641823 -> https://bugreport.apple.com/web/?problemID=33641823 "[Chrome] HighSierra: Possible leaked autorelease page under -[NSView(NSInternal) _recursive:displayRectIgnoringOpacity..]"

Thank you tapted@ for filing the rdar.

Found in the AppKit release notes - https://developer.apple.com/library/content/releasenotes/AppKit/RN-AppKit/index.html :

NSWindow Lifecycle Changes (Updated since Seed 2)
If your application is linked on macOS 10.13 SDK or later, NSWindows that are ordered-in will be strongly referenced by AppKit, until they are explicitly ordered-out or closed (not including hide or minimize operations). This means that, generally, an on-screen window will not be deallocated (and close/order-out as a side-effect of deallocation).



Of course... these failing tests are almost certainly invoking [NSWindow close], and I'm pretty sure this happens whether or not linking occurs against 10.13. But "Something Has Changed" around lifetimes, somewhere in AppKit, and that changed logic might be tickling this in some way.

I dug into this today.

The issue is that the BrowserWindowCOntroller is never -dealloc'ed even though it explicitly autoreleases itself at the end of -windowWillClose:. My guess is that the tainting that 10.13 has done with keeping strong references to windows has bleed over to their window controllers (i.e. if you're going to maintain a strong reference to a window you or the window probably need to do the same to its window controller, otherwise you could get into trouble).

The good news is at least when we hit windowWillClose we know we need to clean up the BrowserWindowController and its supporting objects - we can actually start the cleanup process there.

The bad news is there's no easy way to clean up the Browser and other references (right now they are freed as a result of being C++ ivars when the BrowserWindowController instance gets dealloc'ed). Options are:

1.Explicitly .reset() the BWC ivars within windowWillClose, or some method that has this purpose (that subclasses can override to add their ivars). I did a quick-and-dirty POC of this and the tests no longer hang waiting for windows to close (some tests are crashing, but I think that's because I didn't do all the cleanup I needed to). This is brittle, however, because you have to be sure to add resets for any new ivars you add in the future, in base and subclasses.

2. Create a new object that holds all or most of BWC's ivars. Freeing this one ivar would free all of the ivars it holds.

3. Break BWC into two classes, one that is strictly a Cocoa window controller, and another which contains all of the non-window controller logic and ivars currently in BWC. When the stripped down window controller receives windowWillClose it can free the other object to clean up.

3. maybe results in a clearer separation of duties within the code but would be a lot of work. 2. is probably the best of these.

Other thoughts?
 

Thanks for doing the investigation.

I agree that (3) is probably the best solution, but it would indeed be a lot of work (which may be obsoleted by MacViewsBrowser sometime in the future, anyways). I lean more towards (1) rather than (2), personally. I also agree we should move this logic into a separate method (probably -[BrowserWindowController browserWillBeDestroyed]) that is called from -windowWillClose:. My only concern is that there may be things in BWC that always expect a browesr_, but those should be easy to fix.

One more option would be to look at writing something like base/mac/objc_release_properties.mm, but rather than iterating over the property list, we could iterate over the ivar list and reset any scoped_nsobject or scoped_ptrs automatically, so that they would not have to be manually reset.

> One more option would be to look at writing something like base/mac/objc_release_properties.mm, but rather than iterating over the property list, we could iterate over the ivar list and reset any scoped_nsobject or scoped_ptrs automatically, so that they would not have to be manually reset.

I looked for something like this and was bummed it didn't seem to exist. How would you do this kind of object introspection in C++?

We wouldn't be introspecting a C++ object (since we don't preserve RTTI information), but rather the ObjC object which does record the type of the ivar (as part of the non-fragile ObjC 2.0 ABI). Such code would look almost identical to ReleaseProperties(), except it'd use class_copyIvarList() instead of class_copyPropertyList().

Maybe something like...

struct BrowserTiedData {
  BrowserTiedData() = default;

  std::unique_ptr<Browser> browser_;
  std::unique_ptr<BrowserWindowCocoa> windowShim_;
  std::unique_ptr<BookmarkBubbleObserverCocoa> bookmarkBubbleObserver_;

  // The Extension Command Registry used to determine which keyboard events to
  // handle.
  std::unique_ptr<ExtensionKeybindingRegistryCocoa>
      extensionKeybindingRegistry_;

  // Observes whether the omnibox popup is shown or hidden.
  std::unique_ptr<OmniboxPopupModelObserverBridge>
      omniboxPopupModelObserverBridge_;
};

@interface BrowserWindowController ... {
 BrowserData data;
}

- (void)windowWillClose:(NSNotification*)notification {
  ...
 data = BrowserTiedData();
}

?

tapted@ - your suggestion in c#17 is clean for freeing everything at once, but rsesek@ pointed out it means prefixing all existing ivar references with "data.".

I explored making TabWindowController an NSObject subclass instead of an NSWindowController. It turned out to be not that much work to tease the classes apart, but TabWindowController and BrowserWindowController implement NSWindowDelegate classes. Setting the window's delegate to the TabWindowController seems to create the same lifetime problems, which again makes sense (if you're going to keep the window around indefinitely you need to do the same with its delegate).

I confirmed this by creating an NSWindowController subclass that has a pointer to the TabWindowController, and making it the window's windowController and delegate. I implemented a couple of the NSWindowDelegate methods to forward the messages to the TabWindowController, and the browser test that had been hanging now completes and exits.

So one solution would then be to implement all of the NSWindowDelegate methods so they get forwarded to the TabWindowController/BrowserWindowController (checking that they implement them). Seems kind of ugly, and perhaps fragile unless we assume the list of NSWindowDelegate methods will not grow in the future. It seems like we should be able to override respondsToSelector: and forwardInvocation: to forward all NSWindowDelegate protocol methods but my quick-and-dirty attempt at that didn't work.

Can any of the NSWindowDelegate methods be replaced with the NSWindow notifications?

I don't think manually forwarding methods would be the worst thing, either, though.

> Can any of the NSWindowDelegate methods be replaced with the NSWindow notifications?

Yes, but not all, such as returning the window's field editor.

> I don't think manually forwarding methods would be the worst thing, either, though.

OK, if it doesn't seem to horrible I can go down that path.

I got the message forwarding working.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dec06ed3705dc353977df4441eed7cb16d170927

commit dec06ed3705dc353977df4441eed7cb16d170927
Author: Jayson Adams <shrike@chromium.org>
Date: Mon Jan 22 17:52:48 2018

[Mac] Fix browser window controller lifetime issues in 10.13

In High Sierra (macOS 10.13), the AppKit retains windows and their
NSWindowControllers such that closing a window no longer causes the
window contrller to free immediately. TabWindowController, a subclass of
NSWindowController, cleans up browser objects in dealloc. As a result of
this lifetime change, TabWindowController/BrowserWindowController are
no longer freed on window close and therefore never clean up, ultimately
causing some browser tests to hang.

This cl changes the TabWindowController base class from
NSWindowController to NSObject, so it no longer relies on the window's
controller's lifetime to clean up.

Bug:  749196 
Change-Id: Ibcaf69119f7fc7578ff971ca80bb618768a930f6
Reviewed-on: https://chromium-review.googlesource.com/852978
Commit-Queue: Jayson Adams <shrike@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530907}
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/autofill/save_card_bubble_view_unittest.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_command_handler.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_controller.h
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_controller_private.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/browser_window_controller_unittest.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/bubble_anchor_helper.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/chrome_browser_window.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/framed_browser_window.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/share_menu_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/tabbed_browser_window.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/tabs/tab_window_controller.h
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/tabs/tab_window_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm
[modify] https://crrev.com/dec06ed3705dc353977df4441eed7cb16d170927/chrome/browser/ui/cocoa/view_id_util.mm

 Issue 749192  has been merged into this issue.

 Issue 749796  has been merged into this issue.

 Issue 749795  has been merged into this issue.