HttpChannelChild::Release() leaks in the DoNotifyListener case
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr78 | --- | unaffected |
firefox83 | --- | wontfix |
firefox84 | --- | wontfix |
firefox85 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: memory-leak, Whiteboard: [necko-triaged])
Attachments
(1 file)
While investigating the leak in bug 1654651, I noticed that PHttpChannelChild is being listed as leaking but HttpChannelChild is not. That means that we told leakcheck that the refcount of every HttpChannelChild dropped to zero, but never actually called the dtor for HttpChannelChild (which would cause MOZ_COUNT_DTOR to be called for PHttpChannelChild). We don't seem to be hitting any dispatch failures, judging from the log.
Reading over the code for HttpChannelChild::Release() I found one possible cause for this.
Imagine we're on the main thread, with a refcount of 1, and (mOnStartRequestCalled && mOnStopRequestCalled) || !mListener)
is false, and we're calling release.
First, we decrement the refcount of the object to 0.
Then we call NS_LOG_RELEASE(this, 0, "HttpChannelChild");
. The way leakcheck works in Mochitests by default, it interprets this to mean that a HttpChannelChild has been destroyed, so it decrements its count for that.
Now we stabilize the refcount by setting it to 1.
Next we check if we can immediately call delete, but we fall the check as I described above.
Now we create a new runnable to do DoNotifyListener, which has an owning reference to this
. The creation of the owning reference causes our refcount to increase to 2. The leakchecker in this mode only cares about calls to NS_LogAddRef when the refcount is 1, so the leakchecker ignores it.
If the Dispatch() had failed, then we call Release() to get rid of the reference from stabilization, but because it succeeds we just never do anything to clean it up. DoNotifyListener will run later, but we still have our phantom reference from stabilization. The runnable will go away and call release to drop the refcount back down to 1, so the leakchecker will ignore it and HttpChannelChild::Release() won't destroy the object, causing us to leak the child.
That said, this should still end up calling ReleaseListeners(), so I'm not sure if this can explain the leak.
Maybe Fission could cause this problem to happen much more frequently because we end up killing processes quickly before they can process OnStop or something?
In addition to the leak issue, NS_LOG_RELEASE should probably only be called with a count of 0 when we decide to actually destroy the object, though in this case it led me to find an issue.
I'm also not sure how well the two cases of dispatch failing in Release() will actually work. It might be better to just leak (with a warning in the log) and give up.
Assignee | ||
Comment 1•4 years ago
|
||
I did some bc fis try runs and there were no leaks, but that might not mean anything:
https://treeherder.mozilla.org/jobs?repo=try&revision=27850f8645c6f0239f5a074857b737c72176e27a
Now I'm waiting for a full Linux debug try run:
https://treeherder.mozilla.org/jobs?repo=try&revision=24899050d29ba88c54a653ac17769260a87a288c
Assignee | ||
Comment 2•4 years ago
|
||
If there is still a listener remaining when the reference count drops to 0,
HttpChannelChild::Release() dispatches a runnable to call DoNotifyListener.
This runnable will get an additional reference to the channel.
However, stabilization has already occurred, even though we decided to
not destroy the object. We need to take care to release that reference
in order to prevent a leak.
Along similar lines, we should not call NS_LOG_RELEASE with 0 as the
refcount in this case, because the refcount is not yet 0. In order to
deal with this, I pushed the single NS_LOG_RELEASE into the various
cases and then changed the way it worked for the DoNotifyListener case.
Updated•4 years ago
|
Comment 4•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Description
•