213881 - Properties for large RDF literals are repeated

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Ferran Puig]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030721
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030721

When parsing a RDF model which contains a large literal (I think it's  4k or
larger literal), Mozilla "splits" the literal, so when serializing the model
again, the resource which contained the original property, has the property
repeated as many times as (literal_length div 4096)+1, containing the chunks of
the original literal.

Reproducible: Always

Steps to Reproduce:
1. Parse String containing a RDF model with a 4096 bytes or larger literal
2. Serialize the content of the RDF model


Actual Results:  
Mozilla divided the literal, so many properties with the same name appeared,
containing the different chunks of the original literal

Expected Results:  
Keep the literal in just one property

Comment 1

[Ferran Puig]

Attachment: A test case for the bug

Attached a test case to show the bug in xpcshell

Comment 2

[Chase Tingley]

Confirming, -> All/All

This is a result of the AddText calling FlushText once the 4k buffer size in the
content sink has been hit.

This would be easy to fix by setting mConstrainSize to PR_FALSE, or removing it
altogether (it never changes after it's set by the constructor).  However,
looking at this code I notice that there are all manner of memcopies going on in
the literal-creation path (one to mText, another in FlushText for trim()ming,
and a third in GetLiteral() itself).  I wonder if one or more of those could be
eliminated.

Cc'ing waterson on the off-chance he knows something I don't about mConstrainSize.

Comment 3

[Chase Tingley]

Attachment: patch

This removes the mConstrainSize logic and the (bogus) call to FlushText to
create a literal based on partial data.

It also replaces the reallocate call (which never used to execute) with one
that will grow mText much faster, so that it doesn't simply reallocate for its
current size + aLength every time (which would cause gobs of incremental
reallocs once the literal got longer than 4k).

Note that the content sink is transient, so that the enlarged mText shouldn't
be hanging around and causing memory bloat after it's no longer needed.

As for the memory copies I mentioned in the previous comment, they should be
dealt with separately.

Comment 4

[Robert John Churchill]

Comment on attachment 128559 [details] [diff] [review]
patch

r=rjc

Comment 5

[Chris Waterson]

Looks fine to me, too.

Comment 6

[Chase Tingley]

Comment on attachment 128559 [details] [diff] [review]
patch

I was not aware Waterson was no longer an sr, so I'm fishing at this point. 
Brendan, feel free to suggest someone better...

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 128559 [details] [diff] [review]
patch

Nits:

1. (nsnull == mText) is discouraged compared to (!mText), in most code I watch.

2. The return is overindented by 2 spaces:

+    if (nsnull == mText)
+	 return NS_ERROR_OUT_OF_MEMORY;

Non-nit: use a temporary to hold the realloc return value, because if realloc
fails (I think this is guaranteed by ISO C), the original allocation should be
retained in mText.

Could matter on medium-memory(32-64M)/no-VM devices.

sr=[e-mail] with those cleanups.

/be

Comment 8

[Chase Tingley]

Bleh, forgot to set to myself.  anyways, fix is checked in with brendan's changes.

Regarding the indentation, the file is a little goofed up.  The majority of it
uses 4-space tabs, but a few functions (including this one) had weird hybrid
2/4-space tabs.  I've changed AddText() to follow the bulk of the file.