Ensuring destruction order in MediaStreamImpl unit test.

Ensuring destruction order in MediaStreamImpl unit test.

The crash flakiness seen in the bug may be solved by this, therefor also enabling the test. I haven't been able to reproduce the issue, hence the uncertainty. This change should be done even if it doesn't solve this bug.


BUG=112408
TEST=content_unittests

[tommi (sloooow) - chröme, 2012-02-09 12:20:44]

I'm worried that we're just fixing the test here. The production code might
still be prone to crashing in the event that the destruction order isn't exactly
what you're changing it to now in the test.

I took a quick look at the code and there are some issues I see:

1) RenderViewImpl owns a raw pointer to P2PSocketDispatcher.  The
P2PSocketDispatcher signs up to the render view as an observer and deletes
itself in OnDestruct().  This means that for at least some period of time,
RenderViewImpl has a bogus pointer to P2PSocketDispatcher.
2) When MediaStreamImpl is constructed, RenderViewImpl passes the
P2PSocketDispatcher pointer to MediaStreamImpl.  MediaStreamImpl, like
RenderViewImpl, has no idea when P2PSocketDispatcher is deleted, hence this
crash.

So, rather than avoid this scenario in the unit test, I would rather handle it
in the production code.  Here are a couple of options:

* Make P2PSocketDispatcher reference counted.
  - RenderViewImpl and MediaStreamImpl have a scoped_refptr<> instead of a raw
pointer.
  - When P2PSocketDispatcher registers as an observer, it calls AddRef on
itself.
  - When OnDestroy is called, it calls Release()

or

* MediaStreamImpl also observes RenderViewImpl and invalidates the
P2PSocketDispatcher pointer when OnDestroy is called.
  - We would also have to modify RenderViewImpl to somehow be aware that
P2PSocketDispatcher is also an observer, so when OnDestroy is fired, the pointer
must be set to NULL.

https://chromiumcodereview.appspot.com/9368022/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9368022/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:48: ms_impl = NULL;
Since this doesn't guarantee that the object is deleted, what about adding this
check?

EXPECT_TRUE(ms_impl->HasOneRef());
ms_impl = NULL;

[Henrik Grunell, 2012-02-09 13:02:10]

Indeed will this only solve the unit test case, the real problem must still be
fixed. I didn't write it out explicitly in this issue, but did so in the bug.
That was the intention of this patch and also to enable the test (not clear in
description, sorry). Maybe that's not a good idea if one thinks that all is fine
since the test passes. The unit test could also be affected by the real fix, so
that's another reason for holding the enabling. What do you think?

https://chromiumcodereview.appspot.com/9368022/diff/1/content/renderer/media/...
File content/renderer/media/media_stream_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9368022/diff/1/content/renderer/media/...
content/renderer/media/media_stream_impl_unittest.cc:48: ms_impl = NULL;
On 2012/02/09 12:20:45, tommi wrote:
> Since this doesn't guarantee that the object is deleted, what about adding
this
> check?
> 
> EXPECT_TRUE(ms_impl->HasOneRef());
> ms_impl = NULL;

Sounds good. (Though currently in the unit test there will only be one ref.)

[Henrik Grunell, 2012-02-10 13:12:43]

On 2012/02/09 13:02:10, Henrik Grunell wrote:
> Indeed will this only solve the unit test case, the real problem must still be
> fixed. I didn't write it out explicitly in this issue, but did so in the bug.
> That was the intention of this patch and also to enable the test (not clear in
> description, sorry). Maybe that's not a good idea if one thinks that all is
fine
> since the test passes. The unit test could also be affected by the real fix,
so
> that's another reason for holding the enabling. What do you think?

I'll hold this CL until a proper solution for the real issue is in place.

[Henrik Grunell, 2012-10-03 06:51:21]

This issue has been resolved in another CL(s). Closing.