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

Status: RESOLVED WORKSFORME

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

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase makes Firefox leak an nsGenericElement (as shown by trace-refcnt on shutdown).

Comment 1

[Jesse Ruderman]

I'm hitting this leak a lot when I test execCommand.  I tried to debug it but didn't get very far.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

This also leaks on Windows, OS -> All.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - update selection ancestor limiter

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - updated and slightly reorganised

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?

Comment 8

[Peter Van der Beken [:peterv]]

Why do we try to split the br in this case.

Comment 9

[Jesse Ruderman]

WFM, no leaks on trunk.

Comment 10

[Jesse Ruderman]

Crashtest: http://hg.mozilla.org/mozilla-central/rev/e5dd52dd3cb5