Closed Bug 1387903 Opened 3 years ago Closed 3 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: njn, 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
https://hg.mozilla.org/mozilla-central/rev/e90e17ca8d42
Status: ASSIGNED → RESOLVED
Closed: 3 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.