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.

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

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

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

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.

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.

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

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

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.

Please explain the patch.

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

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.

More comments.

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.

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

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

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 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 on attachment 8879420 [details] [diff] [review] crash_url.patch fix oom from blob storage on windows, beta55+

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

Comment on attachment 8879420 [details] [diff] [review] crash_url.patch Approval Request Comment: See comment 23.

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 on attachment 8879420 [details] [diff] [review] crash_url.patch Same as comment #28. 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?

Comment on attachment 8879420 [details] [diff] [review] crash_url.patch Let's target it on ESR52.4.

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?

Comment on attachment 8901097 [details] [diff] [review] esr52 fix oom crash in esr52

Comment on attachment 8879420 [details] [diff] [review] crash_url.patch esr52 flag moved to rebased patch