Use fallible allocator in nsTextFragment::AppendTo

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: benjamin, Assigned: Dexter)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla31
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1], crash signature)

Attachments

(3 attachments, 5 obsolete attachments)

Reporter

Description

6 years ago
I've been going through crashes at NS_ABORT_OOM(unsigned int) | AppendASCIItoUTF16(nsACString_internal const&, nsAString_internal&) and found a bunch of them are from nsTextFragment::AppendTo which is often called from 

AppendNodeTextContentsRecurse (nsContentUtils.cpp)
nsContentUtils::AppendNodeTextContent(nsINode *,bool,nsAString_internal &) (nsContentUtils.cpp)

Crash reports:
bp-467b1183-61f8-4544-a665-433b52131212
bp-09c40f45-fb4e-4415-bbe8-6ccc12131209
bp-ac268399-d610-4286-b67c-b8ff02131208
bp-64612baf-71c3-40ad-8ef9-974342131209

The unfortunate thing about this is that it may involve changing the signature of a bunch of methods which are currently assumed to be infallible, including:

nsIContent::AppendTextTo
nsContentUtils::AppendNodeTextContent

There may be cases where we can't propagate errors usefully, so we may need to end up with both a fallible and infallible version of some of these methods. The typical pattern for that is to have two methods, one of which takes fallible_t as a parameter:

void Method(...); // infallible, crashes on failure
bool Method(..., const fallible_t&) NS_WARN_UNUSED_RESULT; // FALLIBLE, CALLER MUST CHECK BOOL RESULT CODE

See e.g. http://hg.mozilla.org/mozilla-central/annotate/8b5875dc7e31/xpcom/string/public/nsTSubstring.h#l356

peterv, do you have any thoughts about how far the OOM errors should propagate or what else this might affect?
Flags: needinfo?(peterv)
Reporter

Updated

6 years ago
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1]
Reporter

Updated

6 years ago
Crash Signature: [@ NS_ABORT_OOM(unsigned int) | AppendASCIItoUTF16(nsACString_internal const&, nsAString_internal&) ]
Reporter

Comment 1

6 years ago
Also see bp-3f8b4233-3fd4-456a-8d5a-996f62131213 which is from mozInlineSpellWordUtil::BuildSoftText.

Comment 2

6 years ago
Hi I am new to all this.
Wanted to start contributing.. could you please help..??
I have been in C++ from the past 2 years..
Assignee

Comment 3

6 years ago
I would suggest you to start by reading this ( https://developer.mozilla.org/en-US/docs/Introduction ) and by joining the #introduction channel on irc.mozilla.org.

(In reply to 291.khwaja from comment #2)
> Hi I am new to all this.
> Wanted to start contributing.. could you please help..??
> I have been in C++ from the past 2 years..

Comment 4

6 years ago
(In reply to Alessio Placitelli from comment #3)
> I would suggest you to start by reading this (
> https://developer.mozilla.org/en-US/docs/Introduction ) and by joining the
> #introduction channel on irc.mozilla.org.

I've joined he channel.. and i read the link you mentioned.. that brought here..
Reporter

Comment 5

6 years ago
Hello khwaja. Feel free to dive in given the information I provided in comment 0, or ask more specific questions. You probably won't be able to tell whether you can just make nsTextFragment::AppendTo infallible in all cases and propagate errors across the system without trying it and seeing what breaks, but it's probably going to be a pretty big patch if you start.

So it might make more sense to start by adding a separate fallible version, starting using that in the specific cases we're worried about here (AppendNodeTextContentsRecurse) and just make that set of functions fallible.

Comment 6

6 years ago
I would like to work on this bug. How would you suggest I get started?
Assignee

Comment 7

5 years ago
Is that ok for me to work on this bug or is somebody else working on it?
Flags: needinfo?(benjamin)
Reporter

Comment 8

5 years ago
Go for it.
Assignee: nobody → alessio.placitelli
Flags: needinfo?(benjamin)
Assignee

Comment 9

5 years ago
Thanks! I've noticed that some functions that have const mozilla::fallible_t& in the signature as an argument (including a function I've created in a previous bug), don't use it in the function's code.

See http://hg.mozilla.org/releases/mozilla-aurora/annotate/0ff7a838c4be/xpcom/string/src/nsReadableUtils.cpp#l166 (arguments) and http://hg.mozilla.org/releases/mozilla-aurora/annotate/0ff7a838c4be/xpcom/string/src/nsReadableUtils.cpp#l181 (usage). 

How should it be used, #1 or #2?

bool AppendUTF8toUTF16( const nsACString& aSource, nsAString& aDest, const mozilla::fallible_t& aFallible) {

// USING THE ARGUMENT (#1)
if (!aDest.SetLength(old_dest_length + count, aFallible)) {

// INSTEAD OF (#2 - Current way)
if (!aDest.SetLength(old_dest_length + count, mozilla::fallible_t())) {
}
Flags: needinfo?(benjamin)
Reporter

Comment 10

5 years ago
#2. The argument carries meaning only by its presence. And in fact the argument shouldn't be named:

bool AppendUTF8toUTF16( const nsACString& aSource, nsAString& aDest, const mozilla::fallible_t&)
Flags: needinfo?(benjamin)
Assignee

Comment 11

5 years ago
Posted patch bug950076.patch (obsolete) — Splinter Review
I've got some doubts about this one.

1. I'm not quite sure how should I deal with the fallible AppendTo here: http://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp#576

2. nsTextFragment::AppendTo uses nsAString::Append which is supposed to set internal buffer to 0-length on OOM ( https://developer.mozilla.org/en-US/docs/NsAString/Append ). Is this true?

3. How far should I propagate the OOM condition? See http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#6543
Attachment #8377280 - Flags: review?(benjamin)
Reporter

Comment 12

5 years ago
I don't actually know the answers to most of your questions, nor am I the right reviewer for this. I will say:

* spellchecking should probably just completely bail if it hits an error condition like this. Since this method doesn't have a return code, probably just break;

* Append() should be an infallible method, but reading the code I'm not sure it actually is. Those docs are ancient and damn MDN to a thousand deaths for insisting on doing mozilla internals docs when it's just going to be so poor. The method forwards to Replace() which currently returns void and should be infallible: http://hg.mozilla.org/mozilla-central/annotate/c4630e39ad6b/xpcom/string/src/nsTSubstring.cpp#l488 

But I think that Replace() isn't actually infallible because it's calling the fallible ReplacePrep() but not aborting if that fails (just checking mLength which is silly). So I'll happily take a patch to add a fallible form of Replace() and Append().

I don't know about propagation. A dom peer needs to answer that question.
Flags: needinfo?(bugs)
Reporter

Updated

5 years ago
Attachment #8377280 - Flags: review?(benjamin)
Assignee

Comment 13

5 years ago
Posted patch bug950076.patch (obsolete) — Splinter Review
Thank you for your tips. I've added a fallible version for Append and Replace.

Also run the tests on the tryserv here: https://tbpl.mozilla.org/?tree=Try&rev=4a34a50fba92

I can't understand why the Android 2.2 Debug and the B2G ICS Emulator Debug fail. Any clue?
Attachment #8377280 - Attachment is obsolete: true
Attachment #8379833 - Flags: review?(jst)
Flags: needinfo?(benjamin)
Assignee

Comment 14

5 years ago
Posted patch bug950076.patch (obsolete) — Splinter Review
Attachment #8379833 - Attachment is obsolete: true
Attachment #8379833 - Flags: review?(jst)
Attachment #8379837 - Flags: review?(jst)
Reporter

Updated

5 years ago
Duplicate of this bug: 975471
Reporter

Comment 16

5 years ago
Comment on attachment 8379837 [details] [diff] [review]
bug950076.patch

Can you fix the implementations of nsTSubstring_CharT::Replace which look infallible but currently aren't?

e.g. in this method

 nsTSubstring_CharT::Replace( index_type cutStart, size_type cutLength, char_type c )
   {
     cutStart = XPCOM_MIN(cutStart, Length());
 
     if (ReplacePrep(cutStart, cutLength, 1))
       mData[cutStart] = c;
   }

it shold probably switch around to

  if (!ReplacePrep(cutStart, cutLength, 1)) {
    NS_ABORT_OOM(Length() - cutLength + 1);
  }

It probably makes sense instead of copying the implementation of nsTSubstring_CharT::Replace( index_type cutStart, size_type cutLength, const char_type* data, size_type length to have the *infallible* form just forward to the fallible form and abort on failure.
Flags: needinfo?(benjamin)
Reporter

Comment 17

5 years ago
Oh, and I think you can ignore the android/B2G failures here; they look like intermittent/machine issues and don't indicate a real problem.
Comment on attachment 8379837 [details] [diff] [review]
bug950076.patch

The DOM pieces here look good to me! There's more we can do here though, i.e. we should identify where these OOM crashes come from. I would imagine that the .innerHTML .textContent getters are likely ones, but probably better to split that into separate bugs (at least separate patches). r=jst for the DOM changes here.
Attachment #8379837 - Flags: review?(jst) → review+
Assignee

Comment 19

5 years ago
Benjamin, this includes your suggestions. As for the improvements suggested by :jst, I would gladly take care of them. Should I open the new bugs?
Attachment #8379837 - Attachment is obsolete: true
Flags: needinfo?(jst)
Assignee

Comment 20

5 years ago
I've followed :jst advices and propagated the OOM check back to .innerHTML by modifying the GetInnerHTML calls. I've got a few doubts on this one as well:

1. Does it make sense to change the signature of nsContentUtils::GetNodeTextContent (http://dxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#1242) to return a bool instead of creating a duplicate function with a different signature?

2. Enforcing NS_WARN_UNUSED_RESULT on nsContentUtils::GetNodeTextContent makes the build fail on OSX and B2G, because the result of GetNodeTextContent here (http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.cpp#358) is not handled. I'm not quite sure how to handle this situation (as well as the one http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.cpp#510 ).
Attachment #8384140 - Flags: review?(jst)
Flags: needinfo?(jst)
Comment on attachment 8384140 [details] [diff] [review]
bug950076_innerHTML.patch

Looks good!
Attachment #8384140 - Flags: review?(jst) → review+
(In reply to Alessio Placitelli from comment #20)
> Created attachment 8384140 [details] [diff] [review]
> bug950076_innerHTML.patch
> 
> I've followed :jst advices and propagated the OOM check back to .innerHTML
> by modifying the GetInnerHTML calls. I've got a few doubts on this one as
> well:

Excellent, thank you!

> 1. Does it make sense to change the signature of
> nsContentUtils::GetNodeTextContent
> (http://dxr.mozilla.org/mozilla-central/source/content/base/public/
> nsContentUtils.h#1242) to return a bool instead of creating a duplicate
> function with a different signature?

I believe you made the right choice here, duplicating that code just for the sake of a different return type would cause more maintenance problem down the road than what we would win from having two versions.

> 2. Enforcing NS_WARN_UNUSED_RESULT on nsContentUtils::GetNodeTextContent
> makes the build fail on OSX and B2G, because the result of
> GetNodeTextContent here
> (http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.
> cpp#358) is not handled. I'm not quite sure how to handle this situation (as
> well as the one
> http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.
> cpp#510 ).

I think in that case I would simply abort the process if it fails. Not much we can do, and I'd rather abort than leave stuff in a likely broken state.

Thanks again for all the work here!
Assignee

Comment 23

5 years ago
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #22)
> I think in that case I would simply abort the process if it fails. Not much
> we can do, and I'd rather abort than leave stuff in a likely broken state.
>
> Thanks again for all the work here!

Thank you for your review and time as well! I'm working on the .textContent patch right now.

When you say abort the process do you mean exiting from the function or something like NS_ABORT_OOM(size)? (i.e. http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.cpp#510 )
Flags: needinfo?(jst)
(In reply to Alessio Placitelli from comment #23)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #22)
> > I think in that case I would simply abort the process if it fails. Not much
> > we can do, and I'd rather abort than leave stuff in a likely broken state.
> >
> > Thanks again for all the work here!
> 
> Thank you for your review and time as well! I'm working on the .textContent
> patch right now.
> 
> When you say abort the process do you mean exiting from the function or
> something like NS_ABORT_OOM(size)? (i.e.
> http://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeBinding.
> cpp#510 )

The latter, NS_ABORT_OOM() please. But you probably don't have a reasonable size to give there, so plain NS_ABORT() is probably better. Thanks!
Flags: needinfo?(jst)
You want NS_RUNTIMEABORT().  NS_ABORT() doesn't do anything in non-debug builds.
Assignee

Comment 26

5 years ago
This is the 3rd (and hopefully last) part of the proposed fix. In this patch wherever nsContentUtils::GetNodeTextContent is used, its return value is checked. If an OOM condition is detected, either NS_RUNTIMEABORT() (thanks Andrew) is called or NS_ERROR_OUT_OF_MEMORY is returned.

Everything seems to be fine on the tryserv: https://tbpl.mozilla.org/?tree=Try&rev=1c0af0af483b
Attachment #8388191 - Flags: review?(jst)
Comment on attachment 8388191 [details] [diff] [review]
bug950076_errorPropagation.patch

Looks great! One minor comment:

- In FragmentOrElement::GetTextContentInternal(nsAString& aTextContent):

-  nsContentUtils::GetNodeTextContent(this, true, aTextContent);
+  bool gotContent = nsContentUtils::GetNodeTextContent(this, true, aTextContent);
+  if (!gotContent)

I'd say loose the gotContent variable here, just move the GetNodeTextContent() call into the if statement. Same thing in nsDocument::GetTitleFromElement().

r=jst with that. Thanks!
Attachment #8388191 - Flags: review?(jst) → review+
Assignee

Comment 28

5 years ago
Thanks :jst for your review, I've followed your suggestions and updated the patch file ;)
Attachment #8388191 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 31

5 years ago
That's odd, the code the compilation is complaining about is inside the  bug950076_errorPropagation.patch (at the end of the file).

This is the patch file I've attached: https://bug950076.bugzilla.mozilla.org/attachment.cgi?id=8393359

This is the raw difference from your checkin: https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/a17ee88f1fe7

It is basically missing the last piece of the patch! What can I do?
Flags: needinfo?(ryanvm)
hrm, there was another MathML patch in that push that originally got the blame. I wonder if this hit conflicts that I missed. Can you please rebase that patch against m-c tip and re-attach?
Flags: needinfo?(ryanvm)
Assignee

Comment 33

5 years ago
Just the one that gave you problems, right?
Attachment #8393359 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Yes, thanks :)
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
I am going to cancel the needinfo requests for Peter and Olli now that this is FIXED.
Flags: needinfo?(peterv)
Flags: needinfo?(bugs)
Reporter

Updated

5 years ago
Duplicate of this bug: 1027168

Comment 39

5 years ago
Is it really fixed?

Just got this on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150113030205 CSet: 3d846527576f

Report ID 	Date Submitted
bp-1dfb9129-2f31-4a8f-8dd6-85ae02150113	1/13/2015	2:26 PM
Flags: needinfo?(benjamin)

Updated

5 years ago
Depends on: 1121206
Reporter

Comment 40

5 years ago
I see you filed 1121206.
Flags: needinfo?(benjamin)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.