318379 - Firefox 1.5 should not crash when viewing an SVG file, or a page embedding that file

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[djn]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Firefox 1.5 crashes when an attempt to view the attached SVG file is made.

Reproducible: Always

Steps to Reproduce:
1. Save the attached SVG file to your machine.
2. Open SVG file in Firefox.
3. Observe crash.

Actual Results:  
Firefox crashes.

Expected Results:  
SVG file should be shown.

This SVG file views fine with the Adobe SVG Viewer 3 and 6 beta. The crash also occurs when the SVG file is embedded within an HTML page using <object> or <iframe>.

Comment 1

[djn]

Attachment: Viewing this SVG file crashes Firefox 1.5

Viewing this SVG file crashes Firefox 1.5.

Comment 2

[djn]

Removing the <defs> section from the file stops the crash from happening. Needless to say, this isn't terribly helpful, as we need that for additional visual effects when the Adobe SVG viewer is used to view the content.

Comment 3

[djn]

I've submitted some Talkbacks for this.

Comment 4

[djn]

Commenting out the <marker id='lineSeries1'...> definition in <defs> stops the crash. Unfortunately, this is critical to our application.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Can you provide the submitted Talkback IDs?

Comment 6

[djn]

I'd be happy to, but how do I determine the Talkback IDs? They're not immediately apparent when submitting the report...

Comment 7

[djn]

I notice that the attached SVG file has a <path> element with the 'd' attribute set to an empty string (obviously not sensible). Commenting out that element eliminates the crash, so this looks like it could be poor error handling of an unexpectedly empty 'd' attribute on the <path> element.

Comment 8

[djn]

Yes -- if I provide path data in the (formerly empty) 'd' attribute on the last path element in the attached file, the crash is eliminated. That looks like I can easily avoid the crash now.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

You need to find the talkback executable and launch it, it should display a list of IDs that can be copied into the bug. For Firefox on windows the default location is:

C:\Program Files\Mozilla Firefox\extensions\[e-mail]\components\talkback.exe

Comment 10

[djn]

Aha, thank you for that. The ids are:

TB12435786M, TB12434697M, TB12434574Q, TB12434380X, TB12434269H

Comment 11

[tor]

Attachment: fix GetCoveredRegion like Render

Comment 12

[Reed Loden [:reed]]

(In reply to comment #11)
> Created an attachment (id=204587) [edit]
> fix GetCoveredRegion like Render

I'm not a reviewer myself, but I couldn't help noticing this.

If every if statement requires num, why not either:
1) if (!num) return region;
2) if (num) { ... other if statements ... }

I just like to keep stuff optimized to the greatest extent when I code, and this was just bugging me. :P

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 204587 [details] [diff] [review]
fix GetCoveredRegion like Render

r=me

Comment 14

[tor]

Checked in on trunk.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using trunk SeaMonkey build 2005-12-03-07 on Windows XP with https://bugzilla.mozilla.org/attachment.cgi?id=204577&action=view as my testcase.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 204587 [details] [diff] [review]
fix GetCoveredRegion like Render

Reed's right, this patch should be minimized.  Please do so and renominate.

/be

Comment 17

[Martijn Wargers (dead)]

*** Bug 332378 has been marked as a duplicate of this bug. ***

Comment 18

[Mike Schroepfer]

Anyone up for a 1.8.1 patch with be's comments addressed?

Comment 19

[Mike Shaver (:shaver emeritus)]

Is this still not fixed on the 1.8.0.x branch either?

Comment 20

[Martijn Wargers (dead)]

Attachment: patch for trunk

I've only tested that it compiles, but I'm basically only doing what is suggested in comment 12.

Comment 21

[Martijn Wargers (dead)]

Comment on attachment 227507 [details] [diff] [review]
patch for trunk

oops, no, that's a different function. Apparently, there are more places that could be optimised, I guess.

Comment 22

[Martijn Wargers (dead)]

So apparently this was already checked into the 1.8.1 tree:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/svg/base/src/nsSVGPathGeometryFrame.cpp&rev=&root=/cvsroot&rev=MOZILLA_1_8_BRANCH#350
as part of bug 339220.
If this is wanted for the 1.8.0.x branch, I guess someone should ask for a blocking 1.8.0.6 flag.
A patch should be easy to make, I think.

Comment 23

[Martijn Wargers (dead)]

Attachment: patch for 1.8.0.7 branch

Comment 24

[Martijn Wargers (dead)]

Attachment: diff -w version of patch

Comment 25

[Martijn Wargers (dead)]

The patch compiles, but I can't check if the patch fixes the crash on 1.8.0.x branch, since I don't have a build that actually supports svg, but it should fix the crash.

Comment 26

[tor]

This latter patch looks similar to bug 346673, which has been checked in on the 1.8 branch.  Could you check if there is still a problem there?

Comment 27

[Martijn Wargers (dead)]

(In reply to comment #26)
> This latter patch looks similar to bug 346673, which has been checked in on the
> 1.8 branch.  Could you check if there is still a problem there?

You mean the patch in that bug would also fix this bug?
That bug is still a problem in the 1.8.0.x tree, because it hasn't been checked in there yet.

Comment 28

[Martijn Wargers (dead)]

Comment on attachment 232116 [details] [diff] [review]
patch for 1.8.0.7 branch

So I guess the patch in bug 346673 will fix it on the 1.8.0.x branch.

Comment 29

[Daniel Veditz [:dveditz]]

To be fixed by bug 346673