Closed Bug 950762 Opened 7 years ago Closed 7 years ago

Switch DOMParser::ParseFromString to use fallible allocator

Categories

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

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: benjamin, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

On Nightly I'm seeing a fairly frequent OOM crash from DOMParser::ParseFromString called from script. e.g. bp-6fa820ca-cabe-4f38-a509-6057e2131215 (268MB allocation). This is coming from the infallible NS_ConvertUTF16toUTF8 at http://hg.mozilla.org/mozilla-central/annotate/1ad9af3a2ab8/content/base/src/DOMParser.cpp#l109

Currently there is not a falliable variant of AppendUTF16toUTF8 or NS_ConvertUTF16toUTF8 which is fallible, but it wouldn't be hard to one or both. There is already a fallible variant of AppendUTF8toUTF16 which can be used as a model.
Assignee: nobody → alessio.placitelli
I've coded a fallible variant for AppendUTF16toUTF8, but I'm not quite sure on how to design a fallible variant for NS_ConvertUTF16toUTF8: should I set the destination to a 0-length string and provide a method to check for failures?
Flags: needinfo?(benjamin)
Maybe the best option would be to skip NS_Convert and just use AppendUTF16toUTF8 directly. But if you wanted to do it, the best API would probably be to set the string as void in case of failure, and use IsVoid() to check for failure.
Flags: needinfo?(benjamin)
I've added a fallible version of AppendUTF16toUTF8, along the lines of the AppendUTF8toUTF16 function. This function is used to make DOMParser::ParseFromString return on out of memory errors.
Attachment #8350174 - Flags: review?(jst)
Comment on attachment 8350174 [details] [diff] [review]
Added fallible version of AppendUTF16toUTF8, currently used to make DOMParser::ParseFromString fallible

r=jst, but reguesting additional review from bsmedberg since he owns the XPCOM string stuff.
Attachment #8350174 - Flags: review?(jst)
Attachment #8350174 - Flags: review?(benjamin)
Attachment #8350174 - Flags: review+
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #4)
> Comment on attachment 8350174 [details] [diff] [review]
> Added fallible version of AppendUTF16toUTF8, currently used to make
> DOMParser::ParseFromString fallible
> 
> r=jst, but reguesting additional review from bsmedberg since he owns the
> XPCOM string stuff.

Thanks for the review!
Comment on attachment 8350174 [details] [diff] [review]
Added fallible version of AppendUTF16toUTF8, currently used to make DOMParser::ParseFromString fallible

>diff --git a/xpcom/string/src/nsReadableUtils.cpp b/xpcom/string/src/nsReadableUtils.cpp

>+bool
>+AppendUTF16toUTF8( const nsAString& aSource, nsACString& aDest,
>+                   const mozilla::fallible_t&)

Nit, add a space before the final ) to match local style.
Attachment #8350174 - Flags: review?(benjamin) → review+
Attached patch bug950762.patchSplinter Review
Thank you Benjamin, I've fixed the problem.
Attachment #8350174 - Attachment is obsolete: true
Attachment #8350875 - Flags: review?(benjamin)
Comment on attachment 8350875 [details] [diff] [review]
bug950762.patch

Once I've marked r+ you don't need to request review again unless you're unsure about something.
Attachment #8350875 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47ffc320ef0c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.