HTTPChannel seems to incorrectly keep references to its mListener.
Categories
(Core :: Networking, defect, P2)
Tracking
()
People
(Reporter: emilio, Assigned: valentin)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P2][necko-triaged])
Attachments
(3 files, 3 obsolete files)
See https://bug1519918.bmoattachments.org/attachment.cgi?id=9036478 and such.
We seem to be leaking due to a circular reference between nsFontFaceLoader::mChannel and the channel's mListener.
It is my understanding that it should not happen if the channel is done with the listener (it's canceled and such):
The second patch in the bug ensures the channels are properly canceled, but without the first (that breaks the reference on the listener's side) we still get leaks like the one linked above.
Updated•5 years ago
|
Comment 1•5 years ago
|
||
You can reproduce this leak by:
- Delete
testing/web-platform/meta/css/css-fonts/font-display/__dir__.ini
- Run
./mach wpt css/css-fonts/font-display/ > bloat.txt; grep UNEXPECTED-FAIL bloat.txt
If you see any output that talks about leaking bytes, you've hit the leak. It reproduces around 1/7 times for me. (At least, the full leak did. I assume the new leak is similar but maybe not...)
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
We should call ReleaseListeners in HttpChannelChild::DoOnStopRequest, but I see there's a path out of that method that doesn't call it:
https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/netwerk/protocol/http/HttpChannelChild.cpp#1189-1208
This would happen for failed tracking channels, and the cycle would never get broken.
Assignee | ||
Comment 3•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0370f6c4ba714444615c4bd3fb04e869dbf6f8e7
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D16548
Assignee | ||
Comment 6•5 years ago
|
||
Depends on D16549
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #2)
We should call ReleaseListeners in HttpChannelChild::DoOnStopRequest, but I see there's a path out of that method that doesn't call it:
https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/netwerk/protocol/http/HttpChannelChild.cpp#1189-1208
This would happen for failed tracking channels, and the cycle would never get broken.
This wasn't the issue, but I added a patch for it just in case.
The reason for the leak was that the window/process was closed before the channel completed, and the call to ActorDestroy did not clean up all the listeners, so we were left with a cycle.
Comment 8•5 years ago
|
||
Thanks for the fixes! I wouldn't be surprised if this ends up fixing other intermittent leaks.
Reporter | ||
Comment 9•5 years ago
|
||
Indeed, thanks so much for looking into this Valentin!
Comment 10•5 years ago
|
||
If this lands before bug 1519918, you'll have to skip the leak threshold removal patch.
Assignee | ||
Comment 11•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa1fa4a42655565325da3c90b6d9ddc6b3b0f341
Comment 12•5 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f10300d5ef4e Don't exit HttpChannelChild::DoOnStopRequest if call returns error code r=kershaw https://hg.mozilla.org/integration/autoland/rev/1b6a90a3d820 Also release listeners in HttpChannelChild::ActorDestroy r=kershaw https://hg.mozilla.org/integration/autoland/rev/01eb1bbef7f2 Remove expected WPT leak threshold for /css/css-fonts/font-display/ r=kershaw
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f10300d5ef4e
https://hg.mozilla.org/mozilla-central/rev/1b6a90a3d820
https://hg.mozilla.org/mozilla-central/rev/01eb1bbef7f2
Updated•5 years ago
|
Comment 14•5 years ago
|
||
https://hg.mozilla.org/projects/cedar/rev/f10300d5ef4e95aac0aeb495cd9cf80aa5d11521 Bug 1520062 - Don't exit HttpChannelChild::DoOnStopRequest if call returns error code r=kershaw https://hg.mozilla.org/projects/cedar/rev/1b6a90a3d8207f9dc9d4716a4d7866c3fd1c0605 Bug 1520062 - Also release listeners in HttpChannelChild::ActorDestroy r=kershaw https://hg.mozilla.org/projects/cedar/rev/01eb1bbef7f25a6988b092cc86f0a85dca7c37f4 Bug 1520062 - Remove expected WPT leak threshold for /css/css-fonts/font-display/ r=kershaw
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 15•5 years ago
|
||
Valentin, is there any chance that whatever was causing issues here can be fixed and this can be relanded? This seems to fix at least some variation of a frequent intermittent leak on TreeHerder (bug 1486101).
Assignee | ||
Comment 17•5 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #15)
Valentin, is there any chance that whatever was causing issues here can be fixed and this can be relanded? This seems to fix at least some variation of a frequent intermittent leak on TreeHerder (bug 1486101).
Yes, I'll try to fix the patch and reland it.
The only problem is that I can no longer reproduce the leak (probably because bug 1519918 landed), so the codepath is much harder to exercise now :)
But in any case I'll do my best to land it.
Comment 18•5 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #17)
The only problem is that I can no longer reproduce the leak (probably because bug 1519918 landed), so the codepath is much harder to exercise now :)
Thanks! Bug 1486101 comment 99 has a patch and STR to hit the leak, though it can take a couple of tries.
Assignee | ||
Comment 19•5 years ago
|
||
Thanks a lot, that's very helpful.
Assignee | ||
Comment 20•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71921d19de400f1a00b0b7443816fa65c6407a0b
Assignee | ||
Comment 21•5 years ago
|
||
Assignee | ||
Comment 22•5 years ago
|
||
Depends on D20499
Assignee | ||
Comment 23•5 years ago
|
||
Depends on D20500
Updated•5 years ago
|
Comment 24•5 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a810eb07eca7 Don't exit HttpChannelChild::DoOnStopRequest if call returns error code r=kershaw https://hg.mozilla.org/integration/autoland/rev/5c93feb48a68 Also release listeners in HttpChannelChild::ActorDestroy r=kershaw https://hg.mozilla.org/integration/autoland/rev/268454ace73e Remove expected WPT leak threshold for /css/css-fonts/font-display/ r=kershaw
Assignee | ||
Comment 25•5 years ago
|
||
I only landed the first 3 patches and opened bug 1529911 to land the rest of them.
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a810eb07eca7
https://hg.mozilla.org/mozilla-central/rev/5c93feb48a68
https://hg.mozilla.org/mozilla-central/rev/268454ace73e
Comment 27•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 28•5 years ago
|
||
Valentin, can you please abandon the remaining revisions? they hang in my "waiting" list.
Comment 29•5 years ago
|
||
Comment on attachment 9045275 [details]
Bug 1520062 - Make httpcancel unit test run in e10s r=mayhemer!
Revision D20501 was moved to bug 1529911. Setting attachment 9045275 [details] to obsolete.
Comment 30•5 years ago
|
||
Comment on attachment 9045272 [details]
Bug 1520062 - Add more tests for cancelling the channel r=mayhemer!
Revision D20500 was moved to bug 1529911. Setting attachment 9045272 [details] to obsolete.
Comment 31•5 years ago
|
||
Comment on attachment 9045271 [details]
Bug 1520062 - Ensure that mOnStartRequestCalled/mOnStopRequestCalled is set just before calling the listener to avoid reentrancy issues r=mayhemer!
Revision D20499 was moved to bug 1529911. Setting attachment 9045271 [details] to obsolete.
Description
•