1520062 - HTTPChannel seems to incorrectly keep references to its mListener.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Emilio Cobos Álvarez [:emilio]]

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):

https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/netwerk/base/nsIChannel.idl#75

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.

Comment 1

[Andrew McCreight [:mccr8]]

You can reproduce this leak by:

1. Delete

  testing/web-platform/meta/css/css-fonts/font-display/__dir__.ini

2. 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...)

Comment 2

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0370f6c4ba714444615c4bd3fb04e869dbf6f8e7

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Don't exit HttpChannelChild::DoOnStopRequest if call returns error code r=kershaw!

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Also release listeners in HttpChannelChild::ActorDestroy r=kershaw!,mayhemer!

Depends on D16548

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Remove expected WPT leak threshold for /css/css-fonts/font-display/ r=kershaw!

Depends on D16549

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(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

[Andrew McCreight [:mccr8]]

Thanks for the fixes! I wouldn't be surprised if this ends up fixing other intermittent leaks.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Indeed, thanks so much for looking into this Valentin!

Comment 10

[Andrew McCreight [:mccr8]]

If this lands before bug 1519918, you'll have to skip the leak threshold removal patch.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa1fa4a42655565325da3c90b6d9ddc6b3b0f341

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/f10300d5ef4e
https://hg.mozilla.org/mozilla-central/rev/1b6a90a3d820
https://hg.mozilla.org/mozilla-central/rev/01eb1bbef7f2

Comment 14

[Mike de Boer [:mikedeboer]]

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

Comment 15

[Andrew McCreight [:mccr8]]

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).

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(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

[Andrew McCreight [:mccr8]]

(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.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Thanks a lot, that's very helpful.

Comment 20

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71921d19de400f1a00b0b7443816fa65c6407a0b

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Ensure that mOnStartRequestCalled/mOnStopRequestCalled is set just before calling the listener to avoid reentrancy issues r=mayhemer!

Comment 22

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Add more tests for cancelling the channel r=mayhemer!

Depends on D20499

Comment 23

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1520062 - Make httpcancel unit test run in e10s r=mayhemer!

Depends on D20500

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Valentin Gosu [:valentin] (he/him)]

I only landed the first 3 patches and opened bug 1529911 to land the rest of them.

Comment 26

[Razvan Maries]

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

[Razvan Maries]

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 28

[Honza Bambas (:mayhemer)]

Valentin, can you please abandon the remaining revisions? they hang in my "waiting" list.

Comment 29

[Phabricator Automation]

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

[Phabricator Automation]

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

[Phabricator Automation]

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.