Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001813002:
Invalidate ChromotingJniInstance's WeakPtr on network thread (Closed)
DescriptionInvalidate ChromotingJniInstance's WeakPtr on network thread
ChromotingJniInstance implements refcounted but also has a weak pointer
factory. The implementation allows the d'tor to be called on any thread
but the weak pointer can only be invalidated on the network thread. In
some rare circumstances, The instance is destructed on a non-network
thread and fails weakptr's DCHECK. This CL makes the instance invalidate
the weak pointer when it disconnects on the network thread.
BUG=611893
Committed: https://crrev.com/3f62203016905be841dfa1142238f338916802c8
Cr-Commit-Position: refs/heads/master@{#395347}
Patch Set 1 #
Messages
Total messages: 19 (7 generated)
yuweih@chromium.org changed reviewers: + lambroslambrou@chromium.org
While I couldn't reproduce the weird logging sequence per comment#6, an automated test does show the possibility of invalidating weakptr on a wrong thread. Please refer to crbug for more information. This CL makes the instance invalidate the weakptr in Disconnect. PTAL
Can you update the description to point to the correct bug? Otherwise lgtm.
Description was changed from ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=548929 ========== to ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=611893 ==========
On 2016/05/20 21:38:30, Lambros wrote: > Can you update the description to point to the correct bug? > Otherwise lgtm. Oops... Not sure why I copied the wrong bug number...
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001813002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001813002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001813002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001813002/1
Message was sent while issue was closed.
Description was changed from ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=611893 ========== to ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=611893 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=611893 ========== to ========== Invalidate ChromotingJniInstance's WeakPtr on network thread ChromotingJniInstance implements refcounted but also has a weak pointer factory. The implementation allows the d'tor to be called on any thread but the weak pointer can only be invalidated on the network thread. In some rare circumstances, The instance is destructed on a non-network thread and fails weakptr's DCHECK. This CL makes the instance invalidate the weak pointer when it disconnects on the network thread. BUG=611893 Committed: https://crrev.com/3f62203016905be841dfa1142238f338916802c8 Cr-Commit-Position: refs/heads/master@{#395347} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3f62203016905be841dfa1142238f338916802c8 Cr-Commit-Position: refs/heads/master@{#395347}
Message was sent while issue was closed.
You can specify custom traits for ref-counted objects, in particular a custom function to call to actually tear-down the object - you could use that to DeleteSoon the ChromotingJniInstance from the correct thread, thereby ensuring that _all_ members can rely on being torn-down on the UI thread, rather than special-casing the WeakPtrFactory.
Message was sent while issue was closed.
On 2016/05/23 20:50:39, Wez wrote: > You can specify custom traits for ref-counted objects, in particular a custom > function to call to actually tear-down the object - you could use that to > DeleteSoon the ChromotingJniInstance from the correct thread, thereby ensuring > that _all_ members can rely on being torn-down on the UI thread, rather than > special-casing the WeakPtrFactory. This class has been problematic. It is run on multiple threads and the weak pointer is only being used on the network thread. We are planning to break down this class by threads and delete each of them on the right thread rather than having a ref-counted object with obscure deletion behavior.
Message was sent while issue was closed.
Perfect! On 23 May 2016 20:04, <yuweih@chromium.org> wrote: > On 2016/05/23 20:50:39, Wez wrote: > > You can specify custom traits for ref-counted objects, in particular a > custom > > function to call to actually tear-down the object - you could use that to > > DeleteSoon the ChromotingJniInstance from the correct thread, thereby > ensuring > > that _all_ members can rely on being torn-down on the UI thread, rather > than > > special-casing the WeakPtrFactory. > > This class has been problematic. It is run on multiple threads and the weak > pointer is only being used on the network thread. We are planning to break > down > this class by threads and delete each of them on the right thread rather > than > having a ref-counted object with obscure deletion behavior. > > https://codereview.chromium.org/2001813002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Perfect! On 23 May 2016 at 20:04, <yuweih@chromium.org> wrote: > On 2016/05/23 20:50:39, Wez wrote: > > You can specify custom traits for ref-counted objects, in particular a > custom > > function to call to actually tear-down the object - you could use that to > > DeleteSoon the ChromotingJniInstance from the correct thread, thereby > ensuring > > that _all_ members can rely on being torn-down on the UI thread, rather > than > > special-casing the WeakPtrFactory. > > This class has been problematic. It is run on multiple threads and the weak > pointer is only being used on the network thread. We are planning to break > down > this class by threads and delete each of them on the right thread rather > than > having a ref-counted object with obscure deletion behavior. > > https://codereview.chromium.org/2001813002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
