Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1157863009:
CRBProtocolObservers can now be mutated while forwarding methods. (Closed)
DescriptionCRBProtocolObservers can now be mutated while forwarding methods.
An observer can now safely call addObserver: or removeObserver:
while being called from an observer method dispatch from
CRBProtocolObservers.
BUG=None
Committed: https://crrev.com/12d50e799365f672b29d6eaf3d273b92bf722e4f
Cr-Commit-Position: refs/heads/master@{#333022}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 20
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Messages
Total messages: 25 (8 generated)
jbbegue@chromium.org changed reviewers: + droger@chromium.org
https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:16: std::vector<__weak id> _observers; Nit: I don't think we use __weak here. I know we used to use it at some point, but I think the current style is to not use it, but rather use a comment: std::vector<id> _observers; // Vector of weak pointers. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; _invocationDepth https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; Maybe add a comment saying it is 0 if nobody is iterating on the observers. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:20: - (void)compact; Add a comment. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:33: CRBProtocolObservers* protocolObservers_ = nullptr; Nit: why the initialization here? https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:41: max_index_(protocolObservers->_observers.size()) { Should we DCHECK(protocolObservers_); ? https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.h (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.h:19: // notification dispatch. Optional, but would be nice: add a code example (see for example TYPICAL USAGE in observer_list). You can also mention your trick to add compile-time type checking if the protocol has only optional methods if you want. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers_unittest.mm (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers_unittest.mm:163: We should also probably test the case where _invocation_depth is > 1.
sdefresne@chromium.org changed reviewers: + sdefresne@chromium.org
https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; _invocationDepth https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:33: CRBProtocolObservers* protocolObservers_ = nullptr; protocol_observers_ https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:58: return index_ < max_index ? observers[index_++] : nullptr; s/nullptr/nil/ https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:108: *it = nullptr; s/nullptr/nil/ https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:152: while ((observer = it.GetNext()) != nullptr) { s/nullptr/ or even remove the test https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:164: while ((observer = it.GetNext()) != nullptr) s/nullptr/ or even remove the test https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:171: _observers.erase(std::remove(_observers.begin(), _observers.end(), nullptr), s/nullptr/nil
https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:16: std::vector<__weak id> _observers; On 2015/06/04 10:04:23, droger wrote: > Nit: I don't think we use __weak here. I know we used to use it at some point, > but I think the current style is to not use it, but rather use a comment: > std::vector<id> _observers; // Vector of weak pointers. If (when) we migrate to ARC, we'll have to pass over all the code and convert the comments to __weak annotation if we continue to do that. I would suggest using both the comment and the annotation to avoid grunt work in the future.
https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:16: std::vector<__weak id> _observers; According to https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transition..., I think it should be __unsafe_unretained: - __strong is the default. An object remains “alive” as long as there is a strong pointer to it. - __weak specifies a reference that does not keep the referenced object alive. A weak reference is set to nil when there are no strong references to the object. - __unsafe_unretained specifies a reference that does not keep the referenced object alive and is not set to nil when there are no strong references to the object. If the object it references is deallocated, the pointer is left dangling.
https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:16: std::vector<__weak id> _observers; On 2015/06/04 10:04:23, droger wrote: > Nit: I don't think we use __weak here. I know we used to use it at some point, > but I think the current style is to not use it, but rather use a comment: > std::vector<id> _observers; // Vector of weak pointers. Wouldn't that facilitate the transition to ARC? https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; On 2015/06/04 10:04:23, droger wrote: > Maybe add a comment saying it is 0 if nobody is iterating on the observers. Done. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; On 2015/06/04 10:04:23, droger wrote: > Maybe add a comment saying it is 0 if nobody is iterating on the observers. Done. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; On 2015/06/04 10:04:23, droger wrote: > _invocationDepth Done. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:20: - (void)compact; On 2015/06/04 10:04:23, droger wrote: > Add a comment. Done. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:33: CRBProtocolObservers* protocolObservers_ = nullptr; On 2015/06/04 10:04:23, droger wrote: > Nit: why the initialization here? Done. https://codereview.chromium.org/1157863009/diff/1/base/ios/crb_protocol_obser... base/ios/crb_protocol_observers.mm:41: max_index_(protocolObservers->_observers.size()) { On 2015/06/04 10:04:23, droger wrote: > Should we > DCHECK(protocolObservers_); > ? Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.h (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.h:19: // notification dispatch. On 2015/06/04 10:04:23, droger wrote: > Optional, but would be nice: > add a code example (see for example TYPICAL USAGE in observer_list). > > You can also mention your trick to add compile-time type checking if the > protocol has only optional methods if you want. Acknowledged. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:16: std::vector<__weak id> _observers; On 2015/06/04 10:45:34, sdefresne wrote: > According to > https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-Transition..., > I think it should be __unsafe_unretained: > > - __strong is the default. An object remains “alive” as long as there is a > strong pointer to it. > - __weak specifies a reference that does not keep the referenced object alive. A > weak reference is set to nil when there are no strong references to the object. > - __unsafe_unretained specifies a reference that does not keep the referenced > object alive and is not set to nil when there are no strong references to the > object. If the object it references is deallocated, the pointer is left > dangling. Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:17: int _invocation_depth; On 2015/06/04 10:18:34, sdefresne wrote: > _invocationDepth Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:33: CRBProtocolObservers* protocolObservers_ = nullptr; On 2015/06/04 10:18:34, sdefresne wrote: > protocol_observers_ Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:58: return index_ < max_index ? observers[index_++] : nullptr; On 2015/06/04 10:18:34, sdefresne wrote: > s/nullptr/nil/ Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:108: *it = nullptr; On 2015/06/04 10:18:34, sdefresne wrote: > s/nullptr/nil/ Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:152: while ((observer = it.GetNext()) != nullptr) { On 2015/06/04 10:18:34, sdefresne wrote: > s/nullptr/ or even remove the test Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:164: while ((observer = it.GetNext()) != nullptr) On 2015/06/04 10:18:34, sdefresne wrote: > s/nullptr/ or even remove the test Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:171: _observers.erase(std::remove(_observers.begin(), _observers.end(), nullptr), On 2015/06/04 10:18:34, sdefresne wrote: > s/nullptr/nil Done. https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers_unittest.mm (right): https://codereview.chromium.org/1157863009/diff/20001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers_unittest.mm:163: On 2015/06/04 10:04:23, droger wrote: > We should also probably test the case where _invocation_depth is > 1. I added a test for the nested case. I also added another test for the case of an observer removing itself while being notified.
lgtm https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:22: int _invocation_depth; _invocationDepth https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:25: // The methods will remove nil observers from the list and will be called when s/will remove/removes/ s/will be/is/
lgtm https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:179: _observers.erase(std::remove(_observers.begin(), _observers.end(), nil), nit: DCHECK(!_invocation_depth);
https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:22: int _invocation_depth; On 2015/06/04 12:29:53, droger wrote: > _invocationDepth Done. https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:25: // The methods will remove nil observers from the list and will be called when On 2015/06/04 12:29:53, droger wrote: > s/will remove/removes/ > s/will be/is/ Done. https://codereview.chromium.org/1157863009/diff/40001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:179: _observers.erase(std::remove(_observers.begin(), _observers.end(), nil), On 2015/06/04 12:32:48, sdefresne wrote: > nit: DCHECK(!_invocation_depth); Done.
The CQ bit was checked by jbbegue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1157863009/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1157863009/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jbbegue@chromium.org changed reviewers: + stuartmorgan@chromium.org
Adding Stuart Morgan for owners. Some context around this change for Stuart: We have found recently some nasty crashes linked to observers adding/removing themselves as observers while being notified, the issue was there since a long time but just recently started to lead to visible crashes. Some ugly fixes have been put in place to temporarily fix that situation see https://chromereviews.googleplex.com/202327013/ but a more robust solution needed to be created. This CL modifies the CRBProtocolObservers class to support adding and removing observers while notifying them. That's what will be used in the TabModel to finally fix the root issue without using duplicated code that performs badly.
LGTM with nits. Thanks for making sure we had a general solution! https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.h (right): https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.h:35: // Returns true if there is currently no observers. s/is/are/ https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:15: // ivar declared here are private to the implementation but must be ivars https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:20: // Indicate the nested level of observer iteration. s/Indicate the/The/ https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:25: // The methods removes nil observers from the list and is called when the s/The method removes/Removes/ https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:62: // Skip null elements. nil https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:122: - (BOOL)empty { It's probably worth calling out in the header that this may return NO during iteration even if if all observers have been removed during the iteration.
https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.h (right): https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.h:35: // Returns true if there is currently no observers. On 2015/06/04 13:40:05, stuartmorgan wrote: > s/is/are/ Done. https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... File base/ios/crb_protocol_observers.mm (right): https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:15: // ivar declared here are private to the implementation but must be On 2015/06/04 13:40:06, stuartmorgan wrote: > ivars Done. https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:20: // Indicate the nested level of observer iteration. On 2015/06/04 13:40:06, stuartmorgan wrote: > s/Indicate the/The/ Done. https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:25: // The methods removes nil observers from the list and is called when the On 2015/06/04 13:40:06, stuartmorgan wrote: > s/The method removes/Removes/ Done. https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:62: // Skip null elements. On 2015/06/04 13:40:06, stuartmorgan wrote: > nil Done. https://codereview.chromium.org/1157863009/diff/60001/base/ios/crb_protocol_o... base/ios/crb_protocol_observers.mm:122: - (BOOL)empty { On 2015/06/04 13:40:06, stuartmorgan wrote: > It's probably worth calling out in the header that this may return NO during > iteration even if if all observers have been removed during the iteration. Good point thanks, I prefer to fix the implementation because explaining that this method can return the wrong value sometimes doesn't feel good.
The CQ bit was checked by jbbegue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, stuartmorgan@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/1157863009/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1157863009/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/12d50e799365f672b29d6eaf3d273b92bf722e4f Cr-Commit-Position: refs/heads/master@{#333022} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
