Maintaining the proper order of initialization WeakPtrFactory in "src/remoting"

Maintaining the proper order of initialization WeakPtrFactory in "src/remoting"

Changing in the intialization order of WeakPtrFactory in "src/remoting" module
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/933aeaf09f92c177ee5f172724f8c1955467ef0e
Cr-Commit-Position: refs/heads/master@{#295868}

[ARUNKK, 2014-09-12 13:32:16]

kulkarni.a@samsung.com changed reviewers:
+ sergeyu@chromium.org, wez@chromium.org

[ARUNKK, 2014-09-12 13:32:17]

sergeyu@chromium.org: Please review changes in 

wez@chromium.org: Please review changes in 

PTAL.

[Wez, 2014-09-12 16:39:58]

Please don't add multiple reviewers on a review unless there is a specific
reason to do so.

Sergey, I'll take this one. :)
On 12 Sep 2014 06:32, <kulkarni.a@samsung.com> wrote:

> Reviewers: Sergey Ulanov, Wez,
>
> Message:
> sergeyu@chromium.org: Please review changes in
>
> wez@chromium.org: Please review changes in
>
> PTAL.
>
> Description:
> Maintaing the proper order of initialization WeakPtrFactory in
> "src/remoting"
>
> Changing in the intialization order of WeakPtrFactory in "src/remoting"
> module
> 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
>
> Please review this at https://codereview.chromium.org/569593002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+15, -14 lines):
>   M remoting/client/jni/chromoting_jni_instance.h
>   M remoting/host/client_session.h
>   M remoting/host/client_session.cc
>   M remoting/host/desktop_session_agent.h
>   M remoting/host/desktop_session_agent.cc
>   M remoting/host/win/host_service.h
>
>
> Index: remoting/client/jni/chromoting_jni_instance.h
> diff --git a/remoting/client/jni/chromoting_jni_instance.h
> b/remoting/client/jni/chromoting_jni_instance.h
> index ce572fc0973772083ba53a8a034c25e05f5a54c9..
> 40c2e248c30d12c67a2e4fae008ef4fb759f8215 100644
> --- a/remoting/client/jni/chromoting_jni_instance.h
> +++ b/remoting/client/jni/chromoting_jni_instance.h
> @@ -153,7 +153,6 @@ class ChromotingJniInstance
>    // This group of variables is to be used on the display thread.
>    scoped_refptr<FrameConsumerProxy> frame_consumer_;
>    scoped_ptr<JniFrameConsumer> view_;
> -  scoped_ptr<base::WeakPtrFactory<JniFrameConsumer> > view_weak_factory_;
>
>    // This group of variables is to be used on the network thread.
>    scoped_ptr<ClientContext> client_context_;
> @@ -190,6 +189,7 @@ class ChromotingJniInstance
>
>    friend class base::RefCountedThreadSafe<ChromotingJniInstance>;
>
> +  scoped_ptr<base::WeakPtrFactory<JniFrameConsumer> > view_weak_factory_;
>    base::WeakPtrFactory<ChromotingJniInstance> weak_factory_;
>
>    DISALLOW_COPY_AND_ASSIGN(ChromotingJniInstance);
> Index: remoting/host/client_session.cc
> diff --git a/remoting/host/client_session.cc b/remoting/host/client_
> session.cc
> index e594b104cf63674e83ca7179ae35c5859ce751cd..
> 527e24cb97b9ac334fec6a3d1dbee5bf3e9bbde6 100644
> --- a/remoting/host/client_session.cc
> +++ b/remoting/host/client_session.cc
> @@ -60,7 +60,6 @@ ClientSession::ClientSession(
>        disable_clipboard_filter_(clipboard_echo_filter_.host_filter()),
>        auth_input_filter_(&disable_input_filter_),
>        auth_clipboard_filter_(&disable_clipboard_filter_),
> -      client_clipboard_factory_(clipboard_echo_filter_.client_filter()),
>        max_duration_(max_duration),
>        audio_task_runner_(audio_task_runner),
>        input_task_runner_(input_task_runner),
> @@ -71,7 +70,8 @@ ClientSession::ClientSession(
>        pairing_registry_(pairing_registry),
>        pause_video_(false),
>        lossless_video_encode_(false),
> -      lossless_video_color_(false) {
> +      lossless_video_color_(false),
> +      client_clipboard_factory_(clipboard_echo_filter_.client_filter()) {
>    connection_->SetEventHandler(this);
>
>    // TODO(sergeyu): Currently ConnectionToClient expects stubs to be
> Index: remoting/host/client_session.h
> diff --git a/remoting/host/client_session.h b/remoting/host/client_
> session.h
> index f9fdd3bd5c25de4e63bc9839a8b1d2cfe2ba76cf..
> 90de054e168de7e862c81087c748d01a341a4f69 100644
> --- a/remoting/host/client_session.h
> +++ b/remoting/host/client_session.h
> @@ -208,11 +208,6 @@ class ClientSession
>    protocol::InputFilter auth_input_filter_;
>    protocol::ClipboardFilter auth_clipboard_filter_;
>
> -  // Factory for weak pointers to the client clipboard stub.
> -  // This must appear after |clipboard_echo_filter_|, so that it won't
> outlive
> -  // it.
> -  base::WeakPtrFactory<protocol::ClipboardStub>
> client_clipboard_factory_;
> -
>    // The maximum duration of this session.
>    // There is no maximum if this value is <= 0.
>    base::TimeDelta max_duration_;
> @@ -263,6 +258,11 @@ class ClientSession
>    bool lossless_video_encode_;
>    bool lossless_video_color_;
>
> +  // Factory for weak pointers to the client clipboard stub.
> +  // This must appear after |clipboard_echo_filter_|, so that it won't
> outlive
> +  // it.
> +  base::WeakPtrFactory<protocol::ClipboardStub>
> client_clipboard_factory_;
> +
>    DISALLOW_COPY_AND_ASSIGN(ClientSession);
>  };
>
> Index: remoting/host/desktop_session_agent.cc
> diff --git a/remoting/host/desktop_session_agent.cc
> b/remoting/host/desktop_session_agent.cc
> index 4a1f259010f70e0a551acac0f1aa59c29a640a76..
> ad8994dd47c87cb40eee07a0179e612d1bdef4c1 100644
> --- a/remoting/host/desktop_session_agent.cc
> +++ b/remoting/host/desktop_session_agent.cc
> @@ -120,10 +120,10 @@ DesktopSessionAgent::DesktopSessionAgent(
>        input_task_runner_(input_task_runner),
>        io_task_runner_(io_task_runner),
>        video_capture_task_runner_(video_capture_task_runner),
> -      control_factory_(this),
>        next_shared_buffer_id_(1),
>        shared_buffers_(0),
> -      started_(false) {
> +      started_(false),
> +      control_factory_(this) {
>    DCHECK(caller_task_runner_->BelongsToCurrentThread());
>  }
>
> Index: remoting/host/desktop_session_agent.h
> diff --git a/remoting/host/desktop_session_agent.h
> b/remoting/host/desktop_session_agent.h
> index b971b6fff73761a022ad8e8d606f212f2b383535..
> 802c399e440dc4a5105e51af23ce4c290879cfed 100644
> --- a/remoting/host/desktop_session_agent.h
> +++ b/remoting/host/desktop_session_agent.h
> @@ -179,9 +179,6 @@ class DesktopSessionAgent
>
>    std::string client_jid_;
>
> -  // Used to disable callbacks to |this|.
> -  base::WeakPtrFactory<ClientSessionControl> control_factory_;
> -
>    base::WeakPtr<Delegate> delegate_;
>
>    // The DesktopEnvironment instance used by this agent.
> @@ -228,6 +225,9 @@ class DesktopSessionAgent
>    // before it's received.
>    scoped_ptr<webrtc::DesktopFrame> last_frame_;
>
> +  // Used to disable callbacks to |this|.
> +  base::WeakPtrFactory<ClientSessionControl> control_factory_;
> +
>    DISALLOW_COPY_AND_ASSIGN(DesktopSessionAgent);
>  };
>
> Index: remoting/host/win/host_service.h
> diff --git a/remoting/host/win/host_service.h b/remoting/host/win/host_
> service.h
> index 855f37c14e79013ac50f642d0c2b803c2d179c35..
> ccd92f7bbebe27a9a287a59912445df02eb62e4e 100644
> --- a/remoting/host/win/host_service.h
> +++ b/remoting/host/win/host_service.h
> @@ -116,9 +116,10 @@ class HostService : public WtsTerminalMonitor {
>    // A waitable event that is used to wait until the service is stopped.
>    base::WaitableEvent stopped_event_;
>
> +  base::WeakPtr<HostService> weak_ptr_;
> +
>    // Used to post session change notifications and control events.
>    base::WeakPtrFactory<HostService> weak_factory_;
> -  base::WeakPtr<HostService> weak_ptr_;
>
>    // Singleton.
>    friend struct DefaultSingletonTraits<HostService>;
>
>
>

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

[Wez, 2014-09-12 18:21:51]

wez@chromium.org changed reviewers:
- sergeyu@chromium.org

[Wez, 2014-09-12 18:21:52]

https://codereview.chromium.org/569593002/diff/1/remoting/client/jni/chromoti...
File remoting/client/jni/chromoting_jni_instance.h (left):

https://codereview.chromium.org/569593002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_instance.h:156:
scoped_ptr<base::WeakPtrFactory<JniFrameConsumer> > view_weak_factory_;
This needs to stay where it is; it's not a factory for |this|, but for |view_|.

https://codereview.chromium.org/569593002/diff/1/remoting/host/client_session.h
File remoting/host/client_session.h (left):

https://codereview.chromium.org/569593002/diff/1/remoting/host/client_session...
remoting/host/client_session.h:214:
base::WeakPtrFactory<protocol::ClipboardStub> client_clipboard_factory_;
This doesn't need to move, since it's not a WeakPtrFactory to |this|.

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
File remoting/host/desktop_session_agent.h (right):

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
remoting/host/desktop_session_agent.h:229:
base::WeakPtrFactory<ClientSessionControl> control_factory_;
This should be WeakPtrFactory<DesktopSessionAgent> and called |weak_factory_|,
to be consistent with other classes using this pattern.

[ARUNKK, 2014-09-13 10:10:01]

kulkarni.a@samsung.com changed reviewers:
+ dmichael@chromium.com

[ARUNKK, 2014-09-13 10:11:41]

kulkarni.a@samsung.com changed reviewers:
- dmichael@chromium.com

[ARUNKK, 2014-09-17 04:52:16]

PTAL.

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
File remoting/host/desktop_session_agent.h (right):

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
remoting/host/desktop_session_agent.h:229:
base::WeakPtrFactory<ClientSessionControl> control_factory_;
On 2014/09/12 18:21:52, Wez wrote:
> This should be WeakPtrFactory<DesktopSessionAgent> and called |weak_factory_|,
> to be consistent with other classes using this pattern.
changed 
1. base::WeakPtrFactory<ClientSessionControl> ==>
base::WeakPtrFactory<DesktopSessionAgent>

But the other change renaming the variable from control_factory_ ==>
weak_factory_ will required more code changes , like all the places where the
"control_factory_" used .
I'll take-up the renaming member variable in other patch.

[Wez, 2014-09-18 00:26:13]

nit: Please correct the spelling in the CL description - thanks!

[Wez, 2014-09-18 00:27:56]

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
File remoting/host/desktop_session_agent.h (right):

https://codereview.chromium.org/569593002/diff/1/remoting/host/desktop_sessio...
remoting/host/desktop_session_agent.h:229:
base::WeakPtrFactory<ClientSessionControl> control_factory_;
On 2014/09/17 04:52:16, kulkarni.a wrote:
> On 2014/09/12 18:21:52, Wez wrote:
> > This should be WeakPtrFactory<DesktopSessionAgent> and called
|weak_factory_|,
> > to be consistent with other classes using this pattern.
> changed 
> 1. base::WeakPtrFactory<ClientSessionControl> ==>
> base::WeakPtrFactory<DesktopSessionAgent>
> 
> But the other change renaming the variable from control_factory_ ==>
> weak_factory_ will required more code changes , like all the places where the
> "control_factory_" used .
> I'll take-up the renaming member variable in other patch.

Those changes will be well-contained in desktop_session_agent.cc, though; should
be a trivial search and replace, surely?

[ARUNKK, 2014-09-18 04:10:00]

Hi Wez,
Thanks for review.
Renaming is done.
PTAL.

[Wez, 2014-09-18 17:53:57]

https://codereview.chromium.org/569593002/diff/60001/remoting/host/client_ses...
File remoting/host/client_session.h (left):

https://codereview.chromium.org/569593002/diff/60001/remoting/host/client_ses...
remoting/host/client_session.h:214:
base::WeakPtrFactory<protocol::ClipboardStub> client_clipboard_factory_;
This can stay where it is, since it doesn't refer to |this|.

[ARUNKK, 2014-09-19 07:04:39]

PTAL.

[Wez, 2014-09-19 18:23:03]

lgtm

https://codereview.chromium.org/569593002/diff/80001/remoting/host/win/host_s...
File remoting/host/win/host_service.h (right):

https://codereview.chromium.org/569593002/diff/80001/remoting/host/win/host_s...
remoting/host/win/host_service.h:119: base::WeakPtr<HostService> weak_ptr_;
nit: The WeakPtrFactory comment below still applies to this, so consider putting
this between the comment and the weak_factory_ declaration.

[ARUNKK, 2014-09-20 02:39:42]

The CQ bit was checked by kulkarni.a@samsung.com

[ARUNKK, 2014-09-20 02:40:26]

The CQ bit was unchecked by kulkarni.a@samsung.com

[commit-bot: I haz the power, 2014-09-20 02:40:32]

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

[commit-bot: I haz the power, 2014-09-20 02:40:34]

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

[ARUNKK, 2014-09-20 05:27:15]

The CQ bit was checked by kulkarni.a@samsung.com

[commit-bot: I haz the power, 2014-09-20 05:28:34]

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

[ARUNKK, 2014-09-20 08:59:08]

The CQ bit was unchecked by kulkarni.a@samsung.com

[ARUNKK, 2014-09-20 08:59:22]

The CQ bit was checked by kulkarni.a@samsung.com

[commit-bot: I haz the power, 2014-09-20 08:59:43]

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

[commit-bot: I haz the power, 2014-09-20 13:19:24]

Committed patchset #5 (id:80001) as a6b4615ab8482afaa9feee955dd0045af00ad22e

[commit-bot: I haz the power, 2014-09-20 13:20:21]

Patchset 5 (id:??) landed as
https://crrev.com/933aeaf09f92c177ee5f172724f8c1955467ef0e
Cr-Commit-Position: refs/heads/master@{#295868}