350252 - Attribute change causes use clone loop

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jayden]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060825 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060825 Minefield/3.0a1

While working on a test case for MineField I came across this bug.  Attached is an svg page that will crash MineField.  Please don't view with MineField unless you are prepared to have your browser crash.

This does not affect Firefox 1.5.

Reproducible: Always




I tested and this bug does not occur if I use a rect directly and not through a use/defs.

Comment 1

[Jayden]

Attachment: SVG page that crashes MineField. Save page so you can see the source or use Firefox 1.5 to view.

Comment 2

[Steve England [:stevee]]

With Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060825 Minefield/3.0a1 the testcase hangs my firefox (not crash, so I don't get a talkbackid)

Works fine with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060825 BonEcho/2.0b2 ID:2006082504

Comment 3

[Jayden]

(In reply to comment #2)
> With Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060825
> Minefield/3.0a1 the testcase hangs my firefox (not crash, so I don't get a
> talkbackid)
> 
> Works fine with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2)
> Gecko/20060825 BonEcho/2.0b2 ID:2006082504
> 

If you kill the application while it is hung you do have windows send an error report.  I have sent a few of these already, before I posted this bug.  I'm not sure how microsoft gets these bugs to the right people but hopefully they have sent something which can be helpful.

When I mentioned crash I should have been more specific and mention that if you had something in another window it would be gone.  If you have chatzilla up while your browser gets hung chatzilla also be hung.  On my system as well when hung the process uses exactly 50% of my CPU which seems a little strange.  I have an AMD Athlon 64 X2 Dual Core Processor 4400+ (2.2 Ghz) - It isn't using 1 CPU at max it is using both.

Comment 4

[timeless]

http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg

please use the mcsmurf build to get a stack. i certainly don't have access to microsoft's feedback system.

Comment 5

[Jayden]

(In reply to comment #4)
> http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg
> 
> please use the mcsmurf build to get a stack. i certainly don't have access to
> microsoft's feedback system.
> 

I'll do this when I get home from work and post an attachment of what I get.

Comment 6

[tor]

Attachment: minimal testcase

Comment 7

[tor]

Regressed in the past month or so - help tracking down the regression window would be appreciated.  Testcase that was used for bug 343900 development now loops.

Comment 8

[tor]

Attachment: testcase without load - click on rect

Comment 9

[Jayden]

(In reply to comment #8)
> Created an attachment (id=235756) [edit]
> testcase without load - click on rect
> 

The issue I was having happened when you set the "backgroundColor" of the rect not the "fill".  I tried setting the fill and don't remember having a problem with that it was when I tried the backgroundColor (which of course shouldn't work) that I had problems.  I don't have MineField here at work so I don't know if your tests also crash MineField or not.  I'll check them out as well when I get home in just under 6 hours.

Comment 10

[tor]

Regressed between the 2006-08-15-04 and 2006-08-16-04 trunk nightlies.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-08-15+04%3A00%3A00&maxdate=2006-08-16+04%3A00%3A00&cvsroot=%2Fcvsroot

Comment 11

[tor]

Looking through the checkins in the regression window, bug 348573 seems like a likely cause, though the followup patch waiting for review doesn't fix the problem in this case.

Comment 12

[tor]

Backing out bug 348573 fixes this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So the problem is that nsSVGUseElement::AttributeChanged removes and readds the nsSVGUseElement as an observer on mSourceContent.  I don't know why it does this, but this isn't going to work given that the listener array is now iterated forwards.  A simple test shows that the problem existed for the ContentInserted notification even before the patch for bug 348573.  Just replace the line:

  element.setAttribute("fill", "red");

with:

  el2 = document.createElement("rect");
  element.setAttribute("fill", "red");

and try a build from August 14.  The basic problem was introduced in bug 343900, of course.

Now as I see it, there are two ways forward.  Either we document on nsINode to Not Do This with nsIMutationObservers and fix the SVG code accordingly, or document that newly-added observers will not get called (subject to some conditions, etc) and actually do that (probably by keeping track of the "original length" adjusted for removals in the iterator).

Sicking, thoughts?

Comment 14

[tor]

Attachment: check if we're looking at the same element to avoid looping

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 236143 [details] [diff] [review]
check if we're looking at the same element to avoid looping

r/sr=sicking

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If this comes up again I think we should consider making the nsTObserverArray iterator hold a position and an end and adjust both of them when an element is removed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK.  We should still document the current behavior, though, right?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, yes, of course.

Comment 19

[tor]

Checked in.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Docs per comment 17

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in the comments.