###!!! ASSERTION: data should be null terminated: 'data[len] == PRUnichar(0)', file ../../../../xpcom/string/src/nsSubstring.cpp, line 252, with filter animation patch applied

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 years ago
Per bug 896050 comment 44, we're hitting assertions in test_smilMappedAttrFromTo.xhtml with that bug's patch applied.

Sample log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27235958&full=1&branch=try#error0

The backtrace is to the ToString() invocation added in bug 904158. I probably need to tweak something from that patch.
Assignee

Comment 1

6 years ago
Ahhh, this is from another nsCheapString usage:
> 97   if (oldValStrBuf && valStr.Equals(nsCheapString(oldValStrBuf))) {
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILMappedAttribute.cpp#97

We need to use the exact same NS_strlen / animValBuf->ToString() stuff from bug 904158, in place of nsCheapString.
Assignee

Comment 2

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #0)
> The backtrace is to the ToString() invocation added in bug 904158.

(That^^ was wrong, BTW: the backtrace is actually to the ToString() invocation in the nsCheapString constructor quoted in comment 1 here.)
Assignee

Comment 3

6 years ago
Posted patch fix v1 (obsolete) — Splinter Review
This patch just factors out bug 904158's code* into a nsContentUtils helper-method, and then calls that helper-method in one additional place.

Reftest included, using "hg cp" from bug 904158's reftest, to change <set> to <animate>.  The test fails an assertion before the patch; it passes after.

*(bug 904158's logic = the code to convert a nsStringBuffer to a nsString, using NS_strlen to determine the length, and doing an allocated-length sanity-check to make sure the resulting string is in-bounds for the nsStringBuffer.)
Attachment #799100 - Flags: review?(dbaron)
Assignee

Comment 4

6 years ago
Posted patch fix v1a (obsolete) — Splinter Review
(forgot to qref in some minor cleanup. fixed now.)
Attachment #799100 - Attachment is obsolete: true
Attachment #799100 - Flags: review?(dbaron)
Assignee

Comment 5

6 years ago
Posted patch fix v1bSplinter Review
This version fixes one bug I just noticed in the original patches here. My old patches made PopulateStringFromStringBuffer() handle null buffers gracefully by returning an empty string.  However, we don't actually want that behavior.  Null is a special case in nsSMILMappedAttribute::SetAnimValue() -- it means we have *no* old animated value.  In that case, we definitely don't want to take the early-return. (With my old patch here, if the old value were null and the new value were the empty string, we would take the early return; but we shouldn't.)
Attachment #799103 - Attachment is obsolete: true
Attachment #799107 - Flags: review?(dbaron)
Comment on attachment 799107 [details] [diff] [review]
fix v1b

r=dbaron
Attachment #799107 - Flags: review?(dbaron) → review+
Comment on attachment 799107 [details] [diff] [review]
fix v1b

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

::: content/base/public/nsContentUtils.h
@@ +1715,5 @@
> +   * Populates aResultString with the contents of the string-buffer aBuf, up
> +   * to aBuf's null-terminator.  aBuf must not be null. Ownership of the string
> +   * is not transferred.
> +   */
> +  static void PopulateStringFromStringBuffer(nsStringBuffer* aBuf,

Why not make this a member function on nsStringBuffer?
Assignee

Comment 8

6 years ago
(In reply to :Ms2ger (away 11-21 September) from comment #7)
> Why not make this a member function on nsStringBuffer?

StringBuffer is an old and "lean" API; I didn't want to just add a new method to it (built on top of its already-exposed public API, so with no need to have access to anything internal) just because I need it in one spot.

For now, I think this makes enough sense as a "utils" method. If it makes a difference, we can maybe move it into nsStringBuffer in a future bug.
https://hg.mozilla.org/mozilla-central/rev/a405e2e0bef8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.