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

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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().

Comment 1

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Hi Alphan, :njn has already pointed out a solution suggestion, mind driving this to completion?

Comment 2

[Alphan Chen [:alchen]]

 I will take the bug.

Comment 3

[Alphan Chen [:alchen]]

Attachment: Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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.

Comment 5

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Andrea, do you know? (See comment 4 and the patch.)

Comment 6

[Andrea Marchesini [:baku]]

> 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?

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> 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.

Comment 8

[Alphan Chen [:alchen]]

Attachment: Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash. r=baku

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=050cfa6e449435475ec3bd0cd3b4eacf107a566a

Comment 9

[Alphan Chen [:alchen]]

Attachment: Take the result of GetDataAsBase64()

does this make sense?
Is there anything we can do here?

Comment 10

[Andrea Marchesini [:baku]]

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.

Comment 11

[Alphan Chen [:alchen]]

Attachment: Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash. r=baku

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e43ebde85253324f4ff20e82ebfec9a2a6e8beb3&selectedJob=126666328

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90e17ca8d42
Use the fallible variant of CopyASCIItoUTF16 to avoid OOM large crash. r=baku

Comment 13

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

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