ASSERTION: unknown path segment

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
14 years ago
12 years ago

People

(Reporter: jruderman, Unassigned)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
PowerPC
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

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

Comment 1

14 years ago
Posted image testcase
Posted patch patch (obsolete) — Splinter Review
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

13 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

12 years ago
WFM on trunk.

longsonr, is the patch here valuable?
Status: NEW → RESOLVED
Closed: 12 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.
Posted patch reftest (obsolete) — Splinter Review
Attachment #285859 - Flags: review?(jruderman)
Reporter

Comment 9

12 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

12 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.
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

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