890284 - Stop splitting textnodes into 4KB chunks in the XML parser

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Like bug 194231, but for XML.

Henri, do you have time for this, or know who else might be a good bet to fix it?

Comment 1

[Henri Sivonen (:hsivonen)]

 I'm hoping to be able to look in the next couple of weeks.  As for who else, Ms2ger maybe?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Possibly. needinfo'ing myself so I don't forget

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I doubt I'll end up doing this.

Comment 5

[feedback]

Please fix this. Firefox is the only browser with this serious bug. Web apps that reply on correct XML parsing will fail under unpredictable circumstances, simply when they download too big data. 

Again, the testcase:

URL: http://quizzicalweb.com/bugreport/firefox_bug_1196926.htm

Comment 6

[Michal Holič]

Hi guys, I registered just to vote for this bug. It is platform independent, not just x86 Mac OS X as stated in the description - I have this same problem with Firefox 40.0.3 on Windows 7 x64.

In our company we use SAP NetWeaver Java AS EJB webservice which returns JSON in XML TextNode which is longer than 4096 characters - it fails so we have to use this workaround: https://bugzilla.mozilla.org/show_bug.cgi?id=194231#c68

What a nuisance, since it works without problem in IE 11, Edge and Chrome.

THIS IS A SERIOUS BUG REPORTED ON 2002-07-13 AND NEVER SOLVED SINCE!!!
https://bugzilla.mozilla.org/show_bug.cgi?id=157299

They say it is intended to work like that for performance reasons. Maybe that was the case in 2002. Today, I see it as a pure bull****.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop splitting textnodes in the XML content sink

Peter, Henri is out for a few more weeks and I think that leaves you and sicking as viable reviewers for this stuff....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff -w for review convenience

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 8662175 [details] [diff] [review]
Stop splitting textnodes in the XML content sink

Review of attachment 8662175 [details] [diff] [review]:
-----------------------------------------------------------------

r- for not removing the loop. Feel free to ignore if I'm missing a reason why it's needed.

::: dom/xml/nsXMLContentSink.cpp
@@ +1437,5 @@
>    // Copy data from string into our buffer; flush buffer when it fills up
>    int32_t offset = 0;
>    while (0 != aLength) {
>      int32_t amount = mTextSize - mTextLength;
>      if (0 == amount) {

So if |0 < amount < aLength| we'll go through this loop twice, but I don't see what that gains us. Couldn't we convert the |0 == amount| check into a check for |amount < aLength| and drop the loop? (You'd need to correct the mTextSize calculation, probably to |mTextSize += aLength - amount|).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop splitting textnodes in the XML content sink

Good catch.  Loop removed.  Interdiffs coming up.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: -w interdiff from previous patch

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8664443 [details] [diff] [review]
Stop splitting textnodes in the XML content sink

Review of attachment 8664443 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b426a4abde4a includes this patch.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e60dc914dd5b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out for being the likely cause of some linux asan failures in mochitest-5:

https://treeherder.mozilla.org/logviewer.html#?job_id=14724678&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/e882f6a95406

Comment 17

[Boris Zbarsky [:bzbarsky]]

I won't have a build environment for a week or so, unfortunately, so can't do anything about that until then.  That said:

1)  Were the asan failures consistent or intermittent?
2)  Did they go away after the backout?

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

1) Yes, quite consistent: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=2b5a7e0e2082&filter-searchStr=asan%20m%285&tochange=e882f6a95406
2) We'll see once the backout gets builds and tests.

There were a few other patches landed in the same push as yours, so maybe one of bug 1202696, bug 1197211, or bug 1182112 caused it, but none of them seems like as likely to me.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, hmm.  None of those seem terribly likely, including this one.  But OK, let's see what the tree says.  At least it's consistent...

Comment 20

[Boris Zbarsky [:bzbarsky]]

So I tried the simplest possible change: just always set mConstrainSize to false.  That same test ends up timing out....

Comment 21

[Boris Zbarsky [:bzbarsky]]

So this is exciting.  In a local ASAN build with my patch, I do see a very long pause right after "doTest testNum 7" as on try... but as soon as I blur the window the test proceeds and finishes.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Something in this test is just weirdly timing-dependent.  A debug build passes.  If I change the timers from 10ms to 200ms the test passes.  If I run under rr, the test passes.

Given that test_prompt.html is already skipped on Linux because it times out there (I bet for similar reasons), can we just skip this test under ASAN somehow?

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like "skip-if = asan" should do it.[1]


1. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser.ini#348

Comment 24

[Boris Zbarsky [:bzbarsky]]

OK, so I did some more digging, and this is _really_ fun.

This particular test in fact sends an XML file with a large textnode in it.  About 1MB of textnode, to be exact; see the "huge" case in toolkit/components/passwordmgr/test/authenticate.sjs.  The text is made up of lots of copies of "123456789\n" and the XML parser feeds that to the content sink as a sequence of "123456789", "\n", "123456789", "\n", etc.  Probably because it has to do newline normalization and counting anyway.

The upshot is that we do two reallocs for every 10 chars in this textnode, the textnode is about 1e6 chars, so we do around 200k reallocs.  I am guessing these are a lot more expensive with ASAN than without, so the test is actually timing out.

When running locally, various things one might do (like moving focus) can trigger a flush, which would empty the large buffer and mean many fewer reallocs are needed, which is why the test passed for me then, I expect.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch to fix the asan timeout

This more or less restores the old behavior, but just uses FlushText(false) to avoid starting a new textnode.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Complete patch against tip; may be easier to review maybe

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 8671460 [details] [diff] [review]
Complete patch against tip; may be easier to review maybe

Review of attachment 8671460 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xml/nsXMLContentSink.cpp
@@ +1429,1 @@
>    if (0 == mTextSize) {

It seems that after your patch mTextSize is either 0 (if mText is null) or NS_ACCUMULATION_BUFFER_SIZE (if mText is non-null)? Could we get rid of mTextSize and check for a null mText here?

@@ +1438,3 @@
>    int32_t offset = 0;
>    while (0 != aLength) {
>      int32_t amount = mTextSize - mTextLength;

This would be |amount = NS_ACCUMULATION_BUFFER_SIZE - mTextLength|.

@@ +1442,5 @@
> +      nsresult rv = FlushText(false);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +      }
> +      amount = mTextSize - mTextLength;

This could be |amount = NS_ACCUMULATION_BUFFER_SIZE - mTextLength;| or an assert that mTextLength == 0 and |amount = NS_ACCUMULATION_BUFFER_SIZE|.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yes, we can nix mTextSize as you suggest.  I'll do that.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Actually, per IRC conversation, we'll just make mText be a PRUnichar[NS_ACCUMULATION_BUFFER_SIZE] and nix that "is the buffer here?" check altogether.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b836be6a5d20

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b836be6a5d20