847347 - Firefox crashes on FileReader readAsDataURL of large file (>100mb)

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P3)

Description

[Cian Kinsella]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130215130331

Steps to reproduce:

Coded Javascript as below (slightly simplified).   Selected a large file (128mb) from Firefox on Windows 7, 4GB machine.

function readFileAsBinary(file)
{
	var fileSize = file.size || 0;
	var reader = new FileReader();
	reader.onerror = errorHandler;
    reader.onabort = function(e) {
      alert('File read cancelled');
    };
    reader.onloadstart = function(e) 
	{
		if (fileSize > 1000000)
		{
	      // display wait message?
		}
   };

	reader.onload = function(event) 
	{
		alert("finished");
	};
	function errorHandler(evt)
	{
		var errorName = "Unknown";
		if (evt.target.error.name)
		{
			errorName = evt.target.error.name;
		}
		var errorMessage = 'Problem while reading ' + file.name + " (" +  fileSize + " bytes): " + errorName;
		alert(errorMessage);
	}

	//reader.readAsBinaryString(file);			// Replace with DataUrl because binarystring has been deprecated
	reader.readAsDataURL(file);
}




Actual results:

Browser crashes.  Neither the onerror or onload events have fired.


Expected results:

onerror if there is some violation of maximum limit or memory problem
onload otherwise

Comment 1

[Loic]

Provide a minimal testcase (.html) with your JS code, please.

In addition, if Firefox crashed, type about:crashes and provide crash IDs (bp-...).

Comment 2

[Cian Kinsella]

Attachment: Test case

Web page that illustrates the problem.  This causes a crash (at least with me) the submitted crash id is 1f2b325b-050b-4f3c-8c07-b35482130304

Comment 3

[Scoobidiver (away)]

Here is the link to the crash report: bp-1f2b325b-050b-4f3c-8c07-b35482130304.

Loading a file of more than 300 MB doesn't crash Firefox for me.

Does it happen in Safe Mode (see https://support.mozilla.org/kb/troubleshoot-firefox-issues-using-safe-mode)?

Comment 4

[Cian Kinsella]

Scoobidiver, thanks for prompt attention. That's interesting that it worked OK for you.  I tried again this time in Safe Mode and it crashed again, the link to the safe-mode crash is http://crash-stats.mozilla.com/report/index/bp-78025056-bdef-4540-85a2-ce90b2130304

Comment 5

[Cian Kinsella]

Just tried this on another computer, a Windows XP machine with only 1GB of RAM.  It also crashed on a large file (70+mb).  The crash link for that event is http://crash-stats.mozilla.com/report/index/bp-585db802-ea15-425f-94ea-533072130304

Comment 6

[Boris Zbarsky [:bzbarsky]]

DOMFileReader::GetAsDataURL does:

  AppendASCIItoUTF16(nsDependentCString(base64), aResult);

Apparently someone made this infallible, so on OOM it just kills the process.  What's needed to make this be a fallible call?

Comment 7

[Scoobidiver (away)]

I meant previously it doesn't crash in Nightly.
It crashes in 19.0 but not in 20.0 Beta.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I see no reason this wouldn't still crash in nightly with large enough file data....

Comment 9

[Benjamin Smedberg]

Make a new version of http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/xpcom/string/src/nsReadableUtils.cpp

with the signature

bool
AppendASCIItoUTF16(const nsACString& aSource, nsAString& aDest, const fallible_t& );

And you'll need to propagate the fallible-ness back to SetLengthForWriting (which currently calls to the infallible version of GetMutableData and is therefore a silly function).

Comment 10

[Jens Stutte [:jstutte]]

Trying with current nightly the test still crashes (with a 2.4 GB file)

Apparently we have an OOM error treatment here:

  if (!AppendASCIItoUTF16(encodedData, aResult, fallible)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  return NS_OK;

that does not work well such that we encounter this MOZ_RELEASE_ASSERT. Unfortunately the code of AppendASCIItoUTF16 seems to be inlined, the next step would be to try the same with a debug build.

Comment 11

[Tom S. (please needinfo tschuster)]

I suspect we are crashing at the Substring in Base64Encode(Substring(aFileData, aDataLen), encodedData) beforehand.

Comment 12

[Jens Stutte [:jstutte]]

Ah, that might have been inlined here as well, thanks. There inside I read:

  if (aBinary.Length() > (UINT32_MAX / 4) * 3) {
    return NS_ERROR_FAILURE;
  }

which is indeed > 2.4 GB and might differ from kMaxCapacity which I assume is < 2.4 GB (probably 2 GB minus some bytes?).

This would indicate that files of size < aprox. 2 GB can work (if we have enough memory), such that I would not be to worried about impact here (probably kMaxCapacity once was much smaller? The current boundary formula dates from 2017 according to bug 1349719). Still the error handling and different max. constants in use should be improved here, IMHO.

Comment 13

[Jens Stutte [:jstutte]]

Running the crash in the debugger confirms the assumption from comment 11.

Though Substring just wraps the already allocated memory here it still checks the length against its kMaxCapacity member, which is significantly lower (aprox. 1GB) - and issues a MOZ_RELEASE_ASSERT(CheckCapacity(aLength), "String is too large.");. Though I can understand the rationale behind assuming string allocations to not require explicit OOM error handling, in this case it would be advisable to have it.

A naive approach could be to hand-check the allowed capacity like

  // CheckCapacity checks, if the data can fit into a nsTSubstring
  if (!nsTSubstring<char>::CheckCapacity(aDataLen)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
  auto tmp = Substring(aFileData, aDataLen);
  nsCString encodedData;
  nsresult rv = Base64Encode(tmp, encodedData);
  NS_ENSURE_SUCCESS(rv, rv);

but nsTSubstring<char>::CheckCapacity(aDataLen) is a protected member of nsTSubstring. Probably a cleaner approach would be to add a public static function that checks capacity before constructing the substring object?

Comment 14

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] (REO for FF 81) from comment #12)

This would indicate that files of size < aprox. 2 GB can work (if we have enough memory)

So looking at the value of kMaxCapacity, the debugger tells me indeed that it's 2147483637, which is "aprox. 2 GB". The rationale behind this is probably to be safe against signed/unsigned conversions of 32Bit length values. The hard limit for length would be uint_32, it seems.

I see two options here:

1. Define an own (lower) const uint32_t kMaxFileAsDataURLLength = 2147400000; and check it beforehand. This is probably safe, as it is very unlikely, that future modifications of nsTSubstring will reduce kMaxCapacity significantly or that uint32_t will ever have more/less than 32 bit (still a constant reads kind of bad here).

2. Add a factory function with error handling and use it here (and who knows where else). This is probably only a good idea, if we expect more places in our codebase where we could hit this limit through normal operations.

If I try 1), I get an alert "Problem while reading ubuntu-19.10-desktop-amd64(1).iso (2463842304 bytes): NotReadableError", which is probably an improvable error message for this case, but the crash goes away.

Comment 15

[Jens Stutte [:jstutte]]

Attachment: Bug 847347 - Add a check for kMaxFileAsDataURLLength

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 847347 - Ensure that FileReader::GetAsDataURL does not trigger a release-mode assertion failure on large files. r=jstutte,#xpcom-reviewers

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24e0d6508e03
Ensure that FileReader::GetAsDataURL does not trigger a release-mode assertion failure on large files. r=jstutte,froydnj

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/24e0d6508e03