407818 - Leak nsGenericDOMDataNode with <ol>, <li>, execCommand("selectall")

Status: RESOLVED FIXED

(Core :: DOM: Serializers, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Loading the testcase makes Firefox leak an nsGenericDOMDataNode and some other stuff (as shown by trace-refcnt).  Patch coming up.

Comment 1

[Jesse Ruderman]

Attachment: patch

The leak was due to
  currNode->GetPreviousSibling(&currNode);
because GetPreviousSibling addrefs before returning and currNode was a raw pointer.

This patch makes currNode be an nsCOMPtr. It also introduces a temporary because smaug and NeilAway say it's bad luck to do 
  obj->GetX(getter_AddRefs(obj));
(I guess you could lose your owning reference to the object before its method returns.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 292501 [details] [diff] [review]
patch

Two suggestions:

1) get rid of the variable |node| and just leave currNode -- initialize it the way node is now initialized.

2) move the declaration of |tmp| to one line above its first use.  Then it'll go out of scope much faster (less code generated for calling its destructor in various places) and hold unneeded references for less time.

r+sr=dbaron with that

Comment 3

[Jesse Ruderman]

Attachment: patch addressing dbaron's review comments

Comment 4

[Jesse Ruderman]

Fix checked in.

Comment 5

[Jesse Ruderman]

Crashtest checked in.