755323 - Attempts to upload a large file with DOM File or FileReader will now kill the browser

Status: RESOLVED INCOMPLETE

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

Description

[Boris Zbarsky [:bzbarsky]]

At least if I read bug 737164 right.

See the code touched in bug 604561 and various similar code in nsDOMFileReader (e.g. in nsDOMFileReader::DoOnDataAvailable).

Requesting tracking for the regression.

Comment 2

[Benjamin Smedberg]

I have a patch for the non-dataURI flavors. The data: URI flavor uses NS_AppendASCIItoUTF16 which is infallible, but the existing code doesn't deal with allocation failure correctly, returning an incomplete data URI (which seems worse than failing completely, I think). I'm not sure what to do about that case yet.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The right thing to do on OOM there is probably to throw.

Comment 4

[Benjamin Smedberg]

Attachment: Part 1, the easy bits of domfilereader, rev. 1

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 624726 [details] [diff] [review]
Part 1, the easy bits of domfilereader, rev. 1

Review of attachment 624726 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMFileReader.cpp
@@ +564,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  bool ok = aResult.SetLength(destLength);
> +  if (!ok)
> +    return NS_ERROR_OUT_OF_MEMORY;

Don't you need to pass fallible_t here too?

Also, shouldn't we really be using SetCapacity?

Finally, can we just write this as

if (!aResult.SetCapacity(destLength, fallible_t()) {
  return NS_ERROR_OUT_OF_MEMORY;
}

Comment 6

[Benjamin Smedberg]

Attachment: Part 1, rev. 1.1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 624779 [details] [diff] [review]
Part 1, rev. 1.1

r=me, but we should fix nsDOMFile too.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Benjamin Smedberg]

This isn't done yet.

Comment 10

[Alex Keybl [:akeybl]]

In email Benjamin suggested this bug remains open for edge case crashes, but shouldn't be a critical issue for release. Untracking given that.

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to Alex Keybl [:akeybl] from comment #10)
> In email Benjamin suggested this bug remains open for edge case crashes, but
> shouldn't be a critical issue for release. Untracking given that.

do we have any such crashes?

Firefox 21 has some sigs, all startup crashes, but I can't tell if they are a match to the bug because I don't know the stack to compare against. 
bp-a1ed5981-493a-4765-97dc-7aadf2130629 

3 stacks similar
bp-d6f3cd1e-0515-4ebd-b0d7-16d622130710
bp-0a199c63-9a07-408c-af56-3f8d32130710
bp-e32c8b35-6063-4d07-99f9-c23022130709

Comment 12

[Robert Kaiser]

I'm not sure what info you want from me here, so I can't provide it.

Comment 13

[Wayne Mery (:wsmwk)]

I suspect you could close this . Only one crash in two weeks and it's version 12. https://crash-stats.mozilla.com/search/?signature=~GetMutableData&product=Firefox&date=%3E%3D2016-11-30T11%3A49%3A44.000Z&date=%3C2016-12-14T11%3A49%3A44.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Comment 14

[Andrew Overholt [:overholt]]

(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #13)
> I suspect you could close this

Thanks!