WeakPtr destruction order cleanup: media edition.

WeakPtr destruction order cleanup:  media edition.

Per WeakPtrFactory documentation it must be the first member variable
destructed to avoid invalid member variable access during destruction.

Additional cleanups:
- Removes "weak_this_" usage when there is no manual factory invalidation.
- Adds DISALLOW_COPY_AND_ASSIGN to SendProcess.
- Removes unused WeakPtr from FFmpegVideoDecoder, VpxVideoDecoder, FFMpegAudioDecoder, OpusAudioDecoder

BUG=351048
TEST=compiles, passes CQ.
R=fischman@chromium.org

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

[Ami GONE FROM CHROMIUM, 2014-03-10 22:02:07]

most/all of the "ditto" comments might be trivially resolvable by dropping the
WeakPtr member in favor of the WeakPtrFactory member (and just creating a new
WeakPtr on demand when one is needed).  I suspect re-using the WeakPtr isn't a
perf boost.
(and I fear I may have been the one to introduce this pattern, for no good
reason)

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.cc:70: main_weak_this_ =
main_weak_factory_.GetWeakPtr();
(if you take my suggestion in the .h then you can move this back to an
initializer)

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.h:240:
base::WeakPtr<MediaSourceDelegate> main_weak_this_;
This reversal (putting the factory _after_ a ptr vended from it) looks weird to
me.  Flip them back?

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/video...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/video...
content/renderer/media/video_capture_impl_manager.h:106: 
This empty line distances the factory below from the comment 2 lines up which
applies to it.  Remove blank line or rewrite the comment above to be clearer?

https://codereview.chromium.org/193303002/diff/1/media/base/text_renderer.h
File media/base/text_renderer.h (right):

https://codereview.chromium.org/193303002/diff/1/media/base/text_renderer.h#n...
media/base/text_renderer.h:143: base::WeakPtr<TextRenderer> weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.h:142: // variables might be accessed.
Call me crazy but this comment is starting to seem repetitive :)
I'd much rather, for example, know which thread this factory was bound to...

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_sender/audi...
File media/cast/audio_sender/audio_encoder.cc (left):

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_sender/audi...
media/cast/audio_sender/audio_encoder.cc:176: private:
PRIVATE!!!

https://codereview.chromium.org/193303002/diff/1/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/193303002/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:318: DISALLOW_COPY_AND_ASSIGN(SendProcess);
Call out in CL description.

https://codereview.chromium.org/193303002/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.h:283: base::WeakPtr<AudioRendererImpl>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/decrypting_dem...
File media/filters/decrypting_demuxer_stream.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/decrypting_dem...
media/filters/decrypting_demuxer_stream.h:131:
base::WeakPtr<DecryptingDemuxerStream> weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/decrypting_vid...
File media/filters/decrypting_video_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/decrypting_vid...
media/filters/decrypting_video_decoder.h:112:
base::WeakPtr<DecryptingVideoDecoder> weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.h:106: base::WeakPtr<FFmpegAudioDecoder>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:267: base::WeakPtr<FFmpegDemuxer> weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:91: base::WeakPtr<FFmpegVideoDecoder>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.h:189: base::WeakPtr<GpuVideoDecoder>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/opus_audio_dec...
File media/filters/opus_audio_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/opus_audio_dec...
media/filters/opus_audio_decoder.h:99: base::WeakPtr<OpusAudioDecoder>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/video_renderer...
File media/filters/video_renderer_impl.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/video_renderer...
media/filters/video_renderer_impl.h:243: base::WeakPtr<VideoRendererImpl>
weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/filters/vpx_video_deco...
File media/filters/vpx_video_decoder.h (right):

https://codereview.chromium.org/193303002/diff/1/media/filters/vpx_video_deco...
media/filters/vpx_video_decoder.h:90: base::WeakPtr<VpxVideoDecoder> weak_this_;
ditto

https://codereview.chromium.org/193303002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/193303002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_mac.mm:323:
weak_factory_.AsWeakPtr(),
s/As/Get/??

[DaleCurtis, 2014-03-10 22:10:00]

My worry about changing all the weak_this_ into GetWeakPtr() isn't performance,
but rather that weak_this_ allows only a single invalidation, where as
GetWeakPtr() will happily vend more WeakPtr's after an invalidation.

If a class doesn't have a manual invalidation stage, this is moot though, so
I'll see if any of those can be cleaned up.

WDYT?

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_sender/audi...
File media/cast/audio_sender/audio_encoder.cc (left):

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_sender/audi...
media/cast/audio_sender/audio_encoder.cc:176: private:
On 2014/03/10 22:02:08, Ami Fischman wrote:
> PRIVATE!!!

Look up; it was extra private. :)

[Ami GONE FROM CHROMIUM, 2014-03-10 22:15:16]

On Mon, Mar 10, 2014 at 3:10 PM, <dalecurtis@chromium.org> wrote:

> My worry about changing all the weak_this_ into GetWeakPtr() isn't
> performance,
> but rather that weak_this_ allows only a single invalidation, where as
> GetWeakPtr() will happily vend more WeakPtr's after an invalidation.
> If a class doesn't have a manual invalidation stage, this is moot though,
> so
> I'll see if any of those can be cleaned up.
> WDYT?


SGTM


> https://codereview.chromium.org/193303002/diff/1/media/
> cast/audio_sender/audio_encoder.cc
> File media/cast/audio_sender/audio_encoder.cc (left):
>
> https://codereview.chromium.org/193303002/diff/1/media/
> cast/audio_sender/audio_encoder.cc#oldcode176
> media/cast/audio_sender/audio_encoder.cc:176: private:
> On 2014/03/10 22:02:08, Ami Fischman wrote:
>
>> PRIVATE!!!
>
> Look up; it was extra private. :)
>

I know.  I think you failed to UNDERSTAND MY USE OF CAPITALIZATION!!!!


>
> https://codereview.chromium.org/193303002/
>

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

[DaleCurtis, 2014-03-10 22:22:39]

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_receiver/au...
File media/cast/audio_receiver/audio_receiver.h (right):

https://codereview.chromium.org/193303002/diff/1/media/cast/audio_receiver/au...
media/cast/audio_receiver/audio_receiver.h:142: // variables might be accessed.
On 2014/03/10 22:02:08, Ami Fischman wrote:
> Call me crazy but this comment is starting to seem repetitive :)
> I'd much rather, for example, know which thread this factory was bound to...

Seemingly it's only repetitive because you're looking at a CL full of them :) 
Digging through every one of these changes to figure out the threading story
appears non-trivial at first glance.

If you're strongly opposed I can remove the text and we can hopefully rely on
the clang-plugin in the future.

[Ami GONE FROM CHROMIUM, 2014-03-10 22:29:04]

I am not strongly opposed but I predict you will come to RUE this comment
as you see it copy/pasted from here till eternity ;)

Maybe we need a
MUST_APPEAR_LAST_FOR_DESTRUCTION_ORDER_SANITY(WeakPtrFactory<Foo>
weak_this_);
macro...
(that would be
#define M_A_L_F_D_O_S(x) x
possibly with that comment to 'splain)


On Mon, Mar 10, 2014 at 3:22 PM, <dalecurtis@chromium.org> wrote:

>
> https://codereview.chromium.org/193303002/diff/1/media/
> cast/audio_receiver/audio_receiver.h
> File media/cast/audio_receiver/audio_receiver.h (right):
>
> https://codereview.chromium.org/193303002/diff/1/media/
> cast/audio_receiver/audio_receiver.h#newcode142
> media/cast/audio_receiver/audio_receiver.h:142: // variables might be
> accessed.
> On 2014/03/10 22:02:08, Ami Fischman wrote:
>
>> Call me crazy but this comment is starting to seem repetitive :)
>> I'd much rather, for example, know which thread this factory was bound
>>
> to...
>
> Seemingly it's only repetitive because you're looking at a CL full of
> them :)  Digging through every one of these changes to figure out the
> threading story appears non-trivial at first glance.
>
> If you're strongly opposed I can remove the text and we can hopefully
> rely on the clang-plugin in the future.
>
> https://codereview.chromium.org/193303002/
>

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

[DaleCurtis, 2014-03-10 23:47:45]

Macro seems obtuse, however I did shorten the message to a one-liner for the
sake of copy pasta.

"weak_this" removed where possible.

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.cc:70: main_weak_this_ =
main_weak_factory_.GetWeakPtr();
On 2014/03/10 22:02:08, Ami Fischman wrote:
> (if you take my suggestion in the .h then you can move this back to an
> initializer)

Done.

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/media_source_delegate.h (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/andro...
content/renderer/media/android/media_source_delegate.h:240:
base::WeakPtr<MediaSourceDelegate> main_weak_this_;
On 2014/03/10 22:02:08, Ami Fischman wrote:
> This reversal (putting the factory _after_ a ptr vended from it) looks weird
to
> me.  Flip them back?

Done.

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/video...
File content/renderer/media/video_capture_impl_manager.h (right):

https://codereview.chromium.org/193303002/diff/1/content/renderer/media/video...
content/renderer/media/video_capture_impl_manager.h:106: 
On 2014/03/10 22:02:08, Ami Fischman wrote:
> This empty line distances the factory below from the comment 2 lines up which
> applies to it.  Remove blank line or rewrite the comment above to be clearer?

Done.

https://codereview.chromium.org/193303002/diff/1/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/193303002/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:318: DISALLOW_COPY_AND_ASSIGN(SendProcess);
On 2014/03/10 22:02:08, Ami Fischman wrote:
> Call out in CL description.

Done.

https://codereview.chromium.org/193303002/diff/1/media/video/capture/mac/vide...
File media/video/capture/mac/video_capture_device_mac.mm (right):

https://codereview.chromium.org/193303002/diff/1/media/video/capture/mac/vide...
media/video/capture/mac/video_capture_device_mac.mm:323:
weak_factory_.AsWeakPtr(),
On 2014/03/10 22:02:08, Ami Fischman wrote:
> s/As/Get/??

Done.

[Ami GONE FROM CHROMIUM, 2014-03-12 18:57:42]

LGTM

[DaleCurtis, 2014-03-17 16:06:37]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-17 16:07:23]

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

[commit-bot: I haz the power, 2014-03-17 16:35:41]

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

[commit-bot: I haz the power, 2014-03-17 16:35:41]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg
  tryserver.chromium on mac_chromium_rel

[DaleCurtis, 2014-03-17 17:37:51]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-17 17:38:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/193303002/40001

[commit-bot: I haz the power, 2014-03-17 19:00:38]

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

[commit-bot: I haz the power, 2014-03-17 19:00:38]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[DaleCurtis, 2014-03-17 20:37:41]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-17 20:38:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/193303002/40001

[commit-bot: I haz the power, 2014-03-18 12:29:29]

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

[commit-bot: I haz the power, 2014-03-18 12:29:30]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[DaleCurtis, 2014-03-18 17:09:20]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-18 17:12:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/193303002/80001

[commit-bot: I haz the power, 2014-03-18 19:26:56]

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

[commit-bot: I haz the power, 2014-03-18 19:26:58]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[DaleCurtis, 2014-03-18 19:36:52]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-18 19:39:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/193303002/80001

[commit-bot: I haz the power, 2014-03-18 21:26:07]

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

[commit-bot: I haz the power, 2014-03-18 21:26:08]

Failed to apply patch for media/video/capture/mac/video_capture_device_mac.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/video/capture/mac/video_capture_device_mac.mm
  Hunk #1 FAILED at 126.
  Hunk #2 succeeded at 313 (offset -7 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
media/video/capture/mac/video_capture_device_mac.mm.rej

Patch:       media/video/capture/mac/video_capture_device_mac.mm
Index: media/video/capture/mac/video_capture_device_mac.mm
diff --git a/media/video/capture/mac/video_capture_device_mac.mm
b/media/video/capture/mac/video_capture_device_mac.mm
index
1c69bdb2ebba15eb950d67126e4b033c54a0b68c..2563526e600c6ceb045d145e7d23bc186c85bfa3
100644
--- a/media/video/capture/mac/video_capture_device_mac.mm
+++ b/media/video/capture/mac/video_capture_device_mac.mm
@@ -126,10 +126,8 @@ VideoCaptureDeviceMac::VideoCaptureDeviceMac(const Name&
device_name)
       tried_to_square_pixels_(false),
       task_runner_(base::MessageLoopProxy::current()),
       state_(kNotInitialized),
-      weak_factory_(this),
-      weak_this_(weak_factory_.GetWeakPtr()),
-      capture_device_(nil) {
-}
+      capture_device_(nil),
+      weak_factory_(this) {}
 
 VideoCaptureDeviceMac::~VideoCaptureDeviceMac() {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -320,8 +318,9 @@ void VideoCaptureDeviceMac::ReceiveFrame(
 
 void VideoCaptureDeviceMac::ReceiveError(const std::string& reason) {
   task_runner_->PostTask(FROM_HERE,
-      base::Bind(&VideoCaptureDeviceMac::SetErrorState, weak_this_,
-          reason));
+                         base::Bind(&VideoCaptureDeviceMac::SetErrorState,
+                                    weak_factory_.GetWeakPtr(),
+                                    reason));
 }
 
 void VideoCaptureDeviceMac::SetErrorState(const std::string& reason) {

[DaleCurtis, 2014-03-18 21:36:30]

Committed patchset #5 manually as r257765 (presubmit successful).