Closed Bug 1373222 Opened 3 years ago Closed 3 years ago

URL.createObjectURL crashes Firefox 54

Categories

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

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 56+ fixed
firefox54 + wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: vobruba.martin, Assigned: baku)

References

Details

(Keywords: crash)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.86 Safari/537.36

Steps to reproduce:

1) Download number of 100MB Blobs using XMLHttpRequest.responseType = "blob"
2) Call URL.createObjectURL(new Blob(arrayOfDownloadedBlobs));


Actual results:

In Firefox 53 you get url object almost instantly. In 54 there is a lot of harddisk work and then it crashes.

Tested configurations:
(tested both 32/64bit Firefox 54)
Windows 32bit 7, 8.1 and 10 (2GB RAM, single CPU): crash with 20x100MB.
Windows 64bit 7, 8.1 and 10 (2GB RAM, single CPU): crash with 100x100MB.
OS X 10.12.6 and Linux 64bit (16GB RAM, 4 CPUs): no crash with 500x100MB but noticed worsen performance because of HDD work.

We used to download large files using our application but that is currently not possible.


Expected results:

No crash, url object should be returned as soon as possible.
Component: Untriaged → DOM
Keywords: crash
Product: Firefox → Core
Maybe baku knows what's changed here recently? Could be related to bug 1353629 or maybe e10s-multi?
Flags: needinfo?(amarchesini)
martin, could you provide a testcase (on jsfiddle for example), it would help to narrow down a possible regression.
Flags: needinfo?(vobruba.martin)
I've tested it on this page: https://stat-info.cz/firefox.html
Flags: needinfo?(vobruba.martin)
I'm not able to reproduce it on linux. Can you submit a crash-report?
The IO is done because when a blob is downloaded via XHR (or Fetch, or WebSocket,...), if the size is > 1mb, we write it down into a temporary file, instead keeping it in memory.
Flags: needinfo?(amarchesini) → needinfo?(vobruba.martin)
Is this what you need? https://pastebin.com/PcSPhUuV

On Linux and OS X there is no crash. But on all systems I noticed a lot of IO during URL.createObjectURL call if the resulting size is >= RAM.
Flags: needinfo?(vobruba.martin)
It seems a OOM crash. Can you open about:crashes and give me the IDs of the crash reports? Thanks!
Flags: needinfo?(vobruba.martin)
Priority: -- → P3
Ok, I know what's happening here. Several months ago, I implemented MutableBlobStorage object. This object is used to store data when a blob is created by a XHR, a Fetch, and so on. Initially MutableblobStorage memorizes data in a memory buffer, but if the buffer size gets higher than 1mb, it starts writing the incoming data in a temporary file. The migration from in-memory buffer to temporary-file is async.

What happens here is that, if the downloading of 100mb is faster than writing of the temporary file, MutableBlobStorage continues to allocate memory and, at the end, it will return to the XHR object a in-memory Blob, instead of a temporary-file Blob.

At this point, URL.createObjectURL() broadcasts the Blob to other processes, and we crash because we try to allocate a IPC message with 100mb of data.
[Tracking Requested - why for this release]:
Regression causing crashes. We might need the fix in a 54.x release.
Wasn't MutableBlobStorage also present in earlier versions? Because we never saw these crashes in <=53.
Attached patch crash_url.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8879074 - Flags: review?(bugs)
Please explain the patch.
Flags: needinfo?(amarchesini)
(In reply to Martin Vobruba from comment #10)
> Wasn't MutableBlobStorage also present in earlier versions? Because we never
> saw these crashes in <=53.

Yes, but in 53 blobs URL were not broadcasted across processes because it was non-e10s, plus, the remote Blob code is changed a lot in the latest months. What I suspect is that the bug was always there, but not exposed as it is right now.
Flags: needinfo?(amarchesini)
Comment 8 explains the bug. The patch makes MutableBlobStorage to wait for the creation of the temporary blob if the operation has started but unfinished when the blob is requested by XHR/Fetch.

If we are in aWaitingForTemporaryFile state, we just wait until the file Descriptor is received. At that point, the callback is executed and the workflow continues.
Comment on attachment 8879074 [details] [diff] [review]
crash_url.patch

>+
>+  if (mStorageState == eClosed) {
>+    MOZ_ASSERT(mPendingCallback);
>+
>+    RefPtr<Runnable> runnable =
>+      new LastRunnable(this, mPendingParent, mPendingContentType,
>+                       mPendingCallback);
>+    DispatchToIOThread(runnable.forget());
>+
>+    mPendingParent = nullptr;
>+    mPendingCallback = nullptr;
>+  }
So what will eventually dispatch CloseFileRunnable ?
Or why is it not needed in this case.

This need some good comment. I can't now figure out the setup.
Attachment #8879074 - Flags: review?(bugs) → review-
Attached patch crash_url.patchSplinter Review
More comments.
Attachment #8879074 - Attachment is obsolete: true
Attachment #8879420 - Flags: review?(bugs)
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

>+  // If we still receiving data, we can proceed in temporary-file mode.
If we are still...


Better to push this to try with debug builds.
Attachment #8879420 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee152ab292bb
MutableBlobStorage always returns a temporary-file Blob if the size of data is greater than 1mb, r=smaug
https://hg.mozilla.org/mozilla-central/rev/ee152ab292bb
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Hi Martin,
Can you help check if this issue is fixed in the latest nightly?
Flags: needinfo?(vobruba.martin)
Yes, this issue seems fixed (tested 64bit build on 64bit Win8.1 and 32bit build on 32bit Win10).

Will you fix it in 54, please? Thanks!
Flags: needinfo?(vobruba.martin) → needinfo?(amarchesini)
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1202006 - MutableBlobStorage
[User impact if declined]: Big Memory Blobs can be kept in memory if the networking is faster than I/O. If these blobs are sent via IPC, we can go OOM. 
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: there is a test attached 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: none.
[Why is the change risky/not risky?]: If the size of the blob is greater than 1mb, we wait until the blob is stored in a temporary file also if the networking is faster than I/O.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8879420 - Flags: approval-mozilla-beta?
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

fix oom from blob storage on windows, beta55+
Attachment #8879420 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
If you think this is severe enough to warrant consideration for inclusion in a 54 dot release, please nominate it for release approval.
Blocks: 1202006
Flags: needinfo?(amarchesini)
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Approval Request Comment: See comment 23.
Flags: needinfo?(amarchesini)
Attachment #8879420 - Flags: approval-mozilla-release?
Attachment #8879420 - Flags: approval-mozilla-esr52?
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

We need this to bake more. It might be a bit risky for release. Release54-.
Attachment #8879420 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Same as comment #28. ESR52-.
Attachment #8879420 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
(In reply to Gerry Chang [:gchang] from comment #29)
> Same as comment #28. ESR52-.

(In reply to Gerry Chang [:gchang] from comment #28)
> We need this to bake more.

Does this mean it's still on the radar for a later ESR52 release or is this wontfix?
Flags: needinfo?(gchang)
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Let's target it on ESR52.4.
Flags: needinfo?(gchang)
Attachment #8879420 - Flags: approval-mozilla-esr52- → approval-mozilla-esr52?
Duplicate of this bug: 1363413
the patch doesn't apply to esr52:

grafting 406063:ee152ab292bb "Bug 1373222 - MutableBlobStorage always returns a temporary-file Blob if the size of data is greater than 1mb, r=smaug"
merging dom/base/MutableBlobStorage.cpp and dom/file/MutableBlobStorage.cpp to dom/base/MutableBlobStorage.cpp
merging dom/base/MutableBlobStorage.h and dom/file/MutableBlobStorage.h to dom/base/MutableBlobStorage.h
 warning: conflicts while merging dom/base/MutableBlobStorage.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Andrea, can you please attached a rebased patch for ESR52 when you get a chance?
Flags: needinfo?(amarchesini)
Attached patch esr52Splinter Review
Flags: needinfo?(amarchesini)
Comment on attachment 8901097 [details] [diff] [review]
esr52

fix oom crash in esr52
Attachment #8901097 - Flags: approval-mozilla-esr52+
Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

esr52 flag moved to rebased patch
Attachment #8879420 - Flags: approval-mozilla-esr52?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.