If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.9alpha

Status

Other Applications
DOM Inspector
P1
normal
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

Trunk
mozilla1.9alpha

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 176649 [details]
Testcase
(Reporter)

Comment 2

13 years ago
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).
WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050216
(Reporter)

Comment 4

13 years ago
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.
Keywords: helpwanted

Comment 5

12 years ago
i don't suppose you have a stack/profile/explanation of what's going on?
(Reporter)

Comment 6

12 years ago
Ctrl-C with gdb under mingw doesn't seem to work, so I can't interrupt the program.
Created attachment 220370 [details] [diff] [review]
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?
Flags: blocking1.9a2?
(Reporter)

Comment 8

12 years ago
(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 :(

Created attachment 221994 [details] [diff] [review]
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.
Assignee: dom-inspector → bzbarsky
Attachment #220370 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #221994 - Flags: superreview?(neil)
Attachment #221994 - Flags: review?
Attachment #221994 - Flags: review? → review?(timeless)
Keywords: helpwanted
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Freeze/hang when removing dynamically generated nodes with DOM Inspector → [FIX]Freeze/hang when removing dynamically generated nodes with DOM Inspector
Target Milestone: --- → mozilla1.9alpha

Updated

12 years ago
Attachment #221994 - Flags: review?(timeless) → review+

Comment 10

12 years ago
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));
Attachment #221994 - Flags: superreview?(neil) → superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 12

12 years ago
re-fixed on the layout/inspector sources since this landed in the middle of bug 325100
(Reporter)

Updated

11 years ago
Flags: blocking1.9a2?
(Reporter)

Updated

11 years ago
Depends on: 349398
QA Contact: timeless → dom-inspector
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.
Flags: blocking1.8.1.14?
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.

Updated

10 years ago
Flags: blocking1.8.1.14?
You need to log in before you can comment on or make changes to this bug.