Closed Bug 1203423 Opened 4 years ago Closed 4 years ago

nsMutationReceiver::nsMutationReceiver(nsINode*t, nsMutationReceiverBase*) calls addref before derived classes constructors have run

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file)

We currently have the following inheritance chain:

    nsISupports
          ↑
 nsIMutationObserver
          ↑
 nsIAnimationObserver
          ↑
nsStubAnimationObserver
          ↑
nsMutationReceiverBase
          ↑
  nsMutationReceiver
          ↑
 nsAnimationReceiver

At the bottom three levels we have two constructor signatures:

a) nsINode* aTarget, nsDOMMutationObserver* aObserver
b) nsINode* aRegisterTarget, nsMutationReceiverBase* aParent

When we call constructor (b) of nsAnimationReceiver, we call constructor (b)
for nsMutationObserver which in turn calls:

  aParent->AddClone(this);

nsMutationReceiverBase::AddClone includes the following line:

  mTransientReceivers.AppendObject(aClone);

This will cause us to add-ref aClone but by this point aClone is not yet
an nsAnimationReceiver hence the call to AddRef will not run the
nsAnimationReceiver::AddRef override, but will nsMutationReceiver::AddRef
instead.

As a result, in nsTraceRefcnt, we will end up logging a create against
nsMutationReceiver but not nsAnimationReceiver. When we produce the bloatview
at the end we'll report a negative leak for nsAnimationReceiver.

Note that is only occurs with the second form of the constructor which
isn't triggered in any of our existing tests for animation observers. I've
come across it when dealing with document trees since in that case we
end up calling nsMutationReceiver::ContentRemoved which triggers the
second form of nsAnimationReceiver::Create.

A reduced test case that, when dropped into test_animation_observers.html,
triggers this for me is:

  var child = document.createElement("div");
  div.appendChild(child);
  child.style = "animation: anim 100s";

  var childAnimations = child.getAnimations();

  yield await_frame();
  assert_records([{ added: childAnimations, changed: [], removed: [] }], "");

  child.remove();

I'm not really familiar with this code but I guess a couple of possibilities
would be:

1) Rework the hierarchy like so:

                  ⋮
        nsMutationReceiverBase
            ↗           ↖
nsMutationReceiver   nsAnimationReceiver

Then call AddClone from each ctor individually. They might be a lot of work
though.

2) Move the call to AddClone to the appropriate form of
   {nsMutationReceiver,nsAnimationReceiver}::Create.

I suspect (2) is going to be easier and safer since (1) doesn't really help if
we later subclass either of the receivers. We should probably just not call
addref in the ctor of anything that might possibly be subclassed later.
This is to avoid performing an AddRef on a potentially derived object
where the subclass constructors have yet to run.
Attachment #8659087 - Flags: review?(bugs)
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attachment #8659087 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/23b49acf7cc0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.