Closed
Bug 950076
Opened 10 years ago
Closed 10 years ago
Use fallible allocator in nsTextFragment::AppendTo
Categories
(Core :: DOM: Core & HTML, defect)
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)
19.44 KB,
patch
|
Details | Diff | Splinter Review | |
4.66 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
15.05 KB,
patch
|
Details | Diff | Splinter Review |
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•10 years ago
|
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][crashkill:P1]
Reporter | ||
Updated•10 years ago
|
Crash Signature: [@ NS_ABORT_OOM(unsigned int) | AppendASCIItoUTF16(nsACString_internal const&, nsAString_internal&) ]
Reporter | ||
Comment 1•10 years ago
|
||
Also see bp-3f8b4233-3fd4-456a-8d5a-996f62131213 which is from mozInlineSpellWordUtil::BuildSoftText.
Comment 2•10 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•10 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•10 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•10 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•10 years ago
|
||
I would like to work on this bug. How would you suggest I get started?
Assignee | ||
Comment 7•10 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•10 years ago
|
||
Go for it.
Assignee: nobody → alessio.placitelli
Flags: needinfo?(benjamin)
Assignee | ||
Comment 9•10 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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
Attachment #8377280 -
Flags: review?(benjamin)
Assignee | ||
Comment 13•10 years ago
|
||
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•10 years ago
|
||
Attachment #8379833 -
Attachment is obsolete: true
Attachment #8379833 -
Flags: review?(jst)
Attachment #8379837 -
Flags: review?(jst)
Reporter | ||
Comment 16•10 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•10 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 18•10 years ago
|
||
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•10 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•10 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 21•10 years ago
|
||
Comment on attachment 8384140 [details] [diff] [review] bug950076_innerHTML.patch Looks good!
Attachment #8384140 -
Flags: review?(jst) → review+
Comment 22•10 years ago
|
||
(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•10 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)
Comment 24•10 years ago
|
||
(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)
Comment 25•10 years ago
|
||
You want NS_RUNTIMEABORT(). NS_ABORT() doesn't do anything in non-debug builds.
Assignee | ||
Comment 26•10 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 27•10 years ago
|
||
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•10 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•10 years ago
|
Keywords: checkin-needed
Comment 29•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e5ed676919 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3aee4a9650 https://hg.mozilla.org/integration/mozilla-inbound/rev/a17ee88f1fe7
Keywords: checkin-needed
Comment 30•10 years ago
|
||
Backed out for Werror bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/f4b09373b94c https://tbpl.mozilla.org/php/getParsedLog.php?id=36402393&tree=Mozilla-Inbound
Assignee | ||
Comment 31•10 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)
Comment 32•10 years ago
|
||
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•10 years ago
|
||
Just the one that gave you problems, right?
Attachment #8393359 -
Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Comment 35•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc71a41bee5 https://hg.mozilla.org/integration/mozilla-inbound/rev/b7da9111c2e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b55547f3b36
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9fc71a41bee5 https://hg.mozilla.org/mozilla-central/rev/b7da9111c2e9 https://hg.mozilla.org/mozilla-central/rev/4b55547f3b36
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 37•10 years ago
|
||
I am going to cancel the needinfo requests for Peter and Olli now that this is FIXED.
Flags: needinfo?(peterv)
Flags: needinfo?(bugs)
Comment 39•9 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
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•