327709 - ASSERTION: unknown path segment

Status: RESOLVED WORKSFORME

(Core :: SVG, defect)

Description

[Jesse Ruderman]

###!!! ASSERTION: unknown path segment: '1==0', file /Users/admin/trunk/mozilla/layout/svg/base/src/nsSVGPathFrame.cpp, line 504

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Robert Longson [:longsonr]]

Attachment: patch

check item is valid before adding it in the first place and also avoid going into an infinite loop if something does go wrong.

Comment 3

[tor]

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

> NS_IMETHODIMP nsSVGPathSegList::Initialize(nsIDOMSVGPathSeg *newItem,
>                                            nsIDOMSVGPathSeg **_retval)
> {
>+  nsISVGValue *val;
>+  CallQueryInterface(newItem, &val);
>+  if (!val) {
>+    *_retval = nsnull;
>+    return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+  }

Why this check?  All pathsegs currently implement nsISVGValue (though that will be going away in bug 345085), so this seems redundant.

> NS_IMETHODIMP nsSVGPathSegList::AppendItem(nsIDOMSVGPathSeg *newItem,
>                                            nsIDOMSVGPathSeg **_retval)
> {
>+  nsISVGValue *val;
>+  CallQueryInterface(newItem, &val);
>+  if (!val) {
>+    *_retval = nsnull;
>     return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
>+  }

Likewise.

Comment 4

[Robert Longson [:longsonr]]

(In reply to comment #3)
> 
> Why this check?  All pathsegs currently implement nsISVGValue (though that will
> be going away in bug 345085), so this seems redundant.

It looks to me as if the testcase creates a pathseg which does not implement nsISVGValue. Should the javascript engine actually allow this syntax?

Comment 5

[Robert Longson [:longsonr]]

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

probably not the right way to fix it and I don't want to interfere with bug 345085.

Comment 6

[Jesse Ruderman]

WFM on trunk.

longsonr, is the patch here valuable?

Comment 7

[Robert Longson [:longsonr]]

No, the patch is a bodge. I guess we need to modify the testcase to become a reftest so it does not regress.

Comment 8

[Robert Longson [:longsonr]]

Attachment: reftest

Comment 9

[Jesse Ruderman]

Please add "var" so the test doesn't cause a js strict warning.

This test should go in the "just load the page and make sure nothing bad happens" test suite (which will exist soon) rather than reftest.  See bug 397725 and http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/3cac080dde1e46e7/89e95144ce57cc30#89e95144ce57cc30.

Comment 10

[Jesse Ruderman]

The appendItem() call should probably be wrapped in a try/catch, assuming that doesn't stop the testcase from triggering the assertion in old builds.

Comment 11

[Robert Longson [:longsonr]]

Attachment: address some review comments

I've made the var change and changed the tests to load tests so that it will be easier to spot these tests need to be moved when the "just load the page and make sure nothing bad happens" test suite is created.

I'm hesitant to add the try/catch as I don't have any old debug builds to test with and I don't want to check in a test that doesn't trigger the assertion in old builds.

Comment 12

[Robert Longson [:longsonr]]

reftest checked in.

http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/bugs/bug327709.svg