Closed Bug 1387903 Opened 8 years ago Closed 8 years ago

Crash in OOM | large | NS_ABORT_OOM | CopyASCIItoUTF16 | nsStructuredCloneContainer::GetDataAsBase64

Categories

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

54 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: n.nethercote, Assigned: alchen)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

This bug was filed from the Socorro interface and is report bp-352c99c2-6419-41c5-86c9-258810170805. ============================================================= This is an OOM large crash that should be easy to work around by using the fallible variant of CopyASCIItoUTF16().
Hi Alphan, :njn has already pointed out a solution suggestion, mind driving this to completion?
Flags: needinfo?(alchen)
I will take the bug.
Assignee: nobody → alchen
Flags: needinfo?(alchen)
Comment on attachment 8899748 [details] [diff] [review] Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash Review of attachment 8899748 [details] [diff] [review]: ----------------------------------------------------------------- This is good as far as it goes. But there are two callers of this method: Notification::InitFromJSVal() and Notification::InitFromBase64(). Neither of those check the return value. Maybe that's reasonable, but maybe it's not. I'm not sure. Somebody who knows nsStructuredCloneContainer might know.
Attachment #8899748 - Flags: review?(n.nethercote) → feedback+
Andrea, do you know? (See comment 4 and the patch.)
Flags: needinfo?(amarchesini)
Attachment #8899748 - Flags: review+
Flags: needinfo?(amarchesini)
> This is good as far as it goes. But there are two callers of this method: > Notification::InitFromJSVal() and Notification::InitFromBase64(). Neither of > those check the return value. Maybe that's reasonable, but maybe it's not. > I'm not sure. Somebody who knows nsStructuredCloneContainer might know. Can you file a bug for this?
> Can you file a bug for this? I'd prefer to deal with it in this bug. Otherwise we're converting a predictable OOM crash with who knows what kind of potentially erroneous behaviour.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Keywords: checkin-needed
does this make sense? Is there anything we can do here?
Attachment #8901714 - Flags: review?(amarchesini)
Comment on attachment 8901714 [details] [diff] [review] Take the result of GetDataAsBase64() Review of attachment 8901714 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/notification/Notification.cpp @@ +2324,5 @@ > if (NS_WARN_IF(aRv.Failed())) { > return; > } > > + aRv = dataObjectContainer->GetDataAsBase64(mDataAsBase64); if (NS_WARN_IF(aRv.Failed())) { return; } I want to see an warning message when this happens. @@ +2341,5 @@ > if (NS_WARN_IF(aRv.Failed())) { > return; > } > > + aRv = container->GetDataAsBase64(mDataAsBase64); same here.
Attachment #8901714 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e90e17ca8d42 Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash. r=baku
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: