Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885803002:
[ios] Remove TabModel cleanup from -dealloc to -browserStateDestroyed. (Closed)
|
Created:
3 years, 7 months ago by sdefresne Modified:
3 years, 7 months ago Reviewers:
rohitrao (ping after 24h) CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Remove TabModel cleanup from -dealloc to -browserStateDestroyed.
As the -dealloc is called at a later time with ARC, move the cleanup
from -dealloc to -browserStateDestroyed to ensure it is determinically
called even if ARC decide to -autorelease TabModel.
Cleanup some of Tab ivar when the WebState is destroyed so that the
corresponding Tab helpers -dealloc is called sooner.
BUG=None
Review-Url: https://codereview.chromium.org/2885803002
Cr-Commit-Position: refs/heads/master@{#473892}
Committed: https://chromium.googlesource.com/chromium/src/+/0531535acb8768a164a255cf752fce224df75ddb
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase #
Dependent Patchsets: Messages
Total messages: 22 (9 generated)
The CQ bit was checked by sdefresne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdefresne@chromium.org changed reviewers: + rohitrao@chromium.org
Please take a look.
lgtm
https://codereview.chromium.org/2885803002/diff/1/ios/chrome/browser/tabs/tab... File ios/chrome/browser/tabs/tab_model.mm (left): https://codereview.chromium.org/2885803002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab_model.mm:195: // removeObserver: to be called first otherwise a lot of unecessary work will We are changing the semantics here. It seems likely that observers will still exist when we call closeAllTabs. Is that ok? Is there any way to restore the existing behavior?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/16 13:28:11, rohitrao (ping after 24h) wrote: > https://codereview.chromium.org/2885803002/diff/1/ios/chrome/browser/tabs/tab... > File ios/chrome/browser/tabs/tab_model.mm (left): > > https://codereview.chromium.org/2885803002/diff/1/ios/chrome/browser/tabs/tab... > ios/chrome/browser/tabs/tab_model.mm:195: // removeObserver: to be called first > otherwise a lot of unecessary work will > We are changing the semantics here. It seems likely that observers will still > exist when we call closeAllTabs. Is that ok? Is there any way to restore the > existing behavior? I know that this CL changes semantic and more observers will be invoked. I've tried to change the client code to remove the observers before calling -browserStateDestroyed but this proved really tricky (as the -browserStateDestroyed is called by multiple objects for the same TabModel: mostly BrowserViewController and BrowserViewWrangler, and they only unregister their observer after some delay). Currently the problem only manifest when I'm trying to convert the Tab & TabModel unittests to ARC, so to fix those, I can just move the cleanup code to a -cleanup method of TabModel and call it from there, but I think it will also fail when we convert BVC or BVW to ARC later, so that would not really work. Another way I could fix this without introducing a semantic change is to remove the TabModelObserversBridge from WebStateList observer. Then the TabModelObservers would no longer be invoked during the closeAllTabs method. This is a bit trickier to implement (not that much complex though, just need to store the std::unique_ptr<> out of _webStateListObservers). WDYT?
> I know that this CL changes semantic and more observers will be invoked. Should we break this up into two pieces? Move the non-contentious pieces in one CL, then move closeAllTabs and webStateListObservers in a second CL? Would that help isolate and mitigate risk?
On 2017/05/16 14:01:39, rohitrao (ping after 24h) wrote: > > I know that this CL changes semantic and more observers will be invoked. > > Should we break this up into two pieces? Move the non-contentious pieces in one > CL, then move closeAllTabs and webStateListObservers in a second CL? Would that > help isolate and mitigate risk? The problem is that if TabModel -dealloc is called after ChromeBrowserState destructor, then AutofillController -dealloc is not called early enough and the ObserverList owned by SigninManagerBase complains about the presence of one observer in the list when it is destroyed. The AutofillController is destroyed in Tab -webStateDestroyed: (this only -autorelease the AutofillController, but since this is called in an @autoreleasepool, this works) which is called from CloseAllTabs (indirectly when the WebStates are destroyed).
On 2017/05/16 14:21:48, sdefresne wrote: > On 2017/05/16 14:01:39, rohitrao (ping after 24h) wrote: > > > I know that this CL changes semantic and more observers will be invoked. > > > > Should we break this up into two pieces? Move the non-contentious pieces in > one > > CL, then move closeAllTabs and webStateListObservers in a second CL? Would > that > > help isolate and mitigate risk? > > The problem is that if TabModel -dealloc is called after ChromeBrowserState > destructor, then AutofillController -dealloc is not called early enough and the > ObserverList owned by SigninManagerBase complains about the presence of one > observer in the list when it is destroyed. The AutofillController is destroyed > in Tab -webStateDestroyed: (this only -autorelease the AutofillController, but > since this is called in an @autoreleasepool, this works) which is called from > CloseAllTabs (indirectly when the WebStates are destroyed). So, I tried to move this to a separate method, but I get many DCHECK failures during app shutdown. BVW does the following (with many other calls in between): [mainTabModel_ browserStateDestroyed]; [mainTabModel_ removeObserver: ...]; [mainTabModel_ closeAllTabs]; mainTabModel_.reset(); So, I think that not calling the observers in closeAllTabs should be okay.
On 2017/05/16 16:23:27, sdefresne wrote: > On 2017/05/16 14:21:48, sdefresne wrote: > > On 2017/05/16 14:01:39, rohitrao (ping after 24h) wrote: > > > > I know that this CL changes semantic and more observers will be invoked. > > > > > > Should we break this up into two pieces? Move the non-contentious pieces in > > one > > > CL, then move closeAllTabs and webStateListObservers in a second CL? Would > > that > > > help isolate and mitigate risk? > > > > The problem is that if TabModel -dealloc is called after ChromeBrowserState > > destructor, then AutofillController -dealloc is not called early enough and > the > > ObserverList owned by SigninManagerBase complains about the presence of one > > observer in the list when it is destroyed. The AutofillController is destroyed > > in Tab -webStateDestroyed: (this only -autorelease the AutofillController, but > > since this is called in an @autoreleasepool, this works) which is called from > > CloseAllTabs (indirectly when the WebStates are destroyed). > > So, I tried to move this to a separate method, but I get many DCHECK failures > during app shutdown. > > BVW does the following (with many other calls in between): > > [mainTabModel_ browserStateDestroyed]; > [mainTabModel_ removeObserver: ...]; > [mainTabModel_ closeAllTabs]; > mainTabModel_.reset(); > > So, I think that not calling the observers in closeAllTabs should be okay. I've added some instrumentation of the observer methods called when the app does clean shutdown in the foreground with one tab open (to limit the number of events that are called). Without the changes, no events are called: TabModel -browserStateDestroyed TabModel -browserStateDestroyed TabModel -dealloc TabModel -cleanup With the changes, three events are called on BrowserViewController: TabModel -browserStateDestroyed TabModel -cleanup Calling @selector(tabModel:willRemoveTab:) on <BrowserViewController: ...> Calling @selector(tabModel:didRemoveTab:atIndex:) on <BrowserViewController: ...> Calling @selector(tabModelDidChangeTabCount:) on <BrowserViewController: ...> TabModel -browserStateDestroyed TabModel -cleanup TabModel -dealloc As far as I can tell, those are purely cosmetic changes. I'll see if I can tell BrowserViewController to unregister its observer before BVW calls -browserStateDestroyed on TabModel.
On 2017/05/17 14:21:05, sdefresne wrote: > On 2017/05/16 16:23:27, sdefresne wrote: > > On 2017/05/16 14:21:48, sdefresne wrote: > > > On 2017/05/16 14:01:39, rohitrao (ping after 24h) wrote: > > > > > I know that this CL changes semantic and more observers will be invoked. > > > > > > > > Should we break this up into two pieces? Move the non-contentious pieces > in > > > one > > > > CL, then move closeAllTabs and webStateListObservers in a second CL? > Would > > > that > > > > help isolate and mitigate risk? > > > > > > The problem is that if TabModel -dealloc is called after ChromeBrowserState > > > destructor, then AutofillController -dealloc is not called early enough and > > the > > > ObserverList owned by SigninManagerBase complains about the presence of one > > > observer in the list when it is destroyed. The AutofillController is > destroyed > > > in Tab -webStateDestroyed: (this only -autorelease the AutofillController, > but > > > since this is called in an @autoreleasepool, this works) which is called > from > > > CloseAllTabs (indirectly when the WebStates are destroyed). > > > > So, I tried to move this to a separate method, but I get many DCHECK failures > > during app shutdown. > > > > BVW does the following (with many other calls in between): > > > > [mainTabModel_ browserStateDestroyed]; > > [mainTabModel_ removeObserver: ...]; > > [mainTabModel_ closeAllTabs]; > > mainTabModel_.reset(); > > > > So, I think that not calling the observers in closeAllTabs should be okay. > > I've added some instrumentation of the observer methods called when the app does > clean shutdown in the foreground with one tab open (to limit the number of > events that are called). Without the changes, no events are called: > > TabModel -browserStateDestroyed > TabModel -browserStateDestroyed > TabModel -dealloc > TabModel -cleanup > > With the changes, three events are called on BrowserViewController: > > TabModel -browserStateDestroyed > TabModel -cleanup > Calling @selector(tabModel:willRemoveTab:) on <BrowserViewController: ...> > Calling @selector(tabModel:didRemoveTab:atIndex:) on <BrowserViewController: > ...> > Calling @selector(tabModelDidChangeTabCount:) on <BrowserViewController: ...> > TabModel -browserStateDestroyed > TabModel -cleanup > TabModel -dealloc > > As far as I can tell, those are purely cosmetic changes. I'll see if I can tell > BrowserViewController to unregister its observer before BVW calls > -browserStateDestroyed on TabModel. So I can get the same event with or without my CL by explicitly dropping references to mainBVC and otrBVC in BrowserViewWrangler -dealloc method, just before calling TabModel -browserStateDestroyed. However, if I do that, some of the object do not unregister correctly as they are unregistered in their -dealloc or in response to events that are invoked when the Tab are closed. So I think the proper fix is to call the -closeAllTabs method in -browserStateDestroyed. WDYT?
Can we clearly document the expected cost of this change (as part of this thread is fine)? Is there any question of affecting correctness, or is this just an efficiency optimization? lgtm still holds
On 2017/05/18 11:39:16, rohitrao (ping after 24h) wrote: > Can we clearly document the expected cost of this change (as part of this thread > is fine)? Is there any question of affecting correctness, or is this just an > efficiency optimization? > > lgtm still holds This is for correctness. Some objects only register themselves from BrowserState service when: 1. they are deallocated, 2. they are notified the Tab is closed. Since Tab and TabModel have been changed to use ARC, the 1. happens with a delay which cause DCHECK during shutdown. Explicitly closing all the Tabs with the observer still installed ensure that 2. happens.
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2885803002/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495549980806630,
"parent_rev": "d57a7acae403b7436284e6f15fe85786eb657e54", "commit_rev":
"0531535acb8768a164a255cf752fce224df75ddb"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Remove TabModel cleanup from -dealloc to -browserStateDestroyed. As the -dealloc is called at a later time with ARC, move the cleanup from -dealloc to -browserStateDestroyed to ensure it is determinically called even if ARC decide to -autorelease TabModel. Cleanup some of Tab ivar when the WebState is destroyed so that the corresponding Tab helpers -dealloc is called sooner. BUG=None ========== to ========== [ios] Remove TabModel cleanup from -dealloc to -browserStateDestroyed. As the -dealloc is called at a later time with ARC, move the cleanup from -dealloc to -browserStateDestroyed to ensure it is determinically called even if ARC decide to -autorelease TabModel. Cleanup some of Tab ivar when the WebState is destroyed so that the corresponding Tab helpers -dealloc is called sooner. BUG=None Review-Url: https://codereview.chromium.org/2885803002 Cr-Commit-Position: refs/heads/master@{#473892} Committed: https://chromium.googlesource.com/chromium/src/+/0531535acb8768a164a255cf752f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0531535acb8768a164a255cf752f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
