Closed Bug 1097452 Opened 5 years ago Closed 5 years ago

Large OOM in nsTextEditorState::GetValue

Categories

(Core :: Editor, defect, critical)

34 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla38
Tracking Status
firefox34 --- affected
firefox35 --- affected

People

(Reporter: alex_mayorga, Assigned: ehsan)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-4647c0ec-e424-4835-b317-28e522141111.
=============================================================
Summary: crash in OOM | large | NS_ABORT_OOM(unsigned int) | nsTextEditorState::GetValue(nsAString_internal&, bool) → Large OOM in nsTextEditorState::GetValue
I got bp-bf084036-9989-4a63-8cb8-616602141126 while playing with the Customize mode of crash-stats super search. 76M OOM, I guess I got a lot of search results!
This is apparently still a thing on 35...

Report ID 	Date Submitted
bp-6d4a20bb-01bd-40d1-9c61-43b3f2141218	18/12/2014	10:36 a.m.

Is there anything worth collecting from the problematic system?
Flags: needinfo?(kairo)
(In reply to alex_mayorga from comment #3)
> Is there anything worth collecting from the problematic system?

I'm not an expert on text editor, but I don't think so.
Flags: needinfo?(kairo)
The stacks in comment 2 are incorrect. Here are the real stacks where we do huge allocations:

0042c57c 5d835b00 mozglue!je_malloc+0x2d
0042c590 5d80b96b xul!nsACString_internal::MutatePrep+0x60
0042c5bc 5d7975fe xul!AppendUTF16toUTF8+0x6b
0042c5d0 5d8cc4b3 xul!AppendUTF16toUTF8+0x11
0042c728 5d727541 xul!nsTextEditorState::SetValue+0x5c3
0042c7e0 5d6b5341 xul!mozilla::dom::HTMLTextAreaElement::SetValue+0x55
0042c888 5d909a35 xul!mozilla::dom::HTMLTextAreaElementBinding::set_value+0x4d
0042c8c4 616d2bc8 xul!mozilla::dom::GenericBindingSetter+0xb5
0042cad4 616d2eb2 mozjs!js::Invoke+0x188

0042c3b8 5d830560 mozglue!je_malloc+0x2d
0042c3d4 5d7f6079 xul!nsAString_internal::SetCapacity+0xe0
0042c400 5d7f3c0f xul!AppendUTF8toUTF16+0x49
0042c514 5d793dff xul!nsTextEditorState::GetValue+0x29f
0042c5d4 5d8cc425 xul!nsTextEditorState::UpdatePlaceholderVisibility+0x4f
0042c728 5d727541 xul!nsTextEditorState::SetValue+0x535
0042c7e0 5d6b5341 xul!mozilla::dom::HTMLTextAreaElement::SetValue+0x55
0042c888 5d909a35 xul!mozilla::dom::HTMLTextAreaElementBinding::set_value+0x4d
0042c8c4 616d2bc8 xul!mozilla::dom::GenericBindingSetter+0xb5
0042cad4 616d2eb2 mozjs!js::Invoke+0x188
Ehsan, how hard would it be to make these allocations fallible? I don't see any existing error paths in nsTextEditorState::SetValue, and this class looks pretty fragile!
Flags: needinfo?(ehsan)
(To expand on that: I am guessing there is a lot of state that would be in limbo if we got partway through a SetValue and OOM'ed)
We should make nsTextEditorState::SetValue fallible.  Its callers in HTMLInputElement and HTMLTextAreaElement are fallible so this should not be a big deal.
Assignee: nobody → ehsan
Component: Untriaged → Editor
Flags: needinfo?(ehsan)
Product: Firefox → Core
This patch handles most of the call sites for these allocations except
for a few where I added TODO comments with some information.  Handling
those places may require reworking lots of code, so I prefer to not do
that here.

Requesting review from Nathan on the XPCOM changes and jst on the rest.

David, do you mind testing this to see if it fixes the OOM crashes?
Attachment #8545567 - Flags: review?(nfroyd)
Attachment #8545567 - Flags: review?(jst)
Attachment #8545567 - Flags: feedback?(dmajor)
Comment on attachment 8545567 [details] [diff] [review]
Use fallible allocation when setting the value of an <input> or <textarea> element

Well, I no longer crash at the original site. Now I crash here:

0021e99c 58d9612e xul!NS_ABORT_OOM+0xa
0021e9ac 59a4afe2 xul!nsAString_internal::Assign+0x1d
0021e9c4 59a4bd9a xul!nsTextEditRules::TruncateInsertionIfNeeded+0x37
0021eac4 59a4ba46 xul!nsTextEditRules::WillInsertText+0x9c
0021eaf0 59a437ce xul!nsTextEditRules::WillDoAction+0x77
0021ebf4 59783948 xul!nsPlaintextEditor::InsertText+0xc6
0021ed24 597818a8 xul!nsTextEditorState::SetValue+0x2a3
0021ee58 597651cc xul!nsTextEditorState::PrepareEditor+0x552
0021ee60 59b94383 xul!mozilla::dom::HTMLTextAreaElement::CreateEditor+0xe
0021eeb8 59b95d6f xul!nsTextControlFrame::EnsureEditorInitialized+0xa2
0021eecc 59298e87 xul!nsTextControlFrame::EditorInitializer::Run+0x51
0021eee8 59b209da xul!nsContentUtils::RemoveScriptBlocker+0x54
0021ef88 59b20b56 xul!PresShell::FlushPendingNotifications+0x1a3
Attachment #8545567 - Flags: feedback?(dmajor)
(In reply to David Major [:dmajor] (UTC+13) from comment #10)
> Well, I no longer crash at the original site. Now I crash here:
> 
> 0021e99c 58d9612e xul!NS_ABORT_OOM+0xa
> 0021e9ac 59a4afe2 xul!nsAString_internal::Assign+0x1d
> 0021e9c4 59a4bd9a xul!nsTextEditRules::TruncateInsertionIfNeeded+0x37
> 0021eac4 59a4ba46 xul!nsTextEditRules::WillInsertText+0x9c
> 0021eaf0 59a437ce xul!nsTextEditRules::WillDoAction+0x77
> 0021ebf4 59783948 xul!nsPlaintextEditor::InsertText+0xc6
> 0021ed24 597818a8 xul!nsTextEditorState::SetValue+0x2a3
> 0021ee58 597651cc xul!nsTextEditorState::PrepareEditor+0x552
> 0021ee60 59b94383 xul!mozilla::dom::HTMLTextAreaElement::CreateEditor+0xe
> 0021eeb8 59b95d6f xul!nsTextControlFrame::EnsureEditorInitialized+0xa2
> 0021eecc 59298e87 xul!nsTextControlFrame::EditorInitializer::Run+0x51
> 0021eee8 59b209da xul!nsContentUtils::RemoveScriptBlocker+0x54
> 0021ef88 59b20b56 xul!PresShell::FlushPendingNotifications+0x1a3

Ah, yeah that's a separate issue.  Do you mind filing it please?  I'll write a fix for it.
Opened bug 1119099
Comment on attachment 8545567 [details] [diff] [review]
Use fallible allocation when setting the value of an <input> or <textarea> element

Review of attachment 8545567 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/string/nsReadableUtils.cpp
@@ +46,5 @@
>  void
>  CopyUTF16toUTF8(const nsAString& aSource, nsACString& aDest)
>  {
> +  if (!CopyUTF16toUTF8(aSource, aDest, mozilla::fallible_t())) {
> +    aDest.AllocFailed(aDest.Length() + aSource.Length());

*grumble* we need better error handling here for better crash report stats. :(

::: xpcom/string/nsTStringObsolete.cpp
@@ +475,5 @@
>  void
> +nsTString_CharT::ReplaceSubstring(const self_type& aTarget,
> +                                  const self_type& aNewValue)
> +{
> +  mozilla::unused << ReplaceSubstring(aTarget, aNewValue, fallible_t());

Shouldn't we be aborting here (or AllocFailed, or what have you)?  Obviously we don't have the right length to complain to AllocFailed with, but just ignoring the failure completely seems wrong.
Attachment #8545567 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #13)
> Comment on attachment 8545567 [details] [diff] [review]
> Use fallible allocation when setting the value of an <input> or <textarea>
> element
> 
> Review of attachment 8545567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/string/nsReadableUtils.cpp
> @@ +46,5 @@
> >  void
> >  CopyUTF16toUTF8(const nsAString& aSource, nsACString& aDest)
> >  {
> > +  if (!CopyUTF16toUTF8(aSource, aDest, mozilla::fallible_t())) {
> > +    aDest.AllocFailed(aDest.Length() + aSource.Length());
> 
> *grumble* we need better error handling here for better crash report stats.
> :(

What would that look like? I just copied what the code around here was already doing.

> ::: xpcom/string/nsTStringObsolete.cpp
> @@ +475,5 @@
> >  void
> > +nsTString_CharT::ReplaceSubstring(const self_type& aTarget,
> > +                                  const self_type& aNewValue)
> > +{
> > +  mozilla::unused << ReplaceSubstring(aTarget, aNewValue, fallible_t());
> 
> Shouldn't we be aborting here (or AllocFailed, or what have you)?  Obviously
> we don't have the right length to complain to AllocFailed with, but just
> ignoring the failure completely seems wrong.

Ah yes, my mistake.  I'll fix it, but I'd like to know what kind of error reporting you would like to see first.
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #13)
> > Comment on attachment 8545567 [details] [diff] [review]
> > Use fallible allocation when setting the value of an <input> or <textarea>
> > element
> > 
> > Review of attachment 8545567 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: xpcom/string/nsReadableUtils.cpp
> > @@ +46,5 @@
> > >  void
> > >  CopyUTF16toUTF8(const nsAString& aSource, nsACString& aDest)
> > >  {
> > > +  if (!CopyUTF16toUTF8(aSource, aDest, mozilla::fallible_t())) {
> > > +    aDest.AllocFailed(aDest.Length() + aSource.Length());
> > 
> > *grumble* we need better error handling here for better crash report stats.
> > :(
> 
> What would that look like? I just copied what the code around here was
> already doing.

I don't know that it's worth addressing here, I was just noting it.  See for instance the same sort of error here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#146

In both of these cases, we really would rather use the length of aSource if it were converted to UTF8, rather than the length of aSource as UTF16.

> > ::: xpcom/string/nsTStringObsolete.cpp
> > @@ +475,5 @@
> > >  void
> > > +nsTString_CharT::ReplaceSubstring(const self_type& aTarget,
> > > +                                  const self_type& aNewValue)
> > > +{
> > > +  mozilla::unused << ReplaceSubstring(aTarget, aNewValue, fallible_t());
> > 
> > Shouldn't we be aborting here (or AllocFailed, or what have you)?  Obviously
> > we don't have the right length to complain to AllocFailed with, but just
> > ignoring the failure completely seems wrong.
> 
> Ah yes, my mistake.  I'll fix it, but I'd like to know what kind of error
> reporting you would like to see first.

How about this:

  if (!ReplaceSubstring(aTarget, aNewValue, fallible_t())) {
    aTarget.AllocFailed(mLength + (aNewValue.Length() - aTarget.Length()));
    // Note that this may wildly underestimate the allocation that failed, as
    // we could have been replacing multiple copies of aTarget.
  }

?
Flags: needinfo?(nfroyd)
Comment on attachment 8545567 [details] [diff] [review]
Use fallible allocation when setting the value of an <input> or <textarea> element

r=jst. We should deal with the todo's etc, but I don't see those blocking this patch landing.
Attachment #8545567 - Flags: review?(jst) → review+
This patch handles most of the call sites for these allocations except
for a few where I added TODO comments with some information.  Handling
those places may require reworking lots of code, so I prefer to not do
that here.
Attachment #8548879 - Flags: review?(nfroyd)
This patch handles most of the call sites for these allocations except
for a few where I added TODO comments with some information.  Handling
those places may require reworking lots of code, so I prefer to not do
that here.
Attachment #8548889 - Flags: review?(nfroyd)
Attachment #8548879 - Attachment is obsolete: true
Attachment #8548879 - Flags: review?(nfroyd)
Attachment #8545567 - Attachment is obsolete: true
Comment on attachment 8548889 [details] [diff] [review]
Use fallible allocation when setting the value of an <input> or <textarea> element

Review of attachment 8548889 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/string/nsReadableUtils.cpp
@@ +46,5 @@
>  void
>  CopyUTF16toUTF8(const nsAString& aSource, nsACString& aDest)
>  {
> +  if (!CopyUTF16toUTF8(aSource, aDest, mozilla::fallible_t())) {
> +    // Note that this may wildly overestimate the allocation that failed, as

Wouldn't this be "underestimate"?  You might have a UTF16 string whose chars all require 2-3 bytes per, so the converted-to-UTF8 string length would be much larger?

@@ +157,5 @@
>  void
>  AppendUTF16toUTF8(const nsAString& aSource, nsACString& aDest)
>  {
>    if (!AppendUTF16toUTF8(aSource, aDest, mozilla::fallible_t())) {
> +    // Note that this may wildly overestimate the allocation that failed, as

Likewise here.
Attachment #8548889 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/27ae894ee4d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1119099
You need to log in before you can comment on or make changes to this bug.