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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

(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.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(forgot to qref in some minor cleanup. fixed now.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1b

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.)

Comment 6

[David Baron :dbaron:]

Comment on attachment 799107 [details] [diff] [review]
fix v1b

r=dbaron

Comment 7

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

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?

Comment 8

[Daniel Holbert [:dholbert]]

(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.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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