Closed Bug 950076 Opened 10 years ago Closed 10 years ago

Use fallible allocator in nsTextFragment::AppendTo

Categories

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

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: benjamin, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(3 files, 5 obsolete files)

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)
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1]
Crash Signature: [@ NS_ABORT_OOM(unsigned int) | AppendASCIItoUTF16(nsACString_internal const&, nsAString_internal&) ]
Also see bp-3f8b4233-3fd4-456a-8d5a-996f62131213 which is from mozInlineSpellWordUtil::BuildSoftText.
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..
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..
(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..
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.
I would like to work on this bug. How would you suggest I get started?
Is that ok for me to work on this bug or is somebody else working on it?
Flags: needinfo?(benjamin)
Go for it.
Assignee: nobody → alessio.placitelli
Flags: needinfo?(benjamin)
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)
#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)
Attached 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)
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)
Attachment #8377280 - Flags: review?(benjamin)
Attached 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)
Attached patch bug950076.patch (obsolete) — Splinter Review
Attachment #8379833 - Attachment is obsolete: true
Attachment #8379833 - Flags: review?(jst)
Attachment #8379837 - Flags: review?(jst)
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)
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+
Attached patch bug950076.patchSplinter Review
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)
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!
(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.
Attached patch bug950076_errorPropagation.patch (obsolete) — Splinter Review
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+
Attached patch bug950076_errorPropagation.patch (obsolete) — Splinter Review
Thanks :jst for your review, I've followed your suggestions and updated the patch file ;)
Attachment #8388191 - Attachment is obsolete: true
Keywords: checkin-needed
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)
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)
Depends on: 1037214
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)
Depends on: 1121206
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.

Attachment

General

Created:
Updated:
Size: