Bug 1383579 - SourceBufferIterator::SetWaiting should not assert for spurious wakeups if no consumer was given. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 27 Jul 2017 21:18:17 -0400
changeset 420304 277525fcf8886becf6bd82fdefe73d7057a18c05
parent 420303 312365b8326d02da6af3ee7e950aefaa76c6b2ba
child 420305 07b534f447404246cce2c784ef9f0136d467b7a6
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1383579
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1383579 - SourceBufferIterator::SetWaiting should not assert for spurious wakeups if no consumer was given. r=tnikkel The ICO decoder creates a cloned SourceBufferIterator for its own SourceBuffer bounded by the resource size. This iterator is used by the child decoder (PNG, BMP) for decoding the actual image. However we rely upon the ICO decoder and its iterator to drive event loop, rather than the child decoder and the cloned iterator. The cloned iterator knows how many bytes it requires, but it is problematic to give it a consumer to tell us when to resume without changes to StreamingLexer. Without a consumer (IResumable), we won't have anything to notify when we get the appropriate amount of data for the caller. If the caller tries to advance after some, unknown amount of data has been written to the SourceBuffer, then it may need to go back to waiting. Thus it should only assert for a spurious wakeup if we have an actual consumer.
image/SourceBuffer.cpp
image/SourceBuffer.h
--- a/image/SourceBuffer.cpp
+++ b/image/SourceBuffer.cpp
@@ -616,17 +616,17 @@ SourceBuffer::AdvanceIteratorOrScheduleR
   if (MOZ_UNLIKELY(mStatus && NS_FAILED(*mStatus))) {
     // This SourceBuffer is complete due to an error; all reads fail.
     return aIterator.SetComplete(*mStatus);
   }
 
   if (MOZ_UNLIKELY(mChunks.Length() == 0)) {
     // We haven't gotten an initial chunk yet.
     AddWaitingConsumer(aConsumer);
-    return aIterator.SetWaiting();
+    return aIterator.SetWaiting(!!aConsumer);
   }
 
   uint32_t iteratorChunkIdx = aIterator.mData.mIterating.mChunk;
   MOZ_ASSERT(iteratorChunkIdx < mChunks.Length());
 
   const Chunk& currentChunk = mChunks[iteratorChunkIdx];
   size_t iteratorEnd = aIterator.mData.mIterating.mOffset +
                        aIterator.mData.mIterating.mAvailableLength;
@@ -654,17 +654,17 @@ SourceBuffer::AdvanceIteratorOrScheduleR
     // There's no more data and this SourceBuffer completed successfully.
     MOZ_ASSERT(NS_SUCCEEDED(*mStatus), "Handled failures earlier");
     return aIterator.SetComplete(*mStatus);
   }
 
   // We're not complete, but there's no more data right now. Arrange to wake up
   // the consumer when we get more data.
   AddWaitingConsumer(aConsumer);
-  return aIterator.SetWaiting();
+  return aIterator.SetWaiting(!!aConsumer);
 }
 
 nsresult
 SourceBuffer::HandleError(nsresult aError)
 {
   MOZ_ASSERT(NS_FAILED(aError), "Should have an error here");
   MOZ_ASSERT(aError == NS_ERROR_OUT_OF_MEMORY,
              "Unexpected error; may want to notify waiting readers, which "
--- a/image/SourceBuffer.h
+++ b/image/SourceBuffer.h
@@ -237,20 +237,24 @@ private:
     // Update metrics.
     mChunkCount++;
     mByteCount += aAvailableLength;
 
     // Attempt to advance by the requested number of bytes.
     return AdvanceFromLocalBuffer(aRequestedBytes);
   }
 
-  State SetWaiting()
+  State SetWaiting(bool aHasConsumer)
   {
     MOZ_ASSERT(mState != COMPLETE);
-    MOZ_ASSERT(mState != WAITING, "Did we get a spurious wakeup somehow?");
+    // Without a consumer, we won't know when to wake up precisely. Caller
+    // convention should mean that we don't try to advance unless we have
+    // written new data, but that doesn't mean we got enough.
+    MOZ_ASSERT(mState != WAITING || !aHasConsumer,
+               "Did we get a spurious wakeup somehow?");
     return mState = WAITING;
   }
 
   State SetComplete(nsresult aStatus)
   {
     mData.mAtEnd.mStatus = aStatus;
     return mState = COMPLETE;
   }