Martijn Wargers (dead) Reporter
	Description • 17 years ago

See testcase, which crashes current trunk build.

This regressed between 2007-01-17 and 2007-01-18:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-01-17+04&maxdate=2007-01-18+09&cvsroot=%2Fcvsroot
Before that date, I get not implemented errors.
So I think this is a regression from bug 363839, somehow.

http://crash-stats.mozilla.com/report/index/30701cb5-280a-11dd-a593-0013211cbf8a?p=1
0  	xul.dll  	nsCOMPtr<nsIScriptContext>::operator=  	 nsCOMPtr.h:713
1 	xul.dll 	nsSVGPathSeg::SetCurrentList 	mozilla/content/svg/content/src/nsSVGPathSeg.cpp:151
2 	xul.dll 	nsSVGPathSegList::RemoveElementAt 	mozilla/content/svg/content/src/nsSVGPathSegList.cpp:344
3 	xul.dll 	nsSVGPathSegList::ReplaceItem 	mozilla/content/svg/content/src/nsSVGPathSegList.cpp:251
4 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
5 	xul.dll 	XPCWrappedNative::CallMethod 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2388

	Jeff Schiller
	Comment 1 • 17 years ago

This is specifically in the nsSVGPathSegList::ReplaceItem() method.  There are at least two cases that are not covered :

1) segList has {x,y}.  replaceItem(x,0) results in a segList with {x}
2) segList has {x}.  replaceItem(x,0) results in a crash

I am pasting in a fix for ReplaceItem() that I _think_ will fix it.  I am currently many months behind on my CVS local copy so building Moz is going to take hours.  If anyone else wants to test this, please feel free:

 /* nsIDOMSVGPathSeg replaceItem (in nsIDOMSVGPathSeg newItem, in unsigned long index); */
 NS_IMETHODIMP nsSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *newItem,
                                             PRUint32 index,
                                             nsIDOMSVGPathSeg **_retval)
 {
  NS_ENSURE_NATIVE_PATH_SEG(newItem, _retval);

  if (index >= static_cast<PRUint32>(mSegments.Count())) {
    return NS_ERROR_DOM_INDEX_SIZE_ERR;
  }
  
  // get item that we will replace
  nsIDOMSVGPathSeg* segToReplace = mSegments.ObjectAt(index);
  
  // if the item we are inserting is not the same we are replacing, 
  // then we have actual work to do
  if (newItem != segToReplace) {
    InsertElementAt(static_cast<nsSVGPathSeg*>(newItem), index);
    RemoveFromCurrentList(segToReplace);
    NS_ADDREF(newItem);
  }
  
  *_retval = newItem;

  return NS_OK;
 }

	Jeff Schiller
	Comment 2 • 17 years ago

Also, is there a problem with insertItemBefore via InsertElementAt() if the index is beyond the length of the current segList?  I'm not sure on how things work.  For instance,

segList has {x,y}.  insertItemBefore(z,5) - by the spec should append the seg to the list resulting in {x,y,z}.  However, the code in InsertElementAt() results in a call to 

   mSegments.InsertObjectAt(aElement, 5);

At this point, mSegments has only two items in it.  Is this a problem for nsCOMArray ?

	Robert Longson [:longsonr]
	Comment 4 • 17 years ago

What about other implementations of replaceItem in svg content e.g. nsSVGNumberList::ReplaceItem are they affected too?

	Jeff Schiller
	Comment 5 • 17 years ago

Ugh - I wish I would have had time for Bug 282247.  It looks like every list has their own implementation (and some are not even complete like SVGPointList).

It doesn't look like SVGNumberList has this problem.  I did not look at every List implementation yet, sorry.

	Jeff Schiller
	Comment 7 • 17 years ago

I'm attaching another test case that exercises an edge case of SVGPathSegList::replaceItem(newItem, index).  In particular, this case covers when newItem is already a part of the list to which it is being added.

It seems the SVG spec is not clear on what 'index' means - are we to replace the item at that position BEFORE newItem is removed from its current list or the item at that position AFTER newItem is removed.  There is a difference when newItem's current position in the list is less than the value of index.

The test case has a seg list with 5 segments, for simplicity: { a, b, c, d, e }.  We then call replaceItem(b,2).  The test case has the following results:

Opera 9.27: { a, b, d, e }
Safari 3.1: { a, b, d, e }
Batik: { a, c, b, e } 
Firefox 2: replaceItem not implemented
Firefox 3 RC1: { a, c, b, e } 

I have a combined patch that fixes the crash and implements things as Opera and Safari - or I can keep our existing behavior.  The key thing is that the spec needs to be clarified, I guess.

Based on this, should I just update the patch to fix the crash and leave our implementation as is (if the spec is clarified, we can fix it under another bug)?  Or do we want to align with Opera and Safari until told otherwise?

	Jeff Schiller
	Comment 8 • 17 years ago

i.e. the patch is only needed for SVGPathSegList

Note that while we do not pass this test (any of the four sub-tests, in fact) - the browser does not crash.  Neither Safari or Batik pass any of the sub-tests either - only Opera passes.

	Jeff Schiller
	Comment 10 • 17 years ago

Uploaded simple patch to fix crash.  This aligns with Batik implementation for the edge case described above.

Let me know if you want the slightly larger patch instead (that fixes crash and aligns with Opera and WebKit).

	Robert Longson [:longsonr]
	Comment 11 • 17 years ago

Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

>+  // NOTE: the new item can never be the item we will be replacing now that we removed it from its current list beforehand

Nit: Can you fold this to stay within 80 characters please.

General points:

We should ask the svg list what should happen if you do replaceItem(b,3000) where the list contains b but does not have 3000 items in it. Should b disappear from the existing list or not? What do the other implementations do?

I don't think this can make the cut for rc2 which is tomorrow as far as I know so perhaps we should give the svg wg a little more time to respond.

Without response I think that the algorithm you have gone for is reasonable although if Batik does not remove b from the list when confronted with replaceItem(b,3000) then our implementation should not do so either.

	Jeff Schiller
	Comment 12 • 17 years ago

Robert - I've asked Erik Dahlstrom of Opera and the SVG WG on this.  I haven't got an official answer yet from the SVG WG but Erik's personal opinion was yes, b should be removed from the list and then the exception raised.

I believe Batik has the same behavior but I will confirm.

I'll also address the nit later tonight.

	Jeff Schiller
	Comment 13 • 16 years ago

Sorry - I do not have bandwidth to work on this in the short term.  Hopefully someone can pick up where I left off.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 16 years ago

Not going to block 1.9.1 but definitely wanted. It would be good to have mochitest coverage for these APIs.

	Martijn Wargers (dead) Reporter
	Comment 15 • 16 years ago

Still crashes in current trunk build.

	Daniel Veditz [:dveditz]
	Comment 16 • 16 years ago

At the very least it appears to be dereferencing an uninitialized nsSVGPathSeg, and if the index can reach into page-controlled memory this could easily be exploitable.

	Jeff Schiller
	Comment 17 • 16 years ago

I don't have time to work more on this, but the patch should prevent the crashes.  The SVG WG had some action [1] to provide an answer and I'm not sure if they ever officially did [2].

[1] http://www.w3.org/2008/06/19-svg-minutes.html#item07
[2] http://lists.w3.org/Archives/Public/www-svg/2008Aug/0049.html

	Robert Longson [:longsonr]
	Comment 18 • 16 years ago

Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Lets just align ourselves with Batik for now then.

	Robert Longson [:longsonr]
	Comment 19 • 16 years ago

Apologies for the delay.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 20 • 16 years ago

We could use a crashtest here.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 16 years ago

Pushed 6b858b6ab162

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 22 • 16 years ago

Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Simple patch, fixes crash

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 23 • 16 years ago

Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

a191=beltzner; please do get a crashtest added as per roc's request

	Daniel Veditz [:dveditz]
	Comment 24 • 16 years ago

This should be P1 to make sure it lands on 191 in time to make 1907

	chris hofmann
	Comment 25 • 16 years ago

also thowing it on the blocking list, in addition to the wanted list, so it has less chance of  falling off the radar.

	Robert Longson [:longsonr]
	Comment 26 • 16 years ago

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3749974eee0e

	Samuel Sidler (old account; do not CC)
	Comment 27 • 16 years ago

Please don't land a crash test before we ship this in a 1.9.0.x release.

	Daniel Veditz [:dveditz]
	Comment 28 • 16 years ago

Comment on attachment 322727 [details] [diff] [review]
Fix crash - aligns with Batik implementation

Approved for 1.9.0.7, a=dveditz for release-drivers.

	Tony Chung [:tchung]
	Comment 29 • 16 years ago

(In reply to comment #8)
> Created an attachment (id=322723) [details]
> Test case that proves that the crash does not happen with SVGNumberList,
> SVGTransformList, SVGPointList or SVGLengthList.
> 
> i.e. the patch is only needed for SVGPathSegList
> 
> Note that while we do not pass this test (any of the four sub-tests, in fact) -
> the browser does not crash.  Neither Safari or Batik pass any of the sub-tests
> either - only Opera passes.

Noted testcase does not turn green as expected.  Yet the crash in both testcases attached are truly gone.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090130 Minefield/3.2a1pre

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 30 • 16 years ago

Checked into 1.9.0 branch.

	Al Billings [:abillings - ex-MoCo]
	Comment 31 • 16 years ago

The second test case does not give a green circle. As with 3.1 now, the first case does not crash the 1.9.0.7 nightly though. I'm marking this as verified as the issue with the second test case seems to be an open question.

Verified for 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021204 GranParadiso/3.0.7pre.

	Bob Clary [:bc] (inactive)
	Comment 32 • 16 years ago

crash test added
http://hg.mozilla.org/mozilla-central/rev/4f5ce5b9d525