814470 - AsyncFetchAndSetIconFromNetwork may leak channel when setup fails

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Nils Maier [:nmaier]]

The AsyncFetchAndSetIconFromNetwork helper tracks the associated channel in the mChannel member. In ::Run, the channel setup may fail in such a way that a leak is created, e.g. when the nsIChannel::AsyncOpen fails, as the channel references AFASIFN and vice versa, creating circular references.
This blocks bug 719180, which changes the jar channel implementation so that ::AsyncOpen there will fail early in some cases (instead of failing in OnStopRequest later). Please note that even without said changes ::AsyncOpen may fail causing a leak in this helper.

Comment 1

[Nils Maier [:nmaier]]

Attachment: Fix a leak when channel setup in AsyncFetchAndSetIconFromNetwork fails

Do not track the channel in the first place, thus avoiding a circular references

Marco, since you seem to have written the original code, can you please review?

Comment 2

[Nils Maier [:nmaier]]

Green try: https://tbpl.mozilla.org/?tree=Try&rev=d823eea1abc0
The patch does not depend on the bug 719180 patches on top.

Comment 3

[Marco Bonardo [:mak]]

Does AsyncOpen ensure the channel stays alive by itself until done? Otherwise, would it be fine to just null mChannel if AsyncOpen fails?

Regarding the change to AsyncFetchAndSetIconFromNetwork::OnStopRequest, in which case we we may reach that point with a success status code, but unable to QI the request to nsIChannel? Note that in any case GetExpirationTimeFromChannel should be capable to handle invalid argument by itself, at a maximum we may add an abort to  ensure doesn't happen.

Regardless all of this, please try to keep changes to a minimum, there's no need to do all of that "cleanup" (like the NS_ENSURE_SUCCESS replacements, or moving nsresult) just to fix this bug.

Comment 4

[Nils Maier [:nmaier]]

(In reply to Marco Bonardo [:mak] from comment #3)
> Does AsyncOpen ensure the channel stays alive by itself until done?

AFAIK, yes. As long as AsyncOpen succeeded, something will always strong-ref the channel until OnStopRequest.

> Regarding the change to AsyncFetchAndSetIconFromNetwork::OnStopRequest, in
> which case we we may reach that point with a success status code, but unable
> to QI the request to nsIChannel?

This should always QI to nsIChannel, yes. MOZ_ASSERT would work as well to make sure of that.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 684460 [details] [diff] [review]
Fix a leak when channel setup in AsyncFetchAndSetIconFromNetwork fails

Review of attachment 684460 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, so I'd just like to get a minimal patch without code polish

Comment 6

[Nils Maier [:nmaier]]

Attachment: Bug 814470: Fix a leak when channel setup in AsyncFetchAndSetIconFromNetwork fails

Comments addressed

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 685287 [details] [diff] [review]
Bug 814470: Fix a leak when channel setup in AsyncFetchAndSetIconFromNetwork fails

Review of attachment 685287 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good, thank you.

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +565,5 @@
>    nsCOMPtr<nsIURI> iconURI;
>    nsresult rv = NS_NewURI(getter_AddRefs(iconURI), mIcon.spec);
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIChannel> channel;

nit: the added empty like doesn't look needed here

Comment 8

[Nils Maier [:nmaier]]

Attachment: For checkin, r=mak

Same patch, sans that newline.

Green try: https://tbpl.mozilla.org/?tree=Try&rev=240962f65a48

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85334460fd29

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85334460fd29

Comment 11

[Jason Duell]

Comment on attachment 685574 [details] [diff] [review]
For checkin, r=mak

See bug 719180 comment 43

Comment 12

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e152108b36c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6ab847ed3a8e