New issue
Advanced search Search tips

Issue 722879 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug

Blocked on:
issue 723633



Sign in to add a comment

Chrome on iOS clean shutdown should not DCHECK / crash

Project Member Reported by sdefresne@chromium.org, May 16 2017

Issue description

With migration to ARC, the clean shutdown DCHECK / crash. This should be fixed.

☂ bug to track fixing those issues.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 17 2017

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

commit 8ac159105b78c8987a16faa4113d18b37682566b
Author: sdefresne <sdefresne@chromium.org>
Date: Wed May 17 13:21:06 2017

[ios] Fix clean shutdown of Chrome on iOS.

Drop the references to SpotlightManager and BrowserViewWrangler in
MainController -stopChromeMain before destroying IOSChromeMain as
IOSChromeMain leads to the destruction of ChromeBrowserState and
both SpotlightManager and BrowserViewWrangler must not outlive the
ChromeBrowserState.

Fix SpotlightTopSitesBridge destructor by keeping a reference to
history::TopSites as __weak pointer are set to nil before the ivar
are destroyed, thus owner_->topSites is null by that point.

Fix ToolsMenuButtonObserverBridge to destroy the observer bridge
when setting _model to null to avoid DCHECK.

BUG= 722879 

Review-Url: https://codereview.chromium.org/2883323002
Cr-Commit-Position: refs/heads/master@{#472437}

[modify] https://crrev.com/8ac159105b78c8987a16faa4113d18b37682566b/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/8ac159105b78c8987a16faa4113d18b37682566b/ios/chrome/app/spotlight/topsites_spotlight_manager.mm
[modify] https://crrev.com/8ac159105b78c8987a16faa4113d18b37682566b/ios/chrome/browser/ui/toolbar/tools_menu_button_observer_bridge.mm

Comment 2 by pkl@chromium.org, May 17 2017

Blockedon: 723633
Components: Internals
Project Member

Comment 4 by bugdroid1@chromium.org, May 24 2017

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

commit bd06230f1936e9f11cea89ffa8be83813e80007e
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed May 24 15:31:42 2017

[ios] Proper cleanup of SpotlightManager.

As SpotlightManager depends on ChromeBrowserState's service add
a -shutdown method to control when the instance unregister from
the services to avoid depending on when -dealloc is called.

BUG= 722879 

Change-Id: Ifa3c627ff9ba5ef0da03816f9ad1400e9c1df04a
Reviewed-on: https://chromium-review.googlesource.com/514065
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#474295}
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/base_spotlight_manager.h
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/base_spotlight_manager.mm
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/bookmarks_spotlight_manager.h
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/bookmarks_spotlight_manager.mm
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/spotlight_manager.h
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/spotlight_manager.mm
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/spotlight_manager_unittest.mm
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/topsites_spotlight_manager.h
[modify] https://crrev.com/bd06230f1936e9f11cea89ffa8be83813e80007e/ios/chrome/app/spotlight/topsites_spotlight_manager.mm

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 12 2017

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

commit 1c7e3e09531837467cb3f412a72e373853e575aa
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Mon Jun 12 14:55:06 2017

[ios] Ensure proper shutdown of Chrome on iOS.

As ARC uses -autorelease (or at least the method used has similar
effect), the -dealloc method call may be delayed. This does not
mix well with C++ memory management.

Add -shutdown method to ensure that Objective-C objects that need
to deterministically deregister themselves from C++ objects can
do it.

BUG= 722879 

Change-Id: Ifdbddd584790b0a4d73029d92c3ecd8df413827e
Reviewed-on: https://chromium-review.googlesource.com/528134
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478621}
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/app/main_controller_unittest.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/browser_view_controller.h
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/main/browser_view_wrangler.h
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/main/browser_view_wrangler.mm
[modify] https://crrev.com/1c7e3e09531837467cb3f412a72e373853e575aa/ios/chrome/browser/ui/main/browser_view_wrangler_unittest.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 15 2017

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

commit 41170aaa533cabaf2612204a1243f3d5d0732c6e
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Thu Jun 15 13:13:53 2017

[ios] Ensure proper shutdown of Chrome on iOS.

As ARC uses -autorelease (or at least the method used has similar
effect), the -dealloc method call may be delayed. This does not
mix well with C++ memory management.

Add -shutdown method to ensure that Objective-C objects that need
to deterministically deregister themselves from C++ objects can
do it.

BUG= 722879 

Change-Id: I2eebf29a510421ca0518a92535782cc61056ffd4
Reviewed-on: https://chromium-review.googlesource.com/532874
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479681}
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/app/main_controller_unittest.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/test/perf_test_with_bvc_ios.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/browser_view_controller.h
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/browser_view_controller.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/browser_view_controller_unittest.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/main/browser_view_wrangler.h
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/main/browser_view_wrangler.mm
[modify] https://crrev.com/41170aaa533cabaf2612204a1243f3d5d0732c6e/ios/chrome/browser/ui/main/browser_view_wrangler_unittest.mm

Status: Fixed (was: Assigned)

Sign in to add a comment