579616 - we leak any element with an XBL binding that outlives its own document

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Andreas's landing of bug 549806 on tracemonkey today exposed a leak in a crashtest (I think it was content/xul/content/crashtests/360078-1.xhtml ).

We still don't know why bug 549806 caused this leak to start happening reliably on this crashtest when it didn't happen before.  However, we do understand the leak itself.

The basic problem is that whenever an element has an XBL binding, we have the following cycle:

  -----------                 ------                     -------
  | element | -> mNodeInfo -> | NI | -> mOwnerManager -> | NIM | -----.
  -----------                 ------                     -------      |
       ^                                                              |
       |                               ------                         |
       `------ mBindingTable (keys) <- | BM | <- mBindingManager <----'
                                       ------

However, in order to allow such elements to be collected before the document goes away, we tell the cycle collector that this entire cycle is a single edge:  an edge from the element to itself, added in this code in the nsGenericElement traverse implementation:

  nsIDocument* ownerDoc = tmp->GetOwnerDoc();
  if (ownerDoc) {
    ownerDoc->BindingManager()->Traverse(tmp, cb);
  }

and unlinked in the nsGenericElement unlink implementation:

    nsIDocument *doc;
    if (!tmp->GetNodeParent() && (doc = tmp->GetOwnerDoc())) {
      doc->BindingManager()->RemovedFromDocument(tmp, doc);
    }


However, when the corresponding document goes away, ~nsDocument calls mNodeInfoManager->DropDocumentReference(), which makes the element's GetOwnerDoc() method return null.  This breaks the above traversal and unlinking code, but it doesn't break any part of the cycle.


I see two possible fixes to this problem:

 (1) make the above traverse and unlink code get the binding manager without going through the document (probably by adding a GetBindingManager() method to either nsINode or nsINodeInfo)

 (2) make nsBindingManager::DropDocumentReference clear mBindingTable (since nothing will be able to access it anymore anyway)

mrbkap has a patch that does (2).  It involved a little bit of complication since the simplest patch actually caused a crash when the clearing of mBindingTable freed elements that wanted to call GetBinding and SetBinding, which recurred into operations on the same hash table and crashed.  Fixing this required a bit of mucking around with mDestroyed (a part of the patch which can probably be cleaned up a little).

Comment 1

[Boris Zbarsky [:bzbarsky]]

I believe we have a patch around which implements solution 3: never null out the owner document (nodes hold a strong ref to their owner document).  That would fix this leak too, right?

Comment 2

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

Yes, I think it would.  We probably need to land something to fix this sooner, though, given that tracemonkey Cd is currently orange because of it.

I'd been thinking that we wanted to do (2) since it made more sense to break links when we knew they were breakable.  However, given that we're going to make nodes keep their documents alive, it probably makes more sense to do (1), especially as a short-term fix.

Comment 3

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

Attachment: patch from mrbkap's tree (requires additional cleanup): clear binding table when binding manager disconnected

Comment 4

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

Except the part that makes (1) hard is that nsBindingManager::RemovedFromDocument wants the old document in order to pass it to nsXBLBinding::ChangeDocument, so I guess we need (2) until we have (3).

Comment 5

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

I think, really, ~nsBindingManager, nsBindingManager::DropDocumentReference, and nsBindingManager's Unlink should probably all do the same thing.  That might be a slightly large change for fixing this, though.

But I think the main substantive change from doing that would be to make DropDocumentReference additionally:
 * set mDestroyed to true permanently, rather than just temporarily
 * clear mWrapperTable, mBindingTable, mDocumentTable, and mLoadingDocTable

(Clearing mBindingTable is the change we need here.)


Does that seem unreasonable?  Or should we just go with mrbkap's patch with the indentation fixed?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated indentation

This has r=sicking over my shoulder.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/93e7e61caa21

Comment 8

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/93e7e61caa21

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/93e7e61caa21