ASSERTION: unknown path segment

RESOLVED WORKSFORME

Status

()

Core
SVG
RESOLVED WORKSFORME
13 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
PowerPC
Mac OS X
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 212302 [details]
testcase
Created attachment 231736 [details] [diff] [review]
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.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #231736 - Flags: review?(tor)

Comment 3

12 years ago
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.
(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 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.
Attachment #231736 - Attachment is obsolete: true
Attachment #231736 - Flags: review?(tor)
Assignee: longsonr → general
Status: ASSIGNED → NEW
(Reporter)

Comment 6

11 years ago
WFM on trunk.

longsonr, is the patch here valuable?
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME
No, the patch is a bodge. I guess we need to modify the testcase to become a reftest so it does not regress.
Created attachment 285859 [details] [diff] [review]
reftest
Attachment #285859 - Flags: review?(jruderman)
(Reporter)

Comment 9

11 years ago
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.
(Reporter)

Comment 10

11 years ago
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.
Created attachment 285862 [details] [diff] [review]
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.
Attachment #285859 - Attachment is obsolete: true
Attachment #285862 - Flags: review?(jruderman)
Attachment #285859 - Flags: review?(jruderman)
(Reporter)

Updated

11 years ago
Attachment #285862 - Flags: review?(jruderman) → review+
You need to log in before you can comment on or make changes to this bug.