950762 - Switch DOMParser::ParseFromString to use fallible allocator

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

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?

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: Added fallible version of AppendUTF16toUTF8, currently used to make DOMParser::ParseFromString fallible

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.

Comment 4

[Johnny Stenback (: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.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Johnny Stenback (:jst, [e-mail]) 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 6

[Benjamin Smedberg]

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.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug950762.patch

Thank you Benjamin, I've fixed the problem.

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47ffc320ef0c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47ffc320ef0c