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

RESOLVED FIXED

Status

()

RESOLVED FIXED
17 years ago
14 years ago

People

(Reporter: daniele, Assigned: daniele)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

17 years ago
The SVG project need to add a list type into nsCSSValue and the relative parser
(nsSCCParser) for this type of elements.
Summary: New list type into nsCSSValue for parsing SVG property stroke-dasharray → [RFE] New list type into nsCSSValue for parsing SVG property stroke-dasharray
(Assignee)

Comment 1

17 years ago
Created attachment 52214 [details] [diff] [review]
A proposed patch (whith updates of SVG code)
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

17 years ago
Keywords: patch, review
The ifdef-ing in the nsCSSUnit enum will break non-SVG builds.  Haven't looked
closely yet, though.
(Assignee)

Comment 4

17 years ago
Created attachment 52256 [details] [diff] [review]
Fixed.

Comment 5

17 years ago
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
(Assignee)

Comment 6

17 years ago
Created attachment 52357 [details] [diff] [review]
Changed.

Comment 7

17 years ago
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.
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.
(Assignee)

Updated

17 years ago
Attachment #52214 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #52256 - Attachment is obsolete: true
(Assignee)

Comment 9

17 years ago
Created attachment 56068 [details] [diff] [review]
New patch
(Assignee)

Comment 10

17 years ago
New patch. Do not solves problems. 
Is only an update for keep patch apply to cvs tree.
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.
Attachment #56068 - Flags: needs-work+
ccing rjesup for voidarray wisdom.
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.
(Assignee)

Comment 14

17 years ago
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

(Assignee)

Comment 16

17 years ago
Created attachment 64738 [details] [diff] [review]
Completely new patch that uses nsCSSValueList

This patch works well :)
Attachment #52357 - Attachment is obsolete: true
Attachment #56068 - Attachment is obsolete: true
(Assignee)

Comment 17

17 years ago
Created attachment 65023 [details] [diff] [review]
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.
Attachment #64738 - Attachment is obsolete: true
Yes, it appears to leak.
(Assignee)

Comment 19

17 years ago
Created attachment 65830 [details] [diff] [review]
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.
Attachment #65023 - Attachment is obsolete: true
(Assignee)

Comment 20

17 years ago
Created attachment 65908 [details] [diff] [review]
Segfaults fixed (i was looking for the bug in the wrong place)

Plase review and checkin. Thanks
Attachment #65830 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Summary: [RFE] New list type into nsCSSValue for parsing SVG property stroke-dasharray → Better nsCSSParser support for parsing SVG property "stroke-dasharray"
(Assignee)

Comment 21

17 years ago
I haven't time to get s= sr= and a= ... please help
Assignee: daniele → bbaetz
Err. I'm not the best person to be asking for time at the moment ;)

Try mailing dbaron@fas.harvard.edu + bzbarsky@mit.edu with your request, and cc
reviewers@mozilla.org

I'm not really able to review this - its not my area.
Assignee: bbaetz → daniele

Comment 23

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

Comment 24

14 years ago
Created attachment 161881 [details] [diff] [review]
version corresponding to current svg code
Attachment #65908 - Attachment is obsolete: true

Updated

14 years ago
Attachment #161881 - Flags: review?(bzbarsky)
It may take me a few days to get to this, possibly.
I guess I should have said "few weeks".  It'll be at least until Tuesday
best-case, likely later.
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...
Attachment #161881 - Flags: review?(bzbarsky) → review-

Comment 28

14 years ago
Created attachment 164298 [details] [diff] [review]
adjust per review comments
Attachment #161881 - Attachment is obsolete: true

Comment 29

14 years ago
Created attachment 164411 [details] [diff] [review]
adjust ComputeSVGData
Attachment #164298 - Attachment is obsolete: true

Updated

14 years ago
Attachment #164411 - Flags: review?(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.
Attachment #164411 - Flags: review?(bzbarsky) → review+

Comment 31

14 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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 ?
Neither.  That's invalid CSS, and should be ignored.  Drop the inner quotation
marks.

Comment 34

14 years ago
(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.
You need to log in before you can comment on or make changes to this bug.