Closed Bug 407072 Opened 12 years ago Closed 10 years ago

Leak nsGenericElement and more with execCommand("insertimage", ...)

Categories

(Core :: DOM: Editor, defect, P3)

x86
All
defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jruderman, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase)

Attachments

(2 files, 1 obsolete file)

Attached file testcase
Loading the testcase makes Firefox leak an nsGenericElement (as shown by trace-refcnt on shutdown).
I'm hitting this leak a lot when I test execCommand.  I tried to debug it but didn't get very far.
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
This also leaks on Windows, OS -> All.
OS: Mac OS X → All
Ok, I think that part of the problem is that 3 of the 21 transactions that are executed during startup plus this test case are failing.

They fail in nsEditor::SplitNodeImpl(), the call to selection->Collapse() is failing after we split a node. This is because the ancestor limiter of the selection is the right-hand half of the node that was split, and the selection is being collapsed to the left-hand half of the split node. The left-hand-half is not a descendant of the right-hand-side so IsValidSelectionPoint() in Collapse() decides that it's not fit to be a selection point and fails.

So the solution to that is to reset the ancestor limiter to null before collapsing the selection. If I do this, the leak goes away.

I think that the reason this bug never showed up before, is that this case only gets hit when you split a leaf node which is its own ancestor limiter.

Taking bug. I'll investigate some other warnings and failed assertions before submitting patch. I'll see if I can figure out what was leaking in the first place too.
Assignee: nobody → chris
(In reply to comment #3)
> So the solution to that is to reset the ancestor limiter to null before
> collapsing the selection. If I do this, the leak goes away.

Actually, the problem here is that the selection ancestor limiter is limited to the contenteditable root. The original code assumes that when a node is cloned, its selection ancestor limiter is an ancestor of the node. So the actual solution is to change SplitNodeImpl() so that when cloning a node which is its own ancestor limiter, the clone is also its own ancestor limiter - not the original node. That way selection::Collapse() will then work, as the node's ancestor limiter will be actually in its ancestor chain.
This patch:

* When we create new nodes and set the selection to be in the new nodes, we need to update the selection ancestor limiter to be an ancestor of the new node, otherwise IsValidSelectionPoint() will fail. Prevents transactions failures and memory leak.
* Reordered nsEditor::InsertContainerAbove() so that it adds the new container to the tree before adding the node into its new container. This prevents a range update failing in a change listener, stopping another warning from firing.
* Changed nsHTMLEditRules::PinSelectionToNewBlock() to only ask for the last editable child of the new block if the new block actually has children. It now just takes the block instead if it has no children. This prevents us bailing early and stops a transaction failing.
Attachment #292993 - Flags: review?(peterv)
Ho hum, the change to nsHTMLEditRules::PinSelectionToNewBlock() in my patch also fixes bug 404682, "Gmail compose "quote" function broken". The same problem is being hit there, and it is causing the insertParagraph command to bail without finishing properly. Marking this blocking bug 404682.
Blocks: 404682
Blocks: 398292
This is pretty much the same as previous, but it's updated to current trunk, and moves FindEditableRoot into a static function in nsEditor (was a virtual function in nsEditor previously, requiring a cast). Also normalizes line endings in nsEditorEventListeners.cpp, change in endings was introduced recently.

PeterV, can you review please?
Attachment #292993 - Attachment is obsolete: true
Attachment #298356 - Flags: review?(peterv)
Attachment #292993 - Flags: review?(peterv)
Why do we try to split the br in this case.
Flags: wanted-next+
Flags: blocking1.9-
Flags: tracking1.9+
Attachment #298356 - Flags: review?(peterv)
WFM, no leaks on trunk.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Crashtest: http://hg.mozilla.org/mozilla-central/rev/e5dd52dd3cb5
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.