103294 - Better nsCSSParser support for parsing SVG property "stroke-dasharray"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniele Nicolodi]

The SVG project need to add a list type into nsCSSValue and the relative parser
(nsSCCParser) for this type of elements.

Comment 1

[Daniele Nicolodi]

Attachment: A proposed patch (whith updates of SVG code)

Comment 2

[Bradley Baetz (:bbaetz)]

I'll check this in on the SVG branch, but since it touches core code I'm not
familiar with I'd like style system people to look at it first.

Confirming

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The ifdef-ing in the nsCSSUnit enum will break non-SVG builds.  Haven't looked
closely yet, though.

Comment 4

[Daniele Nicolodi]

Attachment: Fixed.

Comment 5

[Pierre Saslawsky]

In the second patch, eCSSUnit_Seconds and eCSSUnit_Milliseconds have the same 
value in non-svg builds (= 3000).  You could have replaced the original 
definition with the following (note the comma at the beginning of the line):

    #ifdef MOZ_SVG
      // Lists
      , eCSSUnit_List         = 4000     // (array)
    #endif

Comment 6

[Daniele Nicolodi]

Attachment: Changed.

Comment 7

[Pierre Saslawsky]

It is difficult to do a review on files that belong to a branch without pulling 
the branch itself.  Reviews are usually requested for diffs that apply to the 
trunk.  Some of the files even belong to the Attic: it sounds bad to me!

My advice would be for you to work on the trunk whenever possible.  Otherwise 
turn off the MOZ_SVG option and compile with and without your changes.  As long 
as the changes don't cause any modification of the binaries (menaing they are all 
#ifdef'd MOZ_SVG), you are fine.  Then request a review when you are ready to 
land the branch.

Comment 8

[Bradley Baetz (:bbaetz)]

Trust me. I really really want to merge the branch, but its stuck on legal
issues (we can't use the stock libart on the mac, and can't check the one we
have into the tree because its LGPL'd not MPL'd, and staff@m.o haven't finalised
a place for non-mPL stuff to be checked in)

SVG is not going to be on by default, but I've been trying to get sanity checks
for main parts of the code which I don't know much about. Such as the css parser.

danielle, since noone went "yuck", if you fix up the #ifdefs, I'll put this onto
the branch. Also:

a) where do you delete mList?
b)
  else if (eCSSUnit_None == SVGData.mStrokeDasharray.GetUnit()) {
-    svg->mStrokeDasharray.Truncate();
+    svg->mStrokeDasharray = nsnull;

leaks if we change via the dom.

pierre: the stuff in the attic is for new files added on the branch.

Comment 9

[Daniele Nicolodi]

Attachment: New patch

Comment 10

[Daniele Nicolodi]

New patch. Do not solves problems. 
Is only an update for keep patch apply to cvs tree.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 56068 [details] [diff] [review]
New patch

>+    nsAutoVoidArray * aList = new nsAutoVoidArray();
>+    while (1) {
>+      nsCSSValue * element = new nsCSSValue();
>+      if (! ParseVariant(aErrorCode, *element, aVariantMask, aKeywordTable)) {
>+        return PR_FALSE;
>+      }

Um. If we hit that |return PR_FALSE;| we leak the nsAutoVoidArray and the
nsCSSValue that we just allocated.

>+      if (! ExpectSymbol(aErrorCode, ',', PR_TRUE)) {

Shouldn't you use GetToken and check whether it's ',', then unget it if it's
not and return?  I think this usage of ExpectSymbol will screw up error
reporting (report an error in a non-error case).

>   void  Reset(void)  // sets to null
>   {
>+#ifdef MOZ_SVG
>+    if ((eCSSUnit_List == mUnit) && (mValue.mList != nsnull)) {
>+      printf("mValue.mList->Clear() %p\n", this);
>+      mValue.mList->Clear();
>+    }
>+#endif

This will leak all the CSSValues in the array.	Clear() does not free the
pointers, it just forgets about them.

Please do one of two things:
1) Walk the array and free the pointers by hand
2) Create a new subclass of nsVoidArray (see nsStringArray, eg)
   that handles deletion on Clear() for you.

Also, since Reset() is called by all the Set* methods and they immediately
clobber the value (and since it's the only cleanup in the destructor) you want
to free mList itself here.  See what mString does, eg.

Alos, in nsCSSValue.cpp some of the tests will stop being correct when you add
|eCSSUnit_List	     = 4000|.  For example:

NS_ASSERTION(eCSSUnit_Percent <= aUnit, "not a float value");

will now _not_ assert if you pass in eCSSUnit_List to the constructor that
takes
a float. Please check this file over and see whether any other tests need
modification #ifdef MOZ_SVG.

>+  nsAutoVoidArray * GetListValue(void) const
>+  {
>+    NS_ASSERTION((mUnit == eCSSUnit_List), "not a list value");
>+    printf("nsCSSValue::GetListValue %p\n", this);
>+    if (mUnit == eCSSUnit_List) {
>+      return mValue.mList;
>+    }
>+    return 0;
>+  }

maybe nsnull instead of 0?

>+  if (style.dasharray != nsnull) {

just do |if (style.dasharray)|

I don't know anything about svg, so someone else should review the svg foo in
this.

Comment 12

[Boris Zbarsky [:bzbarsky]]

ccing rjesup for voidarray wisdom.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

bz's comments are correct; if the items in the ns[Auto]VoidArray are XPCOM
objects that were refcounted when they went in, you need to un-refcount them by
hand before Clear().  Or you could use nsSupportsArray, but that adds overhead
on top of refcounting.

Deleting the list implicitly Clear()'s it (again note refcounting issues).

nsAutoVoidArray is appropriate if the expected number of elements in an array is
between 1 and 8.  If 0 is predominate, or values over 8, a plain nsVoidArray
should be used.  If the (likely) size is known, nsVoidArray foo(number) should
be used.  If the expected values are 0 and 1 (with 1 fairly frequent) and bloat
is a major consideration, then nsCheapVoidArray (soon to move to XPCOM and
become nsSmallVoidArray) may be better.

Comment 14

[Daniele Nicolodi]

Now there is a new nsCSSValueList. I think we could i use that.

See new code on 3.15 revision:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/html/style/src/nsICSSDeclaration.huse

Comment 15

[Daniele Nicolodi]

Wrong URL. The right one:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/content/html/style/src/nsICSSDeclaration.h

Comment 16

[Daniele Nicolodi]

Attachment: Completely new patch that uses nsCSSValueList

This patch works well :)

Comment 17

[Daniele Nicolodi]

Attachment: New patch with Alex suggestions

I think that there could be a memory leak. 

struct	nsSVGStrokeStyle 
{
  float * dasharray;
  PRInt32 dashcount;
  float   dashoffset;
  PRInt8  linecap;   // see nsStyleConsts.h
  PRInt8  linejoin;  // see nsStyleConsts.h
  float   miterlimit; 
  float   width;
};

strokeStyle.dasharray = new float[strokeStyle.dashcount];

this array is never freed. I'm right ?

Please review.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Yes, it appears to leak.

Comment 19

[Daniele Nicolodi]

Attachment: Updated patch with leak fixed (we stop to copy the array)

This patch looks good but causes a segfault on
nsCSSValueList::~nsCSSValueList() called from nsCSSSVG::~nsCSSSVG() (go to
http://www.grinta.net/svg/examples/23.xml then click back) but i'm not able to
found the error.

Comment 20

[Daniele Nicolodi]

Attachment: Segfaults fixed (i was looking for the bug in the wrong place)

Plase review and checkin. Thanks

Comment 21

[Daniele Nicolodi]

I haven't time to get s= sr= and a= ... please help

Comment 22

[Bradley Baetz (:bbaetz)]

Err. I'm not the best person to be asking for time at the moment ;)

Try mailing [e-mail] + [e-mail] with your request, and cc
[e-mail]

I'm not really able to review this - its not my area.

Comment 23

[tor]

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

Comment 24

[tor]

Attachment: version corresponding to current svg code

Comment 25

[Boris Zbarsky [:bzbarsky]]

It may take me a few days to get to this, possibly.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I guess I should have said "few weeks".  It'll be at least until Tuesday
best-case, likely later.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 161881 [details] [diff] [review]
version corresponding to current svg code

>Index: content/base/src/nsRuleNode.cpp

Note that the SVG struct is inherit by default.  This means that by the time we
get to here we've already copied the mStrokeDashArray from our parent, possibly
(in the eRuleFullMixed/eRuleFullReset cases).

>+    else if (eCSSUnit_Inherit == list->mValue.GetUnit()) {
>+      inherited = PR_TRUE;
>+      svg->mStrokeDasharrayLength = parentSVG->mStrokeDasharrayLength;
>+      svg->mStrokeDasharray = new float[svg->mStrokeDasharrayLength];
>+      memcpy(svg->mStrokeDasharray,
>+             parentSVG->mStrokeDasharray,
>+             svg->mStrokeDasharrayLength * sizeof(float));

Which means this code does extra work (and possibly leaks).  It should probably
only do this if (!svg->mStrokeDasharray && parentSVG->mStrokeDasharray). 
Indeed, if svg->mStrokeDasharray is non-null here, then we must have copied it
from parent already, and if it's null and the parent's is also null we're
already all set.

That way we also avoid doing |new float[0]|...

>+    } else {

And here we need to |delete [] svg->mStrokeDasharray;|

>+      // count number of values
>+      svg->mStrokeDasharrayLength = 0;
>+      nsCSSValueList *value = SVGData.mStrokeDasharray;
>+      while (nsnull != value) {
>+        svg->mStrokeDasharrayLength++;
>+        value = value->mNext;
>+      }

Add an assert here that svg->mStrokeDasharrayLength is nonzero?

Also, what about the case when it's set to -moz-initial?  That will get through
the parser (because VARIANT_INHERIT includes -moz-initial) and looks like it'll
set the length to a random value (by not setting it) below.

>Index: content/html/style/src/nsCSSParser.cpp

>+PRBool CSSParserImpl::ParseDasharray(nsresult& aErrorCode)
...
>+  for (nsCSSValueList **curp = &list, *cur; ; curp = &cur->mNext) {
>+    if (!ParseVariant(aErrorCode, cur->mValue, VARIANT_HLPN, nsnull)) {

This will parse things like:

  stroke-dasharray: inherit, 5px;

We should only be including VARIANT_INHERIT for the first thing we parse, and
if we get back an inherit/initial we should go to ExpectEndProperty.  See what
ParseContent() does, for example.

>Index: content/html/style/src/nsCSSStruct.cpp

>+    value->mValue.AppendToString(buffer, eCSSProperty_stroke_dasharray);

This won't look pretty, but it's debug-only, I guess.  ;)

>Index: content/shared/src/nsStyleStruct.cpp

>@@ -866,17 +867,25 @@ nsStyleSVG::nsStyleSVG(const nsStyleSVG&
>-  mStrokeDasharray = aSource.mStrokeDasharray;
>+  if (aSource.mStrokeDasharray) {

And if it's null you never set mStrokeDasharray or mStrokeDasharrayLength? 
They want to become null and zero respectively, right?	In fact, it's quite
safe to set the length altogether outside this null-check.

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp

> nsSVGGlyphFrame::GetStrokeDashArray(float **arr, PRUint32 *count)
> {
>+  *count = ((const nsStyleSVG*) mStyleContext->GetStyleData(eStyleStruct_SVG))->mStrokeDasharrayLength;
>+  *arr = ((const nsStyleSVG*) mStyleContext->GetStyleData(eStyleStruct_SVG))->mStrokeDasharray;

This violates the interface contract; if it's an XPCOM interface defined in
IDL, it needs to allocate/addref/whatever when it returns things....

>Index: layout/svg/base/src/nsSVGPathGeometryFrame.cpp

Same issue here.

>Index: layout/svg/renderer/src/cairo/nsSVGCairoGlyphGeometry.cpp

And this change probably needs to be undone.

>Index: layout/svg/renderer/src/cairo/nsSVGCairoPathGeometry.cpp

And this one.

>Index: layout/svg/renderer/src/gdiplus/nsSVGGDIPlusGlyphGeometry.cpp

And this one.

>Index: layout/svg/renderer/src/gdiplus/nsSVGGDIPlusPathGeometry.cpp

And this.

>Index: layout/svg/renderer/src/libart/nsSVGStroke.cpp

And here.

Marking r- based on the rulenode/parser issues and the memory management
issues...

Comment 28

[tor]

Attachment: adjust per review comments

Comment 29

[tor]

Attachment: adjust ComputeSVGData

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164411 [details] [diff] [review]
adjust ComputeSVGData

>Index: content/html/style/src/nsCSSParser.cpp
>+PRBool CSSParserImpl::ParseDasharray(nsresult& aErrorCode)
>+{
>+  nsCSSValue value;
>+  if (ParseVariant(aErrorCode, value, VARIANT_HLPN, nsnull)) {

You want "| VARIANT_NONE" there, right?  Since "none" is a valid value?

>+      if (eCSSUnit_Inherit == value.GetUnit() ||
>+          eCSSUnit_Initial == value.GetUnit()) {

And you'd want to check for eCSSUnit_None here.

>Index: layout/svg/base/src/nsSVGGlyphFrame.cpp

>+  *count = ((const nsStyleSVG*) mStyleContext->GetStyleData(eStyleStruct_SVG))->mStrokeDasharrayLength;

Is there a reason not to use:

 *count = GetStyleSVG()->mStrokeDasharrayLength;

?  It'll end up being the same code, but without the need to cast...

>+      memcpy(*arr, ((const nsStyleSVG*) mStyleContext->GetStyleData(eStyleStruct_SVG))->mStrokeDasharray, *count * sizeof(float));

You may even want to save the nsStyleSVG pointer that GetStyleSVG returns so
you don't have to get it twice...

>Index: layout/svg/base/src/nsSVGPathGeometryFrame.cpp

Same comments here.

r=bzbarsky with those fixed.

Comment 31

[tor]

Checked in.

Comment 32

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050316
Firefox/1.0+ :SVG

<?xml version="1.0"?><html xmlns="http://www.w3.org/1999/xhtml"
xmlns:svg="http://www.w3.org/2000/svg">
 <head></head>
 <body>
  <svg:svg width="100px" height="100px">
   <svg:g style="stroke:grey;stroke-dasharray:'5,5'">
    <svg:line x1="0" y1="0" x2="100" y2="100"/>
   </svg:g>
  </svg:svg>
 </body>
</html>


use to produce a dashed line, since quite a while it does not work anymore.

I'll leave it to someone else to reopen, or do I need to file a new bug ?

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Neither.  That's invalid CSS, and should be ignored.  Drop the inner quotation
marks.

Comment 34

[Robin Berjon]

(In reply to comment #33)
> Neither.  That's invalid CSS, and should be ignored.  Drop the inner quotation
> marks.

And, in case there's any doubt, it's (thankfully) also invalid according to SVG.