URL.createObjectURL crashes Firefox 54

RESOLVED FIXED in Firefox -esr52

Status

()

P3
normal
RESOLVED FIXED
2 years ago
7 days ago

People

(Reporter: vobruba.martin, Assigned: baku)

Tracking

({crash})

54 Branch
mozilla56
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5256+ fixed, firefox54+ wontfix, firefox55+ fixed, firefox56+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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)

Comment 2

2 years ago
martin, could you provide a testcase (on jsfiddle for example), it would help to narrow down a possible regression.
Flags: needinfo?(vobruba.martin)
(Reporter)

Comment 3

2 years ago
I've tested it on this page: https://stat-info.cz/firefox.html
Flags: needinfo?(vobruba.martin)
(Assignee)

Comment 4

2 years ago
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)
(Reporter)

Comment 5

2 years ago
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)
(Assignee)

Comment 6

2 years ago
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
(Assignee)

Comment 8

2 years ago
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.
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
tracking-firefox57: --- → ?
(Reporter)

Comment 10

2 years ago
Wasn't MutableBlobStorage also present in earlier versions? Because we never saw these crashes in <=53.
(Assignee)

Comment 11

2 years ago
Posted patch crash_url.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8879074 - Flags: review?(bugs)
Please explain the patch.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 13

2 years ago
(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)
(Assignee)

Comment 14

2 years ago
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.
tracking per comment 9.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
tracking-firefox56: ? → +
tracking-firefox57: ? → ---
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-
(Assignee)

Comment 17

2 years ago
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+

Comment 19

2 years ago
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

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee152ab292bb
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Hi Martin,
Can you help check if this issue is fixed in the latest nightly?
Flags: needinfo?(vobruba.martin)
(Reporter)

Comment 22

2 years ago
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)
(Assignee)

Comment 23

2 years ago
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+

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b54b431ba47c
status-firefox55: affected → fixed
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
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → ?
Flags: needinfo?(amarchesini)
(Assignee)

Comment 27

2 years ago
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-
status-firefox54: affected → wontfix
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?
tracking-firefox-esr52: ? → 56+
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 35

2 years ago
Posted 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?

Comment 38

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/ecef71fa933f
status-firefox-esr52: affected → fixed
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.