Fix WeakPtrFactory member ordering in chrome/browser and chrome/service

Fix WeakPtrFactory member ordering in chrome/browser

Changing in the intialization order of WeakPtrFactory such that all
member variables should appear before the WeakPtrFactory to ensure
that any WeakPtrs to Controller are invalidated before its members
variable's destructors are executed, rendering them invalid.

BUG=303818
Committed: https://crrev.com/745953646fc11c8ae13620ec3ee561a0e44a8caf
Cr-Commit-Position: refs/heads/master@{#296216}

[MRV, 2014-09-17 07:38:12]

mohan.reddy@samsung.com changed reviewers:
+ sky@chromium.org

[MRV, 2014-09-17 07:38:12]

PTAL

[marja, 2014-09-17 07:43:00]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2014-09-17 07:43:00]

Drive-by comments:

It would be useful to add comments near the WeakPtrFactory, e.g.,

SomeMemberVar var_;

// This needs to be the last member, because ..
WeakPtrFactory<..> factory_;

Otherwise it's pretty easy to end up in the same situation again if people add
more member variables.

(Hmm, based on your description I'm not sure if I understand the issue... the
WeakPtrFactory needs to be destructed first, because... what happens if the
member variables are destructed first? We're already in the dtor of the class in
question...)

[MRV, 2014-09-17 08:46:42]

On 2014/09/17 07:43:00, marja wrote:
> Drive-by comments:
> 
> It would be useful to add comments near the WeakPtrFactory, e.g.,
> 
> SomeMemberVar var_;
> 
> // This needs to be the last member, because ..
> WeakPtrFactory<..> factory_;
> 
> Otherwise it's pretty easy to end up in the same situation again if people add
> more member variables.
> 
> (Hmm, based on your description I'm not sure if I understand the issue... the
> WeakPtrFactory needs to be destructed first, because... what happens if the
> member variables are destructed first? We're already in the dtor of the class
in
> question...)

Dear Mr. Marja, Thanks for your feedback and review, I used to add description
for WeakPtrFactory, but in one of my review which mentioned below, I got
comment, so I didn't add comment before WeakPtrFactory objects.
https://codereview.chromium.org/567873003/#msg3

Please let me know your opinion, I'll make changes accordingly.

[marja, 2014-09-17 08:51:07]

On 2014/09/17 08:46:42, MRV wrote:
> On 2014/09/17 07:43:00, marja wrote:
> > Drive-by comments:
> > 
> > It would be useful to add comments near the WeakPtrFactory, e.g.,
> > 
> > SomeMemberVar var_;
> > 
> > // This needs to be the last member, because ..
> > WeakPtrFactory<..> factory_;
> > 
> > Otherwise it's pretty easy to end up in the same situation again if people
add
> > more member variables.
> > 
> > (Hmm, based on your description I'm not sure if I understand the issue...
the
> > WeakPtrFactory needs to be destructed first, because... what happens if the
> > member variables are destructed first? We're already in the dtor of the
class
> in
> > question...)
> 
> Dear Mr. Marja, Thanks for your feedback and review, I used to add description
> for WeakPtrFactory, but in one of my review which mentioned below, I got
> comment, so I didn't add comment before WeakPtrFactory objects.
> https://codereview.chromium.org/567873003/#msg3
> 
> Please let me know your opinion, I'll make changes accordingly.

Hmm, I see, I'm not going to disagree with avi@, so you should just leave the
comment out.

Btw, I'm still not sure what exactly the comment means. What's "Controller"?
What's the scenario where stuff goes wrong?

[marja, 2014-09-17 08:51:24]

marja@chromium.org changed reviewers:
- marja@chromium.org

[sky, 2014-09-17 15:47:36]

I commented as much in similar reviews. Really there needs to be a
presubmit check, otherwise this'll regress.

On Wed, Sep 17, 2014 at 12:43 AM,  <marja@chromium.org> wrote:
> Drive-by comments:
>
> It would be useful to add comments near the WeakPtrFactory, e.g.,
>
> SomeMemberVar var_;
>
> // This needs to be the last member, because ..
> WeakPtrFactory<..> factory_;
>
> Otherwise it's pretty easy to end up in the same situation again if people
> add
> more member variables.
>
> (Hmm, based on your description I'm not sure if I understand the issue...
> the
> WeakPtrFactory needs to be destructed first, because... what happens if the
> member variables are destructed first? We're already in the dtor of the
> class in
> question...)
>
> https://codereview.chromium.org/557833006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MRV, 2014-09-17 17:03:27]

On 2014/09/17 15:47:36, sky wrote:
> I commented as much in similar reviews. Really there needs to be a
> presubmit check, otherwise this'll regress.
> 
> On Wed, Sep 17, 2014 at 12:43 AM,  <mailto:marja@chromium.org> wrote:
> > Drive-by comments:
> >
> > It would be useful to add comments near the WeakPtrFactory, e.g.,
> >
> > SomeMemberVar var_;
> >
> > // This needs to be the last member, because ..
> > WeakPtrFactory<..> factory_;
> >
> > Otherwise it's pretty easy to end up in the same situation again if people
> > add
> > more member variables.
> >
> > (Hmm, based on your description I'm not sure if I understand the issue...
> > the
> > WeakPtrFactory needs to be destructed first, because... what happens if the
> > member variables are destructed first? We're already in the dtor of the
> > class in
> > question...)
> >
> > https://codereview.chromium.org/557833006/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Dear mr.sky...
I taken your suggestion.. thinking about way add the same in presubmit... to
avoid future. regressions..

PTAL.

[MRV, 2014-09-18 03:28:46]

On 2014/09/17 08:51:07, marja wrote:
> On 2014/09/17 08:46:42, MRV wrote:
> > On 2014/09/17 07:43:00, marja wrote:
> > > Drive-by comments:
> > > 
> > > It would be useful to add comments near the WeakPtrFactory, e.g.,
> > > 
> > > SomeMemberVar var_;
> > > 
> > > // This needs to be the last member, because ..
> > > WeakPtrFactory<..> factory_;
> > > 
> > > Otherwise it's pretty easy to end up in the same situation again if people
> add
> > > more member variables.
> > > 
> > > (Hmm, based on your description I'm not sure if I understand the issue...
> the
> > > WeakPtrFactory needs to be destructed first, because... what happens if
the
> > > member variables are destructed first? We're already in the dtor of the
> class
> > in
> > > question...)
> > 
> > Dear Mr. Marja, Thanks for your feedback and review, I used to add
description
> > for WeakPtrFactory, but in one of my review which mentioned below, I got
> > comment, so I didn't add comment before WeakPtrFactory objects.
> > https://codereview.chromium.org/567873003/#msg3
> > 
> > Please let me know your opinion, I'll make changes accordingly.
> 
> Hmm, I see, I'm not going to disagree with avi@, so you should just leave the
> comment out.
> 
> Btw, I'm still not sure what exactly the comment means. What's "Controller"?
> What's the scenario where stuff goes wrong?

Dear Mr. Marja, 
Please find my updates below on WeakPtrs:

Usaully WeakPtrFactory is to hand off WeakPtrs and let WeakPtrFactory's dtor
invalidate them.
WeakPtrFactory loses all its usefulness if you don't guarantee that it gets
destructed before its referee (or at least all other members of its referee).
It's definitely possible to do that right.
One use for having a WeakPtrFactory refer to something other than the object
that contains it would be for an object to supply weak pointers to some member
it owns that does not on it's own supply weak pointers to itself. Then, you need
to ensure that the WeakPtrFactory is declared after the member that it will be
creating weak pointers to. This would be difficult to get right if ownership of
that member is changed (e.g. member is created, member is destroyed, ownership
passed to someone else) outside of the normal construction/destruction flow--
i.e. you would then want a scoped pointer to the factory so you could make sure
the factory is in the correct state whenever the ownership changes.

PTAL

[MRV, 2014-09-18 15:51:34]

Dear Mr.Sky and Mr.Marja, please review my patch.

[MRV, 2014-09-18 15:51:34]

Dear Mr.Sky and Mr.Marja, please review my patch.

[marja, 2014-09-18 16:04:42]

(I think sky@ is the right person to review, since I *still* don't get this.)

So it would be a problem if we first destruct the other member variables, and
then *something* tries to access them via the weak pointer which we haven't
nullified yet. But what is that something? Is it something from another thread?
(No, that makes no sense; weak pointers cannot be dereferenced from a thread and
invalidated from another.) Is it a theoretical something or can it happen in
practice? Is it something that gets triggered when the weak pointer is
invalidated? (There's no "object about to be destructed" callback for weak
pointers, they just silently go NULL, right?)

[sky, 2014-09-18 19:31:35]

Marja, search for the thread "WeakPtrFactory member variable order
rule weakly followed". For the majority of the cases where we use
WeakPtrFactory order isn't going to matter, but it could, so better to
be safe.

  -Scott

On Thu, Sep 18, 2014 at 9:04 AM,  <marja@chromium.org> wrote:
> (I think sky@ is the right person to review, since I *still* don't get
> this.)
>
> So it would be a problem if we first destruct the other member variables,
> and
> then *something* tries to access them via the weak pointer which we haven't
> nullified yet. But what is that something? Is it something from another
> thread?
> (No, that makes no sense; weak pointers cannot be dereferenced from a thread
> and
> invalidated from another.) Is it a theoretical something or can it happen in
> practice? Is it something that gets triggered when the weak pointer is
> invalidated? (There's no "object about to be destructed" callback for weak
> pointers, they just silently go NULL, right?)
>
> https://codereview.chromium.org/557833006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[MRV, 2014-09-19 03:08:29]

On 2014/09/18 19:31:35, sky wrote:
> Marja, search for the thread "WeakPtrFactory member variable order
> rule weakly followed". For the majority of the cases where we use
> WeakPtrFactory order isn't going to matter, but it could, so better to
> be safe.
> 
>   -Scott
> 
> On Thu, Sep 18, 2014 at 9:04 AM,  <mailto:marja@chromium.org> wrote:
> > (I think sky@ is the right person to review, since I *still* don't get
> > this.)
> >
> > So it would be a problem if we first destruct the other member variables,
> > and
> > then *something* tries to access them via the weak pointer which we haven't
> > nullified yet. But what is that something? Is it something from another
> > thread?
> > (No, that makes no sense; weak pointers cannot be dereferenced from a thread
> > and
> > invalidated from another.) Is it a theoretical something or can it happen in
> > practice? Is it something that gets triggered when the weak pointer is
> > invalidated? (There's no "object about to be destructed" callback for weak
> > pointers, they just silently go NULL, right?)
> >
> > https://codereview.chromium.org/557833006/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

@Mr. marja, as Mr.Scott stated, order is not going to be problem most of the
times, but sometimes we'll have some rare random one-time crashes, heap
corruption etc, so it will be better to have WeakPtr initilization at end.
It topology described for order of initilization with respect to documentation
of weakptr.

 - Mohan.

[MRV, 2014-09-23 01:59:42]

On 2014/09/19 03:08:29, MRV wrote:
> On 2014/09/18 19:31:35, sky wrote:
> > Marja, search for the thread "WeakPtrFactory member variable order
> > rule weakly followed". For the majority of the cases where we use
> > WeakPtrFactory order isn't going to matter, but it could, so better to
> > be safe.
> > 
> >   -Scott
> > 
> > On Thu, Sep 18, 2014 at 9:04 AM,  <mailto:marja@chromium.org> wrote:
> > > (I think sky@ is the right person to review, since I *still* don't get
> > > this.)
> > >
> > > So it would be a problem if we first destruct the other member variables,
> > > and
> > > then *something* tries to access them via the weak pointer which we
haven't
> > > nullified yet. But what is that something? Is it something from another
> > > thread?
> > > (No, that makes no sense; weak pointers cannot be dereferenced from a
thread
> > > and
> > > invalidated from another.) Is it a theoretical something or can it happen
in
> > > practice? Is it something that gets triggered when the weak pointer is
> > > invalidated? (There's no "object about to be destructed" callback for weak
> > > pointers, they just silently go NULL, right?)
> > >
> > > https://codereview.chromium.org/557833006/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> @Mr. marja, as Mr.Scott stated, order is not going to be problem most of the
> times, but sometimes we'll have some rare random one-time crashes, heap
> corruption etc, so it will be better to have WeakPtr initilization at end.
> It topology described for order of initilization with respect to documentation
> of weakptr.
> 
>  - Mohan.

Dear mr.sky and mr.marja,
Please review my patch.
Please let me know if you need any more information on the same.

Thanks and regards,
MRV.

[marja, 2014-09-23 07:37:26]

marja@chromium.org changed reviewers:
+ marja@chromium.org

[marja, 2014-09-23 07:37:27]

(Sorry for responding so slowly, I'm somewhat confused why sky@ didn't yet
l-g-t-m this, as in, is there something pending before this should be landed?
Like "doesn't make sense to fix this unless there's a presubmit check to prevent
regressions".)

As I understand the problem: if the other data members are destroyed before the
WeakPtrFactory, the dtors of the other data members or some functions called via
them might access the object-currently-under-destruction via weak ptrs which are
held somewhere, and this is the code path for the bad access.

Anyhow, LGTM.

[MRV, 2014-09-23 08:44:09]

On 2014/09/23 07:37:27, marja wrote:
> (Sorry for responding so slowly, I'm somewhat confused why sky@ didn't yet
> l-g-t-m this, as in, is there something pending before this should be landed?
> Like "doesn't make sense to fix this unless there's a presubmit check to
prevent
> regressions".)
> 
> As I understand the problem: if the other data members are destroyed before
the
> WeakPtrFactory, the dtors of the other data members or some functions called
via
> them might access the object-currently-under-destruction via weak ptrs which
are
> held somewhere, and this is the code path for the bad access.
> 
> Anyhow, LGTM.

Dear Mr. Marja, thanks for LGTM.
I am trying different ways to under track the weakptr issues to not regress
(either using in presubmit or clang plugin flags)

Currently I am bit struk at files where more than one "WeakPtrFactory" used in
the same file like below:
content/renderer/pepper/pepper_plugin_instance_impl.h
ui/aura/window_event_dispatcher.h

If you find any clue please let me know.

[MRV, 2014-09-23 08:44:45]

The CQ bit was checked by mohan.reddy@samsung.com

[commit-bot: I haz the power, 2014-09-23 08:45:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/557833006/40001

[MRV, 2014-09-23 08:48:19]

Dear Mr. Sky,
Could you please looks Review, looks like Mr. marja are author of 
 chrome/browser/sessions
For the rest of the files need to have review.
PTAL.

[commit-bot: I haz the power, 2014-09-23 08:56:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 08:56:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2014-09-23 13:21:24]

LGTM

[MRV, 2014-09-23 13:38:13]

The CQ bit was checked by mohan.reddy@samsung.com

[MRV, 2014-09-23 13:38:28]

On 2014/09/23 13:21:24, sky wrote:
> LGTM

Thanks Mr. Sky for LGTM

[commit-bot: I haz the power, 2014-09-23 13:38:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/557833006/40001

[commit-bot: I haz the power, 2014-09-23 14:29:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 14:29:18]

Failed to apply the patch.

[marja, 2014-09-23 14:55:21]

On 2014/09/23 08:44:09, MRV wrote:
> On 2014/09/23 07:37:27, marja wrote:
> > (Sorry for responding so slowly, I'm somewhat confused why sky@ didn't yet
> > l-g-t-m this, as in, is there something pending before this should be
landed?
> > Like "doesn't make sense to fix this unless there's a presubmit check to
> prevent
> > regressions".)
> > 
> > As I understand the problem: if the other data members are destroyed before
> the
> > WeakPtrFactory, the dtors of the other data members or some functions called
> via
> > them might access the object-currently-under-destruction via weak ptrs which
> are
> > held somewhere, and this is the code path for the bad access.
> > 
> > Anyhow, LGTM.
> 
> Dear Mr. Marja, thanks for LGTM.
> I am trying different ways to under track the weakptr issues to not regress
> (either using in presubmit or clang plugin flags)
> 
> Currently I am bit struk at files where more than one "WeakPtrFactory" used in
> the same file like below:
> content/renderer/pepper/pepper_plugin_instance_impl.h
> ui/aura/window_event_dispatcher.h
> 
> If you find any clue please let me know.

I don't think it matters which one you delete first - why would it matter?
Deletion is just going to nullify a pointer, and will not cause any cascading
actions, especially, it won't cause anybody to access the
class-to-be-destructed.

[MRV, 2014-09-23 16:57:12]

The CQ bit was checked by mohan.reddy@samsung.com

[commit-bot: I haz the power, 2014-09-23 16:57:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/557833006/40001

[commit-bot: I haz the power, 2014-09-23 20:35:44]

Committed patchset #3 (id:40001) as c0bed6293c4b4465d8bc689af7df3bed9ccd9b0b

[commit-bot: I haz the power, 2014-09-23 20:36:19]

Patchset 3 (id:??) landed as
https://crrev.com/745953646fc11c8ae13620ec3ee561a0e44a8caf
Cr-Commit-Position: refs/heads/master@{#296216}