1141788 - nsSHistory uses questionable lifetimes for pointers with GetNext()

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andrew McCreight [:mccr8]]

There's a number of places in nsSHistory that do something like this:

  nsCOMPtr<nsISHTransaction> trans = mListRoot;
  while (trans) {
    ... do some stuff with trans ...

    nsISHTransaction *temp = trans;
    temp->GetNext(getter_AddRefs(trans));
  }

If |trans| had the last reference to the object, then when we assign to |trans| inside of GetNext(), then |this| of |GetNext()| is dead.

As it happens, GetNext() does not look like it touches |this| after setting the return value, and it looks like the places these are being use we are never going to have the last reference to the object, so I think it is safe, but it seems like it would be good to be less fragile here.

A fixed version of the body would look like:
    nsCOMPtr<nsISHTransaction> temp = trans.forget();
    temp->GetNext(getter_AddRefs(trans));

Then we'd be sure to keep |temp| alive for the duration of the getter.  This should not cause any additional refcount traffic.

Olli, do you agree that this code is safe as-is?  Do you think we should change it?

I noticed this while looking at poiru's patch in bug 1129795.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Ancient code, but yeah, looks like it is safe, but we also should fix it to be less
error prone.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Use strong references during iteration in nsSHistory.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5460a481f642

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I feel like I can take this review if you want to hand it off, smaug.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8775805 [details] [diff] [review]
Use strong references during iteration in nsSHistory.

thanks mconley, but I happened to look at the patch before I noticed your comment.

Comment 5

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2882620cd75fe1984479f4d6a9b1982ee95a00e4

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2882620cd75f