Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809543003:
[ios] Change WebStateListObserverBridge to use weak reference. (Closed)
|
Created:
3 years, 8 months ago by sdefresne Modified:
3 years, 8 months ago Reviewers:
rohitrao (ping after 24h) CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Change WebStateListObserverBridge to use weak reference.
For consistency with the other observer bridges, change the API of
WebStateListObserverBridge to *not* own the Objective-C observer,
and change TabModel/WebStateListFastEnumerationHelper to respect
the new API.
Note: the other client of WebStateListObserverBridge used the
API as if the id<WebStateListObserving> was not owned, thus
leaked both objects. No code change is required there.
BUG=709984
Review-Url: https://codereview.chromium.org/2809543003
Cr-Commit-Position: refs/heads/master@{#464029}
Committed: https://chromium.googlesource.com/chromium/src/+/3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase. #
Total comments: 2
Patch Set 3 : Rebase on origin/master. #Patch Set 4 : Fix DCHECK in WebStateList's ObserverList destructor. #
Messages
Total messages: 24 (16 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. https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... File ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h:70: // implements the WebStateListObserver protocol (the observer is *not* owned). I wonder why I bother writing documentation, all clients of WebStateListObserverBridge (except TabModel and WebStateListFastEnumerationHelper) passed self to WebStateListObserverBridge while owning the WebStateListObserverBridge :-(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... File ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h:70: // implements the WebStateListObserver protocol (the observer is *not* owned). On 2017/04/10 15:34:49, sdefresne wrote: > I wonder why I bother writing documentation, all clients of > WebStateListObserverBridge (except TabModel and > WebStateListFastEnumerationHelper) passed self to WebStateListObserverBridge > while owning the WebStateListObserverBridge :-( FWIW, I didn't see this comment because I skipped all the way down to the ivar on line 104 and looked for this information there =) https://codereview.chromium.org/2809543003/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2809543003/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:295: base::scoped_nsobject<TabModelClosingWebStateObserver> Should we just have individual ivars for the three observers, rather than putting them into |retainedWebStateListObservers|?
https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... File ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h (right): https://codereview.chromium.org/2809543003/diff/1/ios/chrome/browser/web_stat... ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h:70: // implements the WebStateListObserver protocol (the observer is *not* owned). On 2017/04/11 12:51:42, rohitrao wrote: > On 2017/04/10 15:34:49, sdefresne wrote: > > I wonder why I bother writing documentation, all clients of > > WebStateListObserverBridge (except TabModel and > > WebStateListFastEnumerationHelper) passed self to WebStateListObserverBridge > > while owning the WebStateListObserverBridge :-( > > FWIW, I didn't see this comment because I skipped all the way down to the ivar > on line 104 and looked for this information there =) I agree that the comment was redundant as all Objective-C pointers are strong pointers by default when the implementation file is compiled with ARC (which is the case here) unless marked as __weak/__unsafe_unretained :-) https://codereview.chromium.org/2809543003/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab_model.mm (right): https://codereview.chromium.org/2809543003/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab_model.mm:295: base::scoped_nsobject<TabModelClosingWebStateObserver> On 2017/04/11 12:51:42, rohitrao wrote: > Should we just have individual ivars for the three observers, rather than > putting them into |retainedWebStateListObservers|? They are never (and should never) be accessed as individual observers, so I don't think it is worth keeping direct pointers to them.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I added an @autoreleasepool in TabModel -dealloc to fix the unit tests. Could you take another look (just the difference between the last two patchsets).
lgtm but it seems like unregistering in dealloc might be fundamentally incorrect, since anything can keep refcounted objects alive beyond our control.
On 2017/04/12 15:04:46, rohitrao wrote: > lgtm but it seems like unregistering in dealloc might be fundamentally > incorrect, since anything can keep refcounted objects alive beyond our control. Ack. Proper fix would be to remove NSFastEnumeration on TabModel.
The CQ bit was checked by sdefresne@chromium.org
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": 60001, "attempt_start_ts": 1492009821637950,
"parent_rev": "b55da6367aadc5c1e7c0c5769f6d32fef5249fa0", "commit_rev":
"3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Change WebStateListObserverBridge to use weak reference. For consistency with the other observer bridges, change the API of WebStateListObserverBridge to *not* own the Objective-C observer, and change TabModel/WebStateListFastEnumerationHelper to respect the new API. Note: the other client of WebStateListObserverBridge used the API as if the id<WebStateListObserving> was not owned, thus leaked both objects. No code change is required there. BUG=709984 ========== to ========== [ios] Change WebStateListObserverBridge to use weak reference. For consistency with the other observer bridges, change the API of WebStateListObserverBridge to *not* own the Objective-C observer, and change TabModel/WebStateListFastEnumerationHelper to respect the new API. Note: the other client of WebStateListObserverBridge used the API as if the id<WebStateListObserving> was not owned, thus leaked both objects. No code change is required there. BUG=709984 Review-Url: https://codereview.chromium.org/2809543003 Cr-Commit-Position: refs/heads/master@{#464029} Committed: https://chromium.googlesource.com/chromium/src/+/3b0155e9b6ab3719ce1d1fda5cdb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3b0155e9b6ab3719ce1d1fda5cdb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
