Closed Bug 327709 Opened 15 years ago Closed 13 years ago

ASSERTION: unknown path segment

Categories

(Core :: SVG, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jruderman, Unassigned)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 2 obsolete files)

###!!! ASSERTION: unknown path segment: '1==0', file /Users/admin/trunk/mozilla/layout/svg/base/src/nsSVGPathFrame.cpp, line 504
Attached image testcase
Attached 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 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
WFM on trunk.

longsonr, is the patch here valuable?
Status: NEW → RESOLVED
Closed: 13 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.
Attached patch reftest (obsolete) — Splinter Review
Attachment #285859 - Flags: review?(jruderman)
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.
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)
Attachment #285862 - Flags: review?(jruderman) → review+
You need to log in before you can comment on or make changes to this bug.