363086 - Manipulating SVGPathSegList through DOM causes seg fault in trunk

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jeff Schiller]

User-Agent:       Opera/9.02 (X11; Linux i686; U; en)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061207 Minefield/3.0a1

I was playing with SVGPathSegList, planning to update the source for two unsupported methods when I noticed that the trunk is causing a seg fault when it tries to clean up SVGPathSeg nodes added via JavaScript.  

Here is the following minimal testcase (I will also attach):

<?xml version="1.0" encoding="UTF-8" ?>
<svg xmlns="http://www.w3.org/2000/svg">
<g id="root"/>
<script><![CDATA[
    var path = document.createElementNS("http://www.w3.org/2000/svg", "path");
    path.setAttributeNS(null, "fill", "green");
    path.setAttributeNS(null, "d", "M200,200 L300,100");
    document.getElementById("root").appendChild(path);
    
    // XXX this line causes seg fault in trunk when you unload the document
    path.pathSegList.appendItem(path.createSVGPathSegLinetoAbs(400,200));

]]></script>
</svg>

1) Clear your cache
2) Browse to the testcase SVG document
3) Force reload of the page (shift-left-click the "Reload current page" button)

Within a few seconds, Firefox trunk crashes.

Alternatively

Reproducible: Always

Steps to Reproduce:
1. Clear your browser cache
2. Browse to the testcase SVG document
3. Force unload of the page (shift-left-click the "Reload current page" button is one option)

Actual Results:  
Firefox seg faults after a few seconds

Expected Results:  
Firefox should not crash when unloading a node

Comment 1

[Jeff Schiller]

Attachment: Minimal test case document

Force unload of this document in the Firefox trunk.  i.e. load this document, then force reload (shift-click the Reload button)

Comment 2

[Jeff Schiller]

Confirmed that this also happens in Windows.

Comment 3

[Andrew Schultz]

Attachment: stacktrace

Comment 4

[Andrew Schultz]

confirmed with current seamonkey trunk CVS 

Comment 5

[Robert Longson [:longsonr]]

Attachment: patch

Comment 6

[tor]

(In reply to comment #5)
> Created an attachment (id=248227) [edit]
> patch

Are you sure this is the right fix?  mSegments is a nsCOMArray, which will addref objects added to it itself.

Comment 7

[Jeff Schiller]

I agree with tor.  Something else is erroneously removing a reference
that was not added earlier, but it's not nsSVGPathSegList (which holds
its references via nsCOMArray).  

Could one of the "memory leak" fixes have done this accidentally?  Can
we pinpoint when this bug first showed up?  Since it's not happening when first creating the path (i.e. if you remove the XXX line), then it means something is correctly being done upon creation of the path, but not when updating...

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=248227) [edit]
> > patch
> 
> Are you sure this is the right fix?  mSegments is a nsCOMArray, which will
> addref objects added to it itself.
> 

The crash happens in ~nsSVGPathSegList as the 3rd nsSVGPathSeg element added by the javascript createSVGPathSegLinetoAbs has already been GCed (the first 2 elements are OK)

I do see we get 2 addrefs in the insert/append which seems unsettling as does the fact that we seem not to need an addref in SetValueString.

With the patch when you get to the nsSVGPathSegList destructor all the objects have refcnt = 0 so we are an addref short with the create or append somewhere. Something works differently between the SetValueString which creates the first two elements and the createSVGPathSegLinetoAbs which creates the third.

Comment 9

[Robert Longson [:longsonr]]

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

not sure what the right fix is.

Comment 10

[Jeff Schiller]

Oddly this only happens after you do the refresh of the document then move your mouse over the Fx window.  (This is also evident in the stack trace #55, OnMotionNotifyEvent).

Comment 11

[Jeff Schiller]

Scratch that - I did get it to crash using just the keyboard...

Comment 12

[Jeff Schiller]

Attachment: Updated patch

Add Ref to the return value in DOM AppendItem method

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 248622 [details] [diff] [review]
Updated patch

Jeff, nice to see you working on the code! Unfortunately that's not the right fix. AppendElement addrefs, and even if it didn't, NS_ADDREF would crash if null was passed into that method from JavaScript (you'd need NS_IF_ADDREF).

Comment 14

[Jeff Schiller]

Jonathan,

It's certainly possible that I don't understand parts of the code here, but here was my reasoning:   AppendElement does not explicitly add a reference, but it is true that mSegments is a nsCOMArray, which add and releases references internally.  However, what led me to believe this might be the right fix is the code for nsSVGPathSegList::RemoveItem() which does this (note the *****):

  *_retval = mSegments.ObjectAt(index);
  NS_ADDREF(*_retval); // *****
  WillModify();
  RemoveElementAt(index);
  DidModify();
  return NS_OK;

which I assume is adding a reference to the segment so that it doesn't get garbage collected and is still usable as a return value from script.  I had assumed from this, that any value that is returned from a JavaScript interface method should be addref'ed, I guess that was an invalid assumption?  Imagine the following JavaScript code:

// CASE A:
{
  var seg = somePath.pathSegList.removeItem(3);
  // at this point seg is still a valid segment
  doSomethingWithSeg(seg);
} // at this point, since all references to "seg" are gone
  // I'm assuming the JS engine calls Release on the pointer to seg
  // and the nsSVGPathSeg would now be GC'ed

// CASE B:
{
  var seg = null;

  { 
    // create somePath object here
    seg = somePath.pathSegList.appendItem(someNewSeg); 
    // delete somePath object here
  } // at this point, "seg" still should hold a reference to the path segment
    // shouldn't it?

  // as you mention, appendItem indirectly addref's within mSegments
  // but if we don't also addref for the JS engine, then wouldn't the
  // segment have been GCed and we'd be left with a dangling reference?

} // as with CASE A, when this brace is reached, we'd try to Release the 
  // reference to the seg and encounter a seg fault

Of course fixing the NULL issue could also be done by add ref'ing after the NULL check (which I did to my local copy of the code after I posted this patch, actually).

Anyway, I hope at least that some of my reasoning made sense and I look forward to understanding the Moz code a little better each day ;)

Comment 15

[Jeff Schiller]

Comment on attachment 248622 [details] [diff] [review]
Updated patch

>Index: nsSVGPathSegList.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGPathSegList.cpp,v
>retrieving revision 1.14
>diff -u -8 -p -r1.14 nsSVGPathSegList.cpp
>--- nsSVGPathSegList.cpp	10 Aug 2006 22:17:14 -0000	1.14
>+++ nsSVGPathSegList.cpp	14 Dec 2006 05:40:31 -0000
>@@ -268,16 +268,17 @@ NS_IMETHODIMP nsSVGPathSegList::RemoveIt
> NS_IMETHODIMP nsSVGPathSegList::AppendItem(nsIDOMSVGPathSeg *newItem,
>                                            nsIDOMSVGPathSeg **_retval)
> {
>   // XXX The SVG specs state that 'if newItem is already in a list, it
>   // is removed from its previous list before it is inserted into this
>   // list'. We don't do that. Should we?
> 
>   *_retval = newItem;
>   if (!newItem)
>     return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+  NS_ADDREF(*_retval);
>   AppendElement(newItem);
>   return NS_OK;
> }
> 
> 
> //----------------------------------------------------------------------

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 248622 [details] [diff] [review]
Updated patch

(In reply to comment #14)
> I had
> assumed from this, that any value that is returned from a JavaScript interface
> method should be addref'ed, I guess that was an invalid assumption?

Ah crap! You're right of course. The out parameter requires an addref too. Apologies.

> Of course fixing the NULL issue could also be done by add ref'ing after the
> NULL check (which I did to my local copy of the code after I posted this patch,
> actually).

Yes, please do it that way.

Comment 17

[Jeff Schiller]

Attachment: (not) Fixed per jwatt comment

Comment 18

[Jeff Schiller]

Attachment: Fixed per jwatt comment

Comment 19

[Jonathan Watt [:jwatt]]

Checked in for Jeff.
http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=1168505401&maxdate=1168505702