Make sure that when a SupportsUserData is destroyed, objects destroyed transitively see it as empty.

Make sure that when a SupportsUserData is destroyed, objects destroyed transitively see it as empty.

BUG=358707
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273568

[Jeffrey Yasskin, 2014-05-28 01:55:18]

I ran into this today, so I figured I'd fix it. Note that this attacks the
recursive-access-during-destruction problem, not the
recursive-access-through-RemoveUserData mentioned in the bug. It'll be
straightforward to extend the change and test to handle RemoveUserData, but I
think that should be a separate change.

[Peter Kasting, 2014-05-28 03:44:12]

LGTM

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data.cc
File base/supports_user_data.cc (right):

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data.cc#n...
base/supports_user_data.cc:42: // calling into a being-destroyed object.
Nit: calling into -> examining

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
File base/supports_user_data_unittest.cc (right):

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
base/supports_user_data_unittest.cc:19: :
supports_user_data_(supports_user_data), key_(key) {}
Nit: One initializer per line if they don't all fit on the same line as the
function name

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
base/supports_user_data_unittest.cc:37: }  // namespace base
Nit: Either add a blank line here or take it out above

(Though probably I'd just remove the anonymous namespace, it doesn't seem
important for a test)

[Jeffrey Yasskin, 2014-05-28 05:45:26]

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data.cc
File base/supports_user_data.cc (right):

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data.cc#n...
base/supports_user_data.cc:42: // calling into a being-destroyed object.
On 2014/05/28 03:44:13, Peter Kasting wrote:
> Nit: calling into -> examining

Done.

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
File base/supports_user_data_unittest.cc (right):

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
base/supports_user_data_unittest.cc:19: :
supports_user_data_(supports_user_data), key_(key) {}
On 2014/05/28 03:44:13, Peter Kasting wrote:
> Nit: One initializer per line if they don't all fit on the same line as the
> function name

Done, and filed b/15292387.

https://codereview.chromium.org/298023008/diff/1/base/supports_user_data_unit...
base/supports_user_data_unittest.cc:37: }  // namespace base
On 2014/05/28 03:44:13, Peter Kasting wrote:
> Nit: Either add a blank line here or take it out above

Thanks; I've taken it out above.

> (Though probably I'd just remove the anonymous namespace, it doesn't seem
> important for a test)

It's especially important in a test, where we'd like to use short names for
helper classes, but we really don't want multiple tests to use the same name as
each other, lest the ODR bite us.

[Peter Kasting, 2014-05-28 18:41:49]

On 2014/05/28 05:45:26, Jeffrey Yasskin wrote:
> > (Though probably I'd just remove the anonymous namespace, it doesn't seem
> > important for a test)
> 
> It's especially important in a test, where we'd like to use short names for
> helper classes, but we really don't want multiple tests to use the same name
as
> each other, lest the ODR bite us.

Don't functions/classes declared in a C++ file have internal linkage by default?
 Or am I wildly misremembering?

[Jeffrey Yasskin, 2014-05-28 22:22:47]

On 2014/05/28 18:41:49, Peter Kasting wrote:
> On 2014/05/28 05:45:26, Jeffrey Yasskin wrote:
> > > (Though probably I'd just remove the anonymous namespace, it doesn't seem
> > > important for a test)
> > 
> > It's especially important in a test, where we'd like to use short names for
> > helper classes, but we really don't want multiple tests to use the same name
> as
> > each other, lest the ODR bite us.
> 
> Don't functions/classes declared in a C++ file have internal linkage by
default?
>  Or am I wildly misremembering?

I think you're misremembering:
https://gist.github.com/jyasskin/b6675d537dedb5d49c70

[Peter Kasting, 2014-05-28 22:23:57]

On 2014/05/28 22:22:47, Jeffrey Yasskin wrote:
> On 2014/05/28 18:41:49, Peter Kasting wrote:
> > On 2014/05/28 05:45:26, Jeffrey Yasskin wrote:
> > > > (Though probably I'd just remove the anonymous namespace, it doesn't
seem
> > > > important for a test)
> > > 
> > > It's especially important in a test, where we'd like to use short names
for
> > > helper classes, but we really don't want multiple tests to use the same
name
> > as
> > > each other, lest the ODR bite us.
> > 
> > Don't functions/classes declared in a C++ file have internal linkage by
> default?
> >  Or am I wildly misremembering?
> 
> I think you're misremembering:
> https://gist.github.com/jyasskin/b6675d537dedb5d49c70

Maybe it's just global-scope functions then... clearly, I'm too lazy to look up
the right answer.

The namespace is A-OK by me.

[Jeffrey Yasskin, 2014-05-28 23:30:43]

+Nico who's been more involved in the bug. I think all of the relevant base/
owners are in Paris working on C++11, and sorry for interrupting you, but I'd
appreciate a review before I go on 3 weeks of vacation/C++ Thursday evening.

Thanks,
Jeffrey

[awong, 2014-05-29 04:57:57]

LGTM

(Nico's here in Paris too)

[Jeffrey Yasskin, 2014-05-29 14:57:45]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-05-29 15:00:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/298023008/20001

[commit-bot: I haz the power, 2014-05-29 16:36:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-05-29 20:10:38]

Change committed as 273568