New issue
Advanced search Search tips

Issue 709984 link

Starred by 1 user
Status: Fixed
Owner:
sdefresne@chromium.org
Closed: May 2017
Cc:
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Consider changing WebStateObserverListBridge to hold weak references

Project Member Reported by rohitrao@chromium.org, Apr 10 2017 
I believe that it will be very common for a single object to both own the bridge and act as its target.  To avoid accidental retain cycles in this common case, maybe it would be safer to use a WeakNSObject or __unsafe_unretained?

This header file is also included from both ARC and non-ARC contexts, so the ivar should be explicitly labeled as __strong/__weak or scoped_nsobject/WeakNSObject.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 12 2017 

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

commit 3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Apr 12 15:18:53 2017

[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}

[modify] https://crrev.com/3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec/ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm
[modify] https://crrev.com/3b0155e9b6ab3719ce1d1fda5cdb5fb9d0e486ec/ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h

Comment 2 by sdefresne@chromium.org, May 31 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment