Closed
Bug 213881
Opened 21 years ago
Closed 21 years ago
Properties for large RDF literals are repeated
Categories
(Core Graveyard :: RDF, defect)
Core Graveyard
RDF
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sisqbatas, Assigned: mozilla)
Details
Attachments
(2 files)
6.47 KB,
application/x-javascript
|
Details | |
2.24 KB,
patch
|
mozilla
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•21 years ago
|
||
Attached a test case to show the bug in xpcshell
Comment 2•21 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Comment 3•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #128559 -
Flags: review?(rjc)
Assignee | ||
Comment 4•21 years ago
|
||
Comment on attachment 128559 [details] [diff] [review] patch r=rjc
Attachment #128559 -
Flags: review?(rjc) → review+
Comment 5•21 years ago
|
||
Looks fine to me, too.
Comment 6•21 years ago
|
||
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...
Attachment #128559 -
Flags: superreview?(brendan)
Comment 7•21 years ago
|
||
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=brendan@mozilla.org with those cleanups. /be
Attachment #128559 -
Flags: superreview?(brendan) → superreview+
Updated•21 years ago
|
Status: NEW → ASSIGNED
Comment 8•21 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•