1403706 - Discolored or missing src=blob: image on www.bmw-motorrad.de

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Opening [1] in a tab sometimes loads the top image and sometimes doesn't. Keep reloading (F5 / Cmd+R / Ctrl+R) the page and the teaser image will sometimes be discolored and misaligned [2].

This is reproducible on Linux and macOS, I didn't try Windows.

Firefox 55 and 56 are not affected. This is a regression introduced by 57.

Chrome renders the page fine every time I reload.

When on the first load the page didn't load I saw the following messages in the console:
> Corrupt JPEG data: 53723 extraneous bytes before marker 0xd9
> Corrupt JPEG data: premature end of data segment

A corrupted image shows the following:
> Corrupt JPEG data: 49891 extraneous bytes before marker 0xd9


[1] https://www.bmw-motorrad.de/de/models/adventure/s1000xr.html
[2] https://imgur.com/a/Adb72

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #0)
> When on the first load the page didn't load I saw the following messages in

Correction: "When on the first load the image didn't show ...".

Comment 2

[Andrea Marchesini [:baku]]

I cannot reproduce this issue but i suspect the bug is in how SourceBuffer works with async inputStreams.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Coarse regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7a6baa6cca3292e8099e652b64d27e74df560874&tochange=e916ab827babb677ce5ab2cac0390c1401eaca0e

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Narrower:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=464b2a3c25aa1065760d9ecbb0870bca4a66c62e&tochange=f31652d75fb5f377db8de3da30b0252600d3c8ca

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

mozregression blames bug 1373222.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Sorry, definitely affects 55 and 56. Didn't reproduce when I tried yesterday :(

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Marking ESR as affected because bug 1373222 was uplifted to 52 too.

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

FWIW, I can reproduce in a debug build when I open the test page and then run the following in the web console: for (var i = 0; i < 10; ++i) window.open(this.location, "", "width=1000");
Need to enable popups.

Comment 9

[Andrea Marchesini [:baku]]

I managed to reproduce this issue. The bug is in how we manage temporary file blobs.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57. I hope we can get a fix ready soon

Comment 11

[Andrea Marchesini [:baku]]

I have some patches to test. I'll ask for a review asap.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - max memory use in MutableBlobStorage

Comment 13

[Andrea Marchesini [:baku]]

This first patch is required for the porting of MediaRecorder to MutableBlobStorage. See next patch.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - MediaRecorder should use MutableBlobStorage

The goal of this patch is to unify the creation of temporary Blobs. MutableBlobStorage will be deeply changed in the following patches.
If needed, a follow up could be to have MutableBlobStorage able to work out of the main-thread when ::Append() is called.

Note that ::Append() doesn't use the main-thread if there is a temporary file involved.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 3 - NS_OpenAnonymousTemporaryNsIFile

This is needed by the following patch.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 4 - nsFileInputStream & DELETE_ON_CLOSE

Currently nsFileInputStream is buggy when DELETE_ON_CLOSE is used. Here why:

1. nsFileInputStream should not be IPCSerializable when DELETE_ON_CLOSE behavior is set, otherwise, a) in ::Serialize() we call Close(). This fails on windows because we have an handle keeping the file alive, but we nullify mFile. b) DELETE_ON_CLOSE is not sent via IPC. c) because of mFile is null, the file is not deleted on windows. Good news: we don't use nsIFileInputStream::DELETE_ON_CLOSE yet.

2. When the stream is cloned, DELETE_ON_CLOSE should not be passed, otherwise there is a race condition about which stream will be deleted the file. Better to keep DELETE_ON_CLOSE on the 'original' stream.

All of these is needed for the next patch.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileBlobImpl with stream file behavior

Let's have the stream file behavior in FileBlobImpl.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 6 - PTemporaryIPCBlob

Here the main part. MutableBlobStorage uses a PTemporaryIPCBlob protocol in order to:

1. go to the parent process (via PBackground) and ask for a file descriptor.
2. When the file descriptor is received, MutableBlobStorage writes data into it.
3. At the end of the operation, the parent actor sends and IPCBlob back to the child actor.

Note that the parent actor, sending the IPCBlob, indirectly stores the internal inputStream into IPCBlobInputStreamStorage. That inputStream is a nsIFileInputStream with DELETE_ON_CLOSE flag (because FileBlobImpl has such behavior).

IPCBlobInputStreamStorage clones the original inputStreams each time the child PIPCBlobInputStream needs a new 'real' stream.

When the last PIPCBlobInputStream child actor is released, IPCBlobInputStreamStorage will release the file inputStream and the temporary file is deleted.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 7 - Get rid of TemporaryBlobImpl

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 8 - Get rid of nsTemporaryFileInputStream

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 9 - Get rid of PContent temporary file methods

Comment 22

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Is there some minimal patch we could take, since we need to which this on branches too, I think?

Comment 23

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(and the patchset isn't anything simple enough for branches)

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8914066 [details] [diff] [review]
part 9 - Get rid of PContent temporary file methods

This is still needed elsewhere.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 2 - MediaRecorder should use MutableBlobStorage

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 6 - PTemporaryIPCBlob

Proper propagation of content type. Now these patches are fully green on try.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileBlobImpl with stream file behavior

Comment 28

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #22)
> Is there some minimal patch we could take, since we need to which this on
> branches too, I think?

If we don't care too much about memory usage, we can make TemporaryBlobImpl::GetInternalStream to return a pipe and move all in memory.

Comment 29

[Paul Adenot (:padenot)]

Comment on attachment 8914198 [details] [diff] [review]
part 2 - MediaRecorder should use MutableBlobStorage

Andreas (:perhsons) or Bryce (:SingingTree) are better reviewers for MediaRecorder things, these days.

Comment 30

[Andrea Marchesini [:baku]]

Attachment: part 2 - MediaRecorder should use MutableBlobStorage

Sorry for the spam. There was an unhappy test.

Comment 31

[Andrea Marchesini [:baku]]

> If we don't care too much about memory usage, we can make
> TemporaryBlobImpl::GetInternalStream to return a pipe and move all in memory.

Or, faster, let's disable MutableBlobStorage completely, but we still need MediaRecorder to use MutableBlobStorage.

Comment 32

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8914218 [details] [diff] [review]
part 2 - MediaRecorder should use MutableBlobStorage

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

Looks good to me, thanks!

::: dom/media/MediaRecorder.cpp
@@ +252,4 @@
>      RefPtr<Session> mSession;
> +
> +    // The generation of the blob is async. In order to dispatch DestroyRunnable
> +    // always after the pushing of the blob event, we store the runnable here.

Perhaps:
In order to avoid dispatching the DestroyRunnable before pushing the blob event, we store the runnable here.

Also mention that we only dispatch a DestroyRunnable if it was passed in.

Comment 33

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914053 [details] [diff] [review]
part 1 - max memory use in MutableBlobStorage

> MutableBlobStorage::MutableBlobStorage(MutableBlobStorageType aType,
>-                                       nsIEventTarget* aEventTarget)
>+                                       nsIEventTarget* aEventTarget,
>+                                       uint32_t aMaxMemory)
It is unclear to me why we need aMaxMemory. Perhaps some followup patch explains.

>   : mData(nullptr)
>   , mDataLen(0)
>   , mDataBufferLen(0)
>   , mStorageState(aType == eOnlyInMemory ? eKeepInMemory : eInMemory)
>   , mFD(nullptr)
>   , mErrorResult(NS_OK)
>   , mEventTarget(aEventTarget)
>+  , mMaxMemory(aMaxMemory)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>   if (!mEventTarget) {
>     mEventTarget = GetMainThreadEventTarget();
>   }
> 
>+  if (aMaxMemory == 0 && aType == eCouldBeInTemporaryFile) {
>+    mMaxMemory = Preferences::GetUint("dom.blob.memoryToTemporaryFile",
>+                                      BLOB_MEMORY_TEMPORARY_FILE);
>+  }
>+
>   MOZ_ASSERT(mEventTarget);
> }
> 
> MutableBlobStorage::~MutableBlobStorage()
> {
>   free(mData);
> 
>   if (mFD) {
>@@ -565,18 +572,17 @@ MutableBlobStorage::ShouldBeTemporarySto
> 
>   CheckedUint32 bufferSize = mDataLen;
>   bufferSize += aSize;
> 
>   if (!bufferSize.isValid()) {
>     return false;
>   }
> 
>-  return bufferSize.value() >= Preferences::GetUint("dom.blob.memoryToTemporaryFile",
>-                                                    BLOB_MEMORY_TEMPORARY_FILE);
>+  return bufferSize.value() >= mMaxMemory;
> }
I don't understand the setup. mMaxMemory is updated from the pref only if type is eCouldBeInTemporaryFile.
Otherwise it can be 0, and MutableBlobStorage::ShouldBeTemporaryStorage return always true.
Maybe I'm missing something here, but why this behavior? Or is code relying that this method is called only when 
eCouldBeInTemporaryFile was passed to the ctor?
Please explain this.


>+  // Returns the heap size in bytes of our internal buffers.
>+  // Note that this intentionally ignores the data in the temp file.
>+  size_t SizeOfCurrentBuffer() const;
Well, shouldn't its name be then something else. SizeOfCurrentMemoryBuffer perhaps?

Comment 34

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914204 [details] [diff] [review]
part 5 - FileBlobImpl with stream file behavior

This needs comments what aBehaviorFlags are, and when one should use them and why.
Would be also be good to explain how those flags don't override sFileStreamFlags.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: ESR52

for ESR52, I propose a simpler patch. This will disable the use of TemporaryBlobImpl.

Comment 36

[Andrea Marchesini [:baku]]

Attachment: part 1 - max memory use in MutableBlobStorage

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileBlobImpl with stream file behavior

Comment 38

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914319 [details] [diff] [review]
part 1 - max memory use in MutableBlobStorage

It is still hard to understand mMaxMemory handling. What if our type is eOnlyInMemory.
Looks like one needs to read http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/file/MutableBlobStorage.cpp#388 and 
http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/dom/file/MutableBlobStorage.cpp#492 to understand the setup.

Perhaps clarify that mMaxMemory is never used when the object is created with eOnlyInMemory.

Comment 39

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914313 [details] [diff] [review]
ESR52

The MediaRecorder part needs review from a media person. I'm worried about memory usage, so I'd give r- for that. But if media folks are ok with using lots of more memory, then fine.

Can we focus on fixing the actual regression. What in bug 1373222 caused the issue?

Comment 40

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914218 [details] [diff] [review]
part 2 - MediaRecorder should use MutableBlobStorage

>     // Append pulled data into cache buffer.
>-    for (uint32_t i = 0; i < encodedBuf.Length(); i++) {
>-      if (!encodedBuf[i].IsEmpty()) {
>-        mEncodedBufferCache->AppendBuffer(encodedBuf[i]);
>+    RefPtr<Session> self = this;
>+    NS_DispatchToMainThread(NS_NewRunnableFunction(
>+      "MediaRecorder::Session::Extract",
>+      [self, encodedBuf]() {
>+        self->MaybeCreateMutableBlobStorage();
>+        for (uint32_t i = 0; i < encodedBuf.Length(); i++) {
>+          if (!encodedBuf[i].IsEmpty()) {
>+            self->mMutableBlobStorage->Append(encodedBuf[i].Elements(),
>+                                              encodedBuf[i].Length());
>+          }
>+        }
>       }
Doesn't this end up making copy of encodedBuf. That sounds like rather memory hungry.

Comment 41

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Andrea Marchesini [:baku] from comment #16)
 > 2. When the stream is cloned, DELETE_ON_CLOSE should not be passed,
> otherwise there is a race condition about which stream will be deleted the
> file. Better to keep DELETE_ON_CLOSE on the 'original' stream.
Why? Shouldn't we rather have some refcounted thing, which, when drops to zero deletes the file.

Comment 42

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914060 [details] [diff] [review]
part 4 - nsFileInputStream & DELETE_ON_CLOSE

I think I mostly don't understand at all what this is about :)
... and the clone handling is a tad surprising, isn't it.

Comment 43

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914327 [details] [diff] [review]
part 5 - FileBlobImpl with stream file behavior

> FileBlobImpl::CreateInputStream(nsIInputStream** aStream, ErrorResult& aRv)
> {
>   nsCOMPtr<nsIInputStream> stream;
>+
>+  uint32_t flags = sFileStreamFlags;
>+  if (mFileType == eTemporaryFile) {
>+    flags |= nsIFileInputStream::DELETE_ON_CLOSE;
>+    mInputStreamCreated = true;
mInputStreamCreated is debug build member variable, so this doesn't compile on opt builds.
And shouldn't mInputStreamCreated be set only after stream creation worked.


But, mInputStreamCreated is unused variable. My guess is that you didn't upload the latest version of the patch.

Comment 44

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914203 [details] [diff] [review]
part 6 - PTemporaryIPCBlob

>   // If the object has been already closed and we don't need to execute a
>   // callback, we need just to close the file descriptor in the correct thread.
>   if (mStorageState == eClosed && !mPendingCallback) {
>     RefPtr<Runnable> runnable = new CloseFileRunnable(aFD);
>     DispatchToIOThread(runnable.forget());
>+
>+    // Let's inform the parent that we have nothing else to do.
>+    mActor->SendOperationDone(false, nsCString());
Doesn't EmptyCString() work here?

> MutableBlobStorage::ErrorPropagated(nsresult aRv)
> {
>   MOZ_ASSERT(NS_IsMainThread());
>   mErrorResult = aRv;
>+
>+  mActor->SendOperationDone(false, nsCString());
Same here

>+mozilla::ipc::IPCResult
>+TemporaryIPCBlobChild::RecvFileDesc(const FileDescriptor& aFD)
>+{
>+  MOZ_ASSERT(mActive);
>+
>+  auto rawFD = aFD.ClonePlatformHandle();
Could you not use rawFD here. It nicely hides the real type.


>+  RefPtr<FileBlobImpl> blobImpl =
>+    new FileBlobImpl(file, NS_LITERAL_STRING("temporary"),
>+                     NS_ConvertUTF8toUTF16(aContentType),
>+                     nsIFileInputStream::DELETE_ON_CLOSE);
Why "temporary" as a name?

Comment 45

[Andrea Marchesini [:baku]]

Attachment: part 2 - MediaRecorder should use MutableBlobStorage

Comment 46

[Andrea Marchesini [:baku]]

> Can we focus on fixing the actual regression. What in bug 1373222 caused the
> issue?

A temporary blob, in general, is racy: if you do 2 FileReaders, probably one of the 2 readers (or both) will fail, because both tries to read using the same FileDescriptor. With URL.createObjectURL we are exposing the same level inconsistence everywhere else in gecko: if the temporary inputStream is used more than once at the same time, we end up having issues.

A 'temporary' solution here is to avoid the use of Temporary Blobs. Nothing related with URL.createObjectURL.

Comment 47

[Andrea Marchesini [:baku]]

Attachment: part 5 - FileBlobImpl with stream file behavior

Comment 48

[Andrea Marchesini [:baku]]

Attachment: part 4 - Get rid of nsIFileInputStream::DELETE_ON_CLOSE

This can land in a separate bug.

Comment 49

[Andrea Marchesini [:baku]]

Attachment: part 5 - TemporaryFileBlobImpl

Comment 50

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8914313 [details] [diff] [review]
ESR52

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

Sad to see this feature go for esr, but if it avoids races, sure.

::: dom/media/EncodedBufferCache.cpp
@@ +54,5 @@
>        }
>      }
>      mEncodedBuffers.Clear();
>    }
> +#endif

Should be enough to comment out the first if.

Comment 51

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Do we have any MediaRecorder tests which might cause high memory usage with that patch? Even just manual tests? Or which websites might use the API?

Comment 52

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8914313 [details] [diff] [review]
ESR52

Note that my r+ will go for the MediaRecorder part, not MutableBlobStorage.

Some more details on the cause of the racing would be useful too, to understand whether MediaRecorder is susceptible to this race.

Comment 53

[Andrea Marchesini [:baku]]

Attachment: part 6 - PTemporaryIPCBlob

Comment 54

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914686 [details] [diff] [review]
part 5 - TemporaryFileBlobImpl

>+const uint32_t sTemporaryFileStreamFlags =
>+  nsIFileInputStream::REOPEN_ON_REWIND;
This needs a good comment

>+  explicit TemporaryFileInputStream(nsIFile* aFile)
>+    : mFile(aFile)
>+  {}
Please add some assertion that this is called only on the parent process


>+TemporaryFileBlobImpl::TemporaryFileBlobImpl(nsIFile* aFile,
>+                                             const nsAString& aContentType)
>+  : FileBlobImpl(aFile, EmptyString(), aContentType)
>+#ifdef DEBUG
>+  , mInputStreamCreated(false)
>+#endif
>+{}
Similar assertion here.

>+// This class is meant to be used by TemporaryIPCBlobParent only.
>+// Don't use it for anything else, please!
>+// Note that CreateInputStream() _must_ be called, and called just once!
>+
>+class TemporaryFileBlobImpl final : public FileBlobImpl
Please explain in the comment why this class is a BlobImpl, even though no Blob ever points to such object
via mImpl.

Comment 55

[Andreas Pehrson [:pehrsons]]

(In reply to Olli Pettay [:smaug] from comment #51)
> Do we have any MediaRecorder tests which might cause high memory usage with
> that patch? Even just manual tests? Or which websites might use the API?

I can't imagine that we have mochitests that run for long enough, or with video large enough to trigger any issues here. Recording of camera content, which is probably the most common, uses around 20 MB/min for VGA by default.

For a manual test you can use https://jsfiddle.net/pehrsons/a9gugbut/. Click Start to start capturing and record. Stop to stop recording and download the file.

Note also that Chrome's MediaRecorder doesn't use a file to back encoded data, so services recording long and high-resolution videos should be limited.

Comment 56

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914617 [details] [diff] [review]
part 2 - MediaRecorder should use MutableBlobStorage

>   // Main thread task.
>   // Create a blob event and send back to client.
>   class PushBlobRunnable : public Runnable
>+                         , public MutableBlobStorageCallback
>   {
>   public:
>-    explicit PushBlobRunnable(Session* aSession)
>+    NS_DECL_ISUPPORTS_INHERITED
>+
>+    // aDestroyRunnable can be null. If it's not, it will be dispatched after
>+    // the PushBlobRunnable::Run().
>+    PushBlobRunnable(Session* aSession, Runnable* aDestroyRunnable)
>       : Runnable("dom::MediaRecorder::Session::PushBlobRunnable")
>       , mSession(aSession)
>+      , mDestroyRunnable(aDestroyRunnable)
>     { }
> 
>     NS_IMETHOD Run() override
>     {
>       LOG(LogLevel::Debug, ("Session.PushBlobRunnable s=(%p)", mSession.get()));
>       MOZ_ASSERT(NS_IsMainThread());
> 
>+      mSession->GetBlobWhenReady(this);
>+      return NS_OK;
I don't see reason for PushBlobRunnable to be Runnable anymore. 
One could just dispatch runnable method using session and GetBlobWhenReady as the method, and
GetBlobWhenReady itself would create an instance of this class which would inherit only MutableBlobStorageCallback.
But I guess no need to change. It isn't too complicated this way either.

Please test this stuff manually, if we don't have really good automatic tests.

Comment 57

[Andrea Marchesini [:baku]]

Attachment: part 9 - tests

Comment 58

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914709 [details] [diff] [review]
part 6 - PTemporaryIPCBlob

>+TemporaryIPCBlobChild::RecvFileDesc(const FileDescriptor& aFD)
>+{
>+  MOZ_ASSERT(mActive);
>+
>+  auto rawFD = aFD.ClonePlatformHandle();
As I asked before, could you not use auto here. It isn't clear what kind of type we're dealing with there.



This is all super scary.

Comment 59

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8914838 [details] [diff] [review]
part 9 - tests

I don't see why this isn't racy. browser1 may have executed the JS before browser2.

Comment 60

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

IRC:
I wasn't talking about openNewForegroundTab 
but ContentTask.spawn [usage being racy]

Comment 61

[Andrea Marchesini [:baku]]

Attachment: part 9 - tests

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6040c98bc114
Remove race conditions in temporary blob  - part 1 - max memory use in MutableBlobStorage, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb5ca8d115e7
Remove race conditions in temporary blob  - testing, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/47479015f610
Remove race conditions in temporary blob  - part 2 - MediaRecorder should use MutableBlobStorage, r=pehrsons, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e15a074f4bac
Remove race conditions in temporary blob  - part 3 - NS_OpenAnonymousTemporaryNsIFile, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/158c54c25ed1
Remove race conditions in temporary blob  - part 4 - Remove DELETE_ON_CLOSE nsIFileInputStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4d136a609e
Remove race conditions in temporary blob  - part 5 - Introducing TemporaryFileBlobImpl, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf438d8a527
Remove race conditions in temporary blob  - part 6 - Introducing PTemporaryIPCBlob, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4888323f2361
Remove race conditions in temporary blob  - part 7 - Remove TemporaryBlobImpl, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f02f92a49702
Remove race conditions in temporary blob  - part 8 - Remove nsTemporaryFileInputStream, r=smaug

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6040c98bc114
https://hg.mozilla.org/mozilla-central/rev/eb5ca8d115e7
https://hg.mozilla.org/mozilla-central/rev/47479015f610
https://hg.mozilla.org/mozilla-central/rev/e15a074f4bac
https://hg.mozilla.org/mozilla-central/rev/158c54c25ed1
https://hg.mozilla.org/mozilla-central/rev/cb4d136a609e
https://hg.mozilla.org/mozilla-central/rev/1cf438d8a527
https://hg.mozilla.org/mozilla-central/rev/4888323f2361
https://hg.mozilla.org/mozilla-central/rev/f02f92a49702

Comment 64

[Liz Henry (:lizzard) (relman/hg->git project)]

Baku, Tim, do you think we can live with this in 57 or are you looking to uplift these patches? If it is just one known site affected we might want to wontfix for 57.

Comment 65

[Liz Henry (:lizzard) (relman/hg->git project)]

Actually how about this (as I look at the scary patches). I will wontfix for 57 now.  If you disagree, please n-i to Ritu.

Comment 66

[Tim Taubert [:ttaubert] (inactive)]

I can't say much about the complexity of the patches (looks rather high) or whether there's a simpler fix we could uplift. I'll leave this for Andrea :)

Comment 67

[Andrea Marchesini [:baku]]

I would not fix this for 57. These patches are complex enough to require an extra cycle.
If we think that we need it fix for 57, we can disable the memory-to-temporary file feature for Blobs.

Comment 68

[Andrew Overholt [:overholt]]

(In reply to Andrea Marchesini [:baku] from comment #67)
> I would not fix this for 57. These patches are complex enough to require an
> extra cycle.
> If we think that we need it fix for 57, we can disable the
> memory-to-temporary file feature for Blobs.

This code has been around since at least 55 without too many issues so let's leave it and hope it doesn't blow up.