If the loop at line 202 (https://hg.mozilla.org/mozilla-central/annotate/10a237997b8d/dom/media/webm/IntelWebMVideoDecoder.cpp#l202) runs more than one, subsequent VP8Sample allocations will overwrite earlier ones stored in the aSample out-parameter. This was pointed out on IRC but nobody had bothered to file a bug for it.

Hi Matthew, I looked at this code and it appears we have a memory leak. I think the best option is to simply hoist the "new VP8Sample" call on line 202 outside the loop and function and instantiate only one VP8Sample object per instance of the class. Then line 202 will just be a reassignment on successive calls. If you concur I'll make the change and submit a patch.

We can remove the loop entirely and return an error if count > 1. As a historical accident (I think?) we ended up with code handling multiple frames per packet in the WebM code. In practice this never happens, so we're better off to remove the handling and have simpler code.

Comment on attachment 8611622 [details] [diff] [review] bug1157991_v0.patch Forgot to update the comment in DemuxPacket().

And for ease of later code archaeology: the bug I duped to this one points to bug 1158226 comment 10 as the justification for this change.

Helps if I attach the right file. Apologies for bug/review spam.

Comment on attachment 8612114 [details] [diff] [review] bug1157991_v1.patch Review of attachment 8612114 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the review delay. Missed this yesterday. I guess we were already assuming the one-frame-per-packet anyway. Do we have a test for the rejecting files which aren't? ::: dom/media/webm/WebMReader.cpp @@ +929,5 @@ > if (r == -1) { > return nullptr; > } > + vpx_codec_stream_info_t si; > + memset(&si, 0, sizeof(si)); BTW mozilla style would have this as PodZero(&si); these days. Better to be consistent with the rest of the file though.

(In reply to Ralph Giles (:rillian) from comment #9) > I guess we were already assuming the one-frame-per-packet anyway. Do we have > a test for the rejecting files which aren't? We don't, but I'll generate one and add it.