Closed Bug 755323 Opened 9 years ago Closed 5 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED INCOMPLETE
mozilla15
Tracking Status
firefox15 - ---

People

(Reporter: bzbarsky, Unassigned)

References

Details

(Keywords: crash, Whiteboard: [leave open])

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Severity: normal → critical
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::GetMutableData(wchar_t**, unsigned int)]
Keywords: crash
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.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
The right thing to do on OOM there is probably to throw.
Attachment #624726 - Flags: review?(bzbarsky)
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;
}
Attached patch Part 1, rev. 1.1Splinter Review
Attachment #624726 - Attachment is obsolete: true
Attachment #624726 - Flags: review?(bzbarsky)
Attachment #624779 - Flags: review?(bzbarsky)
Comment on attachment 624779 [details] [diff] [review]
Part 1, rev. 1.1

r=me, but we should fix nsDOMFile too.
Attachment #624779 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/f9e1db25530b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
This isn't done yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
In email Benjamin suggested this bug remains open for edge case crashes, but shouldn't be a critical issue for release. Untracking given that.
Assignee: benjamin → nobody
(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
Flags: needinfo?(kairo)
Flags: needinfo?(akeybl)
I'm not sure what info you want from me here, so I can't provide it.
Flags: needinfo?(kairo)
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::GetMutableData(wchar_t**, unsigned int)] → [@ mozalloc_abort(char const* const) | NS_DebugBreak_P | nsAString_internal::GetMutableData(wchar_t**, unsigned int)] [@ mozalloc_abort | NS_DebugBreak_P | nsAString_internal::GetMutableData]
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #13)
> I suspect you could close this

Thanks!
Status: REOPENED → RESOLVED
Closed: 9 years ago5 years ago
Flags: needinfo?(overholt)
Resolution: --- → INCOMPLETE
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.