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)
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)
2.42 KB,
patch
|
alchen
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Hi Alphan, :njn has already pointed out a solution suggestion, mind driving this to completion?
Flags: needinfo?(alchen)
Assignee | ||
Comment 2•8 years ago
|
||
I will take the bug.
Assignee: nobody → alchen
Flags: needinfo?(alchen)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8899748 -
Flags: review?(n.nethercote)
![]() |
Reporter | |
Comment 4•8 years ago
|
||
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+
![]() |
Reporter | |
Comment 5•8 years ago
|
||
Andrea, do you know? (See comment 4 and the patch.)
Flags: needinfo?(amarchesini)
Updated•8 years ago
|
Attachment #8899748 -
Flags: review+
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Comment 6•8 years ago
|
||
> 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?
![]() |
Reporter | |
Comment 7•8 years ago
|
||
> 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.
Assignee | ||
Comment 8•8 years ago
|
||
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=050cfa6e449435475ec3bd0cd3b4eacf107a566a
Attachment #8899748 -
Attachment is obsolete: true
Attachment #8901708 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•8 years ago
|
||
does this make sense?
Is there anything we can do here?
Attachment #8901714 -
Flags: review?(amarchesini)
Comment 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8901708 -
Attachment is obsolete: true
Attachment #8901714 -
Attachment is obsolete: true
Attachment #8902500 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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
![]() |
||
Comment 13•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•8 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•