1166282 - MediaSourceReader::GetBuffered may memmove an nsAutoTArray

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

This is caught by the analysis implemented in bug 1159433.

 4:55.38 /Users/ehsan/moz/src/dom/media/mediasource/MediaSourceReader.cpp:1007:3: error: Cannot instantiate 'nsTArray' with non-memmovable template argument 'mozilla::media::TimeIntervals'
 4:55.38   nsTArray<media::TimeIntervals> activeRanges;
 4:55.38   ^
 4:55.38 ../../../dist/include/nsTArray.h:1971:1: note: 'nsTArray' declared here
 4:55.38 class MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray : public nsTArray_Impl<E, nsTArrayInfallibleAllocator>
 4:55.38 ^
 4:55.39 ../../../dist/include/Intervals.h:541:3: note: 'nsTArray' Cannot accept classes that are marked as non-memmovable as template argument because of this declaration
 4:55.39   ContainerType mIntervals;
 4:55.39   ^
 4:55.47 1 error generated.

Comment 1

[(no longer active)]

Attachment: Convert IntervalSet::ContainerType to a normal array to make it safe to store TimeIntervals objects in nsTArrays

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8607538 [details] [diff] [review]
Convert IntervalSet::ContainerType to a normal array to make it safe to store TimeIntervals objects in nsTArrays

Review of attachment 8607538 [details] [diff] [review]:
-----------------------------------------------------------------

jya I think should look at this.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8607538 [details] [diff] [review]
Convert IntervalSet::ContainerType to a normal array to make it safe to store TimeIntervals objects in nsTArrays

Review of attachment 8607538 [details] [diff] [review]:
-----------------------------------------------------------------

That is a false positive. Only the content of the nsAutoTArray may be memmove, not the whole array, I made sure of this as I hit that exact crash (I opened bug 1164444)

In MediaSourceReader::GetBuffered I commented (and linked to 1164444) on how I got around it by setting the nsTArray capacity *before* storing TimeIntervals in it. That makes it so that the destination nsTArray can't be later resized which could then have caused the nsAutoTArray to be moved.

I also have a fix for nsAutoTArray to make it movable.
I much prefer to fix it the proper way than have a workaround.

Or have a method to tell the static analyser to ignore a particular use.

Comment 4

[Jean-Yves Avenard [:jya]]

Marking as duplicate.

The short term fix is to have a specialisation for nsTArray_CopyChooser.

Comment 5

[Jean-Yves Avenard [:jya]]

Actually, I will fix that one on its own with a TArray_Copy specialization. Will fix the generic use of nsAutoTArray separately in bug 116444

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Have TimeIntervals and IntervalSet nsTArray_CopyChooser specialization

Have TimeIntervals and IntervalSet nsTArray_CopyChooser specialization. I will make nsAutoTArray memmovable in another patch. It it worth pointing that in the current code, the problem could *NOT* have happened and special care was taken to ensure it didn't. Now, I felt it was cleaner to have the respective specialization where the object is defined rather than in nsArray.h (even though there's already a few defined there). Let me know if you think otherwise, in which case I'll move them in nsArray.h

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8607907 [details] [diff] [review]
Have TimeIntervals and IntervalSet nsTArray_CopyChooser specialization

Review of attachment 8607907 [details] [diff] [review]:
-----------------------------------------------------------------

Sure.  (Reading the code in passing, it seems sad that TimeUnit duplicates a lot of TimeDuration, but maybe there's no way around that...)

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> Comment on attachment 8607907 [details] [diff] [review]
> Have TimeIntervals and IntervalSet nsTArray_CopyChooser specialization
> 
> Review of attachment 8607907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sure.  (Reading the code in passing, it seems sad that TimeUnit duplicates a
> lot of TimeDuration, but maybe there's no way around that...)

Indeed. Replacing with typedef mozilla::TimeDuration TimeUnit seems like a feasibility. TBH, I just didn't know about that class.. The only thing we wanted extra to TimeDuration was the ability to detect overflow.

I'm no fan however of the distinction between TimeStamp and TimeDuration and that arithmetic operations on one give you the other. That is probably the biggest hurdle in swapping for TimeDuration.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Jean-Yves Avenard [:jya] from comment #8)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #7)
> > Sure.  (Reading the code in passing, it seems sad that TimeUnit duplicates a
> > lot of TimeDuration, but maybe there's no way around that...)
> 
> Indeed. Replacing with typedef mozilla::TimeDuration TimeUnit seems like a
> feasibility. TBH, I just didn't know about that class.. The only thing we
> wanted extra to TimeDuration was the ability to detect overflow.

Yeah, StickyTimeDuration is pretty close:

https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/StickyTimeDuration.h

I guess you'd implement your own TimeDurationValueCalculator that uses checked arithmetic types?  Or something like that.

> I'm no fan however of the distinction between TimeStamp and TimeDuration and
> that arithmetic operations on one give you the other. That is probably the
> biggest hurdle in swapping for TimeDuration.

This would be an interesting discussion to have offline.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1773e7af4fb5

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1773e7af4fb5