403549 - nsContentUtils::CreateContextualFragment churns memory

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix -w

Anyone calling nsIParser::ParseFragment has to provide a tag-stack in a very suboptimal format, requiring a bunch of manual memory management and a lot of memory churn.

We should use nsTArray<nsAutoString>& as datatype for the tagstack argument instead. Note that that allows callsites to use nsAutoTArray<nsAutoString, ...> since that can be cast to nsTArray

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Without -w

Same as above, but diffed without -w

Comment 2

[Alex Vincent [:WeirdAl]]

Comment on attachment 288400 [details] [diff] [review]
Patch to fix -w

>Index: content/base/src/nsContentUtils.cpp
>+        info->GetNamespaceURI(uriStr);
>+        tagName.Append(NS_LITERAL_STRING(" xmlns=\"") + uriStr +
>                          NS_LITERAL_STRING("\""));

I hope you'll pardon a peanut gallery question, but what if uriStr has a double-quote character in it already?  Wouldn't that lead to a well-formedness error?

Comment 3

[jag (Peter Annema)]

Is 32 enough? Might 40, 50 or 64 be better?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Alex: Yes. We should entity encode quotes. Feel free to file a bug.

Jag: We didn't see any cases where it was over 32. There are limits in the HTML parser that stops DOMs from getting too deep during parsing, however using the DOM you can of course create any depth. However the code should work fine anyway, it'll just hit the heap. As long as we don't hit the heap in 99% of the cases I think we're fine.

Comment 5

[Alex Vincent [:WeirdAl]]

sicking: filed as bug 403643.

Comment 6

[J. Lance Cotton]

I have noticed in Thunderbird (trunk) nightlies that when I send a message and it gets copied to the IMAP Sent folder, Thunderbird crashes.

I noticed that it's not just me:
http://crash-stats.mozilla.com/report/list?range_unit=days&version=Thunderbird%3A3.0a1pre&range_value=2&signature=nsParser%3A%3AParseFragment(nsAString_internal+const%26%2C+void*%2C+nsTArray%3CnsAutoString%3E%26%2C+int%2C+nsACString_internal+const%26%2C+nsDTDMode)

I'm no expert, but I do see that this patch changed a line that is referred to in the crash reports.

Comment 7

[Joe Sabash [:JoeS1]]

FWIW Can't seem to duplicate TB trunk crash in Pop3, requires IMAP ?
Top Crasher for TB trunk now.
http://crash-stats.mozilla.com/topcrasher/byversion/Thunderbird/3.0a1pre

Comment 8

[:Gavin Sharp [email: [e-mail]]]

 (In reply to comment #6)
> I have noticed in Thunderbird (trunk) nightlies that when I send a message and
> it gets copied to the IMAP Sent folder, Thunderbird crashes.

That's probably bug 403664, which is already in this bug's dependency list. (Although if you have evidence that it's not that bug you should certainly file a new one.) 

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This was checked in.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Adding footprint keyword as this causes fragmentation. Don't think the perf win is big enough for that keyword though.