285204 - [FIX]Freeze/hang when removing dynamically generated nodes with DOM Inspector

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
When following the steps in the testcase, I get a freeze/hang of Mozilla and I
need to shut down Mozilla with the Task Manager.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Uhm, the steps to reproduce are specific for the Firefox DOM Inspector, but it
should be quite easy to reproduce also in Mozilla's DOM Inspector (I've already
verified that it freezes the browser also there).

Comment 3

[Alex Vincent [:WeirdAl]]

WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050216

Comment 4

[Martijn Wargers (dead)]

Maybe I was a bit unclear with step 2, it says:
2. Unfold the DOM tree till the body element.
With that I mean that the body element also must be unfolded.

Comment 5

[timeless]

i don't suppose you have a stack/profile/explanation of what's going on?

Comment 6

[Martijn Wargers (dead)]

Ctrl-C with gdb under mingw doesn't seem to work, so I can't interrupt the program.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial patch

This fixes two issues:

1)  The ContentAppended handler was broken; with this patch we see all three of
    the <span> nodes in the tree.
2)  inDOMView::GetParentIndex didn't deal well with not finding a parent.

With this patch we still throw from inDOMView::GetParentIndex, while looking for the parent for a <span>.  The parent pointer on said span looks pretty bogus to me...

Martijn, do you want to try figuring out what's going on with that part?  Or should I try to look into it?

Comment 8

[Martijn Wargers (dead)]

(In reply to comment #7)
> Martijn, do you want to try figuring out what's going on with that part?  Or

I wish I was able to do that. But my debugging skills are just too poor :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

I fixed an assert and a few other minor issues while I was here.

The substantive fix for this patch is the fix to ContentAppended to handle more than one node and the fix to ContentInserted to properly deal with insertion of kids of nodes corresponding to closed rows.

The change to ContentRemoved() to pass the right number to RowCountChanged() fixes an assert if an open row with kids is removed.  The change to invalidate the parent if it's no longer a container hides the twisty on things all of whose kids get removed.

The rest is minor safety cleanup.

Comment 10

[[e-mail]]

Comment on attachment 221994 [details] [diff] [review]
Patch

Irrelevant side note: I wonder whether Expand/CollapseNode should return the number of rows changed (e.g. ToggleOpenState calls RowCountChanged(index + 1, node->isOpen ? CollapseNode(index) : ExpandNode(index));

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 12

[Benjamin Smedberg]

re-fixed on the layout/inspector sources since this landed in the middle of bug 325100

Comment 13

[Alex Vincent [:WeirdAl]]

I keep hitting this on 1.8.1 branch, while inspecting nodes on Google Maps (send e-mail link near upper right corner, inspecting the DOM nodes that are on the page); looks like this patch never landed there.  Requesting blocking1.8.14 to see if we can get this in.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Requesting blocking is the wrong way to go about that.  The right way is to take the patch, apply it to 1.8, fix any merge issues, test it to make sure it works, then post a diff against 1.8 and request approval to check it in.  Or review before approval if you had to make changes to the patch.

I'm not going to be doing the above, so if you want this fixed you'll need to find someone who will.