329848 - ASSERTION: transform-attribute parse error

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

###!!! ASSERTION: transform-attribute parse error: 'Error', file /Users/admin/trunk/mozilla/content/svg/content/src/nsSVGTransformList.cpp, line 246

I think this is just a bogus assertion.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jonathan Watt [:jwatt]]

Yeah, this is yet another case where we should be sending a message to the js console instead of asserting.

Comment 3

[Robert Longson [:longsonr]]

Attachment: patch

I was planning to get tor to sr this, do I need to get anyone else to review it?

Comment 4

[Robert Longson [:longsonr]]

Attachment: new svg.properties file /mozilla/dom/locales/en-US/chrome/svg/svg.properties

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

>Index: content/base/public/nsContentUtils.h
>Index: content/base/src/nsContentUtils.cpp
>+  "chrome://global/locale/svg/svg.properties",

That trailing comma will break builds.  Remove it.

Does this need to be in #ifdef MOZ_SVG?

>Index: content/svg/content/src/nsSVGElement.cpp

>+      nsAutoString attributeName, attributeValue(aValue);

Why is attributeValue an nsAutoString?  Please use PromiseFlatString() as needed if you need a flat string from an nsAString.

>+      nsSVGUtils::ReportToConsole(GetCurrentDoc(),

Why the current doc and not the owner doc?  That looks wrong.

>Index: dom/locales/jar.mn
>+  locale/@AB_CD@/global/svg/svg.properties                     (%chrome/svg/svg.properties)

Again, should this be ifdefed?

Comment 6

[Robert Longson [:longsonr]]

Attachment: address review comments

> (From update of attachment 230612 [details] [diff] [review] [edit])
> >Index: content/base/public/nsContentUtils.h
> >Index: content/base/src/nsContentUtils.cpp
> >+  "chrome://global/locale/svg/svg.properties",

> That trailing comma will break builds.  Remove it.

> Does this need to be in #ifdef MOZ_SVG?

Added #ifdef MOZ_SVG and moved property up so that it always requires a comma.

> >Index: content/svg/content/src/nsSVGElement.cpp

> >+      nsAutoString attributeName, attributeValue(aValue);

> Why is attributeValue an nsAutoString?  Please use PromiseFlatString() as
> needed if you need a flat string from an nsAString.

Done.

> >+      nsSVGUtils::ReportToConsole(GetCurrentDoc(),

> Why the current doc and not the owner doc?  That looks wrong.

Changed. When would you use GetCurrentDoc over GetOwnerDoc?

> >Index: dom/locales/jar.mn
> >+  locale/@AB_CD@/global/svg/svg.properties                     (%chrome/svg/svg.properties)

> Again, should this be ifdefed?

Changed, didn't realise you could do that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230721 [details] [diff] [review]
address review comments

>Index: content/svg/content/src/nsSVGElement.cpp
>+      const PRUnichar *strings[] = { attributeName.get(), PromiseFlatString(aValue).get() };

After this line, that pointer is pointing at the innards of an object that's gone.  So if aValue is not already flat, you will generally crash.

You need to keep the object alive:

const nsAFlatString& flatValue = PromiseFlatString(aValue);

  then use flatValue.get().  Or something along those lines.

>Index: dom/locales/jar.mn
>+#ifdef MOZ_SVG  
>+  locale/@AB_CD@/global/svg/svg.properties                     (%chrome/svg/svg.properties)
>+#endif

And you did check that this works (that is, that this file is preprocessed), right?

In general, GetOwnerDoc() is the same as the DOM's ownerDocument.  GetCurrentDoc() is the document the node is being rendered in (e.g. null for nodes that are not inserted into a document).

Comment 8

[Robert Longson [:longsonr]]

Attachment: address additional review comments

> (From update of attachment 230721 [details] [diff] [review] [edit])
> >Index: content/svg/content/src/nsSVGElement.cpp
> >+      const PRUnichar *strings[] = { attributeName.get(), PromiseFlatString(aValue).get() };

> After this line, that pointer is pointing at the innards of an object that's
> gone.  So if aValue is not already flat, you will generally crash.

> You need to keep the object alive:

> const nsAFlatString& flatValue = PromiseFlatString(aValue);

>   then use flatValue.get().  Or something along those lines.

Done.

> >Index: dom/locales/jar.mn
> >+#ifdef MOZ_SVG  
> >+  locale/@AB_CD@/global/svg/svg.properties                     (%chrome/svg/svg.properties)
> >+#endif

> And you did check that this works (that is, that this file is preprocessed),
right?

I checked that this was the correct syntax by looking at other jar.mn files. I then rebuilt the whole browser and checked that the testcase still caused my new warning message to be displayed in the Error Console. Is there any other testing you would recommend? 

After reading your response I also checked that the svg.properties file was in en-US.jar and that the date of en-US.jar was later than that of the jar.mn file that I changed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230747 [details] [diff] [review]
address additional review comments

r=bzbarsky

Comment 10

[Robert Longson [:longsonr]]

Checked in.

Comment 11

[Robert Longson [:longsonr]]

This checkin seems to have screwed things up on various port builds "nsSVGElement.cpp", line 226: Error: ReportToConsole is not a member of nsSVGUtils

I have commented out the offending call temporarily while I try to determine what is wrong.

Comment 12

[Robert Longson [:longsonr]]

Attachment: restore functionality after removing to fix bustage

Is it OK to try this again now?

Comment 13

[Robert Longson [:longsonr]]

Attachment: fix build warning

additional spaces after MOZ_SVG causes a warning in the build log. Removing the bogus spaces fixes it.

Comment 14

[Robert Longson [:longsonr]]

Comment on attachment 231089 [details] [diff] [review]
fix build warning

Checked in fix for build warning.

Comment 15

[Robert Longson [:longsonr]]

checked in fix to restore functionality. Fortunately blackdeath was OK with it this time.

Comment 16

[Boris Zbarsky [:bzbarsky]]

*** Bug 322528 has been marked as a duplicate of this bug. ***

Comment 17

[Jesse Ruderman]

Crashtest checked in.