Closed Bug 1403706 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + wontfix
firefox58 --- fixed

People

(Reporter: ttaubert, Assigned: baku)

References

()

Details

(Keywords: regression)

Attachments

(10 files, 13 obsolete files)

2.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.40 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.47 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
4.33 KB, patch
smaug
: review+
Details | Diff | Splinter Review
28.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
35.92 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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
(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 ...".
Version: Other Branch → 57 Branch
OS: Unspecified → All
Hardware: Unspecified → All
Assignee: nobody → amarchesini
I cannot reproduce this issue but i suspect the bug is in how SourceBuffer works with async inputStreams.
mozregression blames bug 1373222.
Blocks: 1373222
Sorry, definitely affects 55 and 56. Didn't reproduce when I tried yesterday :(
Marking ESR as affected because bug 1373222 was uplifted to 52 too.
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.
I managed to reproduce this issue. The bug is in how we manage temporary file blobs.
Tracked for 57. I hope we can get a fix ready soon
I have some patches to test. I'll ask for a review asap.
Attachment #8914053 - Flags: review?(bugs)
This first patch is required for the porting of MediaRecorder to MutableBlobStorage. See next patch.
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.
Attachment #8914054 - Flags: review?(padenot)
Attachment #8914054 - Flags: review?(bugs)
This is needed by the following patch.
Attachment #8914056 - Flags: review?(bugs)
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.
Attachment #8914060 - Flags: review?(bugs)
Let's have the stream file behavior in FileBlobImpl.
Attachment #8914061 - Flags: review?(bugs)
Attached patch part 6 - PTemporaryIPCBlob (obsolete) — Splinter Review
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.
Attachment #8914063 - Flags: review?(bugs)
Attachment #8914066 - Flags: review?(bugs)
Is there some minimal patch we could take, since we need to which this on branches too, I think?
(and the patchset isn't anything simple enough for branches)
Comment on attachment 8914066 [details] [diff] [review]
part 9 - Get rid of PContent temporary file methods

This is still needed elsewhere.
Attachment #8914066 - Attachment is obsolete: true
Attachment #8914066 - Flags: review?(bugs)
Attachment #8914054 - Attachment is obsolete: true
Attachment #8914054 - Flags: review?(padenot)
Attachment #8914054 - Flags: review?(bugs)
Attachment #8914198 - Flags: review?(padenot)
Attachment #8914198 - Flags: review?(bugs)
Attached patch part 6 - PTemporaryIPCBlob (obsolete) — Splinter Review
Proper propagation of content type. Now these patches are fully green on try.
Attachment #8914063 - Attachment is obsolete: true
Attachment #8914063 - Flags: review?(bugs)
Attachment #8914203 - Flags: review?(bugs)
Attachment #8914061 - Attachment is obsolete: true
Attachment #8914061 - Flags: review?(bugs)
Attachment #8914204 - Flags: review?(bugs)
(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 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.
Attachment #8914198 - Flags: review?(padenot) → review?(apehrson)
Sorry for the spam. There was an unhappy test.
Attachment #8914198 - Attachment is obsolete: true
Attachment #8914198 - Flags: review?(bugs)
Attachment #8914198 - Flags: review?(apehrson)
Attachment #8914218 - Flags: review?(bugs)
Attachment #8914218 - Flags: review?(apehrson)
> 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 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.
Attachment #8914218 - Flags: review?(apehrson) → review+
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?
Attachment #8914053 - Flags: review?(bugs) → review-
Attachment #8914056 - Flags: review?(bugs) → review+
Attachment #8914064 - Flags: review?(bugs) → review+
Attachment #8914065 - Flags: review?(bugs) → review+
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.
Attachment #8914204 - Flags: review?(bugs) → review-
Attached patch ESR52Splinter Review
for ESR52, I propose a simpler patch. This will disable the use of TemporaryBlobImpl.
Attachment #8914053 - Attachment is obsolete: true
Attachment #8914319 - Flags: review?(bugs)
Attachment #8914313 - Flags: review?(bugs)
Attachment #8914204 - Attachment is obsolete: true
Attachment #8914327 - Flags: review?(bugs)
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.
Attachment #8914319 - Flags: review?(bugs) → review+
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?
Attachment #8914313 - Flags: review?(bugs) → review-
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.
Attachment #8914218 - Flags: review?(bugs) → review-
(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 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.
Attachment #8914060 - Flags: review?(bugs) → review-
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.
Attachment #8914327 - Flags: review?(bugs) → review-
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?
Attachment #8914203 - Flags: review?(bugs) → review+
> 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.
Attachment #8914313 - Flags: review- → review?(apehrson)
Attachment #8914327 - Attachment is obsolete: true
Attachment #8914617 - Flags: review?(bugs)
Attachment #8914618 - Flags: review?(bugs)
Attachment #8914060 - Attachment is obsolete: true
This can land in a separate bug.
Attachment #8914684 - Flags: review?(bugs)
Attachment #8914618 - Attachment is obsolete: true
Attachment #8914618 - Flags: review?(bugs)
Attachment #8914686 - Flags: review?(bugs)
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.
Attachment #8914313 - Flags: review?(apehrson) → review+
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 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.
Attachment #8914684 - Flags: review?(bugs) → review+
Attachment #8914203 - Attachment is obsolete: true
Attachment #8914709 - Flags: review?(bugs)
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.
Attachment #8914686 - Flags: review?(bugs) → review+
(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 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.
Attachment #8914617 - Flags: review?(bugs) → review+
Attached patch part 9 - tests (obsolete) — Splinter Review
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.
Attachment #8914709 - Flags: review?(bugs) → review+
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.
Attachment #8914838 - Flags: review-
IRC:
I wasn't talking about openNewForegroundTab 
but ContentTask.spawn [usage being racy]
Attached patch part 9 - testsSplinter Review
Attachment #8914838 - Attachment is obsolete: true
Attachment #8915033 - Flags: review+
Pushed by amarchesini@mozilla.com:
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
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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(amarchesini)
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.
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 :)
Flags: needinfo?(ttaubert)
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.
Flags: needinfo?(amarchesini)
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.