1373222 - URL.createObjectURL crashes Firefox 54

Status: RESOLVED FIXED

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

Description

[Martin Vobruba]

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.

Comment 1

[Andrew Overholt [:overholt]]

Maybe baku knows what's changed here recently? Could be related to bug 1353629 or maybe e10s-multi?

Comment 2

[Loic]

martin, could you provide a testcase (on jsfiddle for example), it would help to narrow down a possible regression.

Comment 3

[Martin Vobruba]

I've tested it on this page: https://stat-info.cz/firefox.html

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Martin Vobruba]

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.

Comment 6

[Andrea Marchesini [:baku]]

It seems a OOM crash. Can you open about:crashes and give me the IDs of the crash reports? Thanks!

Comment 7

[Martin Vobruba]

Crashed browser
https://crash-stats.mozilla.com/report/index/e86d0ec0-e18e-449e-9fbd-2a05f0170616

Crashed tab
https://crash-stats.mozilla.com/report/index/e5b23a89-022e-4b69-840f-b7f661170616

Comment 8

[Andrea Marchesini [:baku]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:
Regression causing crashes. We might need the fix in a 54.x release.

Comment 10

[Martin Vobruba]

Wasn't MutableBlobStorage also present in earlier versions? Because we never saw these crashes in <=53.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: crash_url.patch

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Please explain the patch.

Comment 13

[Andrea Marchesini [:baku]]

(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.

Comment 14

[Andrea Marchesini [:baku]]

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 15

[Julien Cristau [:jcristau]]

tracking per comment 9.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: crash_url.patch

More comments.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

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

https://hg.mozilla.org/mozilla-central/rev/ee152ab292bb

Comment 21

[Gerry Chang [:gchang]]

Hi Martin,
Can you help check if this issue is fixed in the latest nightly?

Comment 22

[Martin Vobruba]

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!

Comment 23

[Andrea Marchesini [:baku]]

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

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

fix oom from blob storage on windows, beta55+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b54b431ba47c

Comment 26

[Ryan VanderMeulen [:RyanVM]]

If you think this is severe enough to warrant consideration for inclusion in a 54 dot release, please nominate it for release approval.

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Approval Request Comment: See comment 23.

Comment 28

[Gerry Chang [:gchang]]

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-.

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Same as comment #28. ESR52-.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

Let's target it on ESR52.4.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Andrea, can you please attached a rebased patch for ESR52 when you get a chance?

Comment 35

[Andrea Marchesini [:baku]]

Attachment: esr52

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 8901097 [details] [diff] [review]
esr52

fix oom crash in esr52

Comment 37

[Julien Cristau [:jcristau]]

Comment on attachment 8879420 [details] [diff] [review]
crash_url.patch

esr52 flag moved to rebased patch

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ecef71fa933f