1341673 - More leaks in mixed-content tests due to HttpChannelChild not dropping callback refs on redirect (e10s leak)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[James Graham [:jgraham]]

https://treeherder.mozilla.org/logviewer.html#?job_id=79384289&repo=try&lineNumber=27163

Leaked URLs:
[task 2017-02-22T15:36:46.535323Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/allowed/http-csp/same-host-https/link-prefetch-tag/top-level/keep-scheme-redirect/allowed.https.html
[task 2017-02-22T15:36:46.535880Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/allowed/http-csp/same-host-https/link-prefetch-tag/top-level/keep-scheme-redirect/allowed.https.html
[task 2017-02-22T15:36:46.536421Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=f8221689-431f-42c3-938b-94ea9e1951a9&path=%2Fmixed-content&content_type=text/html
[task 2017-02-22T15:36:46.536809Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/allowed/http-csp/same-host-https/link-prefetch-tag/top-level/keep-scheme-redirect/allowed.https.html
[task 2017-02-22T15:36:46.537350Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/generic/expect.py?action=purge&content_type=text%2Fhtml&key=f8221689-431f-42c3-938b-94ea9e1951a9&path=%2Fmixed-content
[task 2017-02-22T15:36:46.537845Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=f8221689-431f-42c3-938b-94ea9e1951a9&path=%2Fmixed-content&content_type=text/html
[task 2017-02-22T15:36:46.538360Z] 15:36:46     INFO - PROCESS | 4977 |   https://web-platform.test:8443/mixed-content/allowed/http-csp/same-host-https/link-prefetch-tag/top-level/keep-scheme-redirect/allowed.https.html

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Is this an error in the implementation of the mixed-content handler or the tests?

Comment 2

[James Graham [:jgraham]]

It at least is expected to be a gecko issue since content APIs shouldn't be able to cause leaks of the kind the leak detector will flag.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, I poked at this for a bit today, just loading http://w3c-test.org/mixed-content/allowed/http-csp/same-host-https/link-prefetch-tag/top-level/keep-scheme-redirect/allowed.https.html in a debug build.

Some observations:

1)  A bunch of necko things are leaking, and so is nsPrefetchNode, but that's about it.
2)  The leak is e10s-only, afaict.
3)  Bug 1354220 is a thing, but my attempt to fix it does not fix the leak here.
4)  Passing nullptr as the callbacks argument to NS_NewChannelInternal in
    nsPrefetchNode::OpenChannel does not fix the leak.
5)  Nulling out mChannel and returning error right before calling AsyncOpen2
    in nsPrefetchNode::OpenChannel fixes the leak.
6)  Nulling out mChannel in nsPrefetchNode::OnStopRequest fixes the leak.

So maybe bug 1354220 is all that's going on here and I just didn't fix it right, or maybe the channel is holding on to the prefetch node in some other way still.  Either way, that seems to be the cycle: prefetch node to its mChannel and then through the channel back to the prefetch node.  Necko channels are supposed to drop backrefs OnStopRequest, right?

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, so I have at least part of the puzzle.  The testcase does a _redirect_.  And if we land in HttpChannelChild::Redirect3Complete, we will hand a ref to mListener and mListenerContext over to mRedirectChannelChild... but then what?  Where will we null out our own mListener, etc?

The answer seems to be "nowhere".  If I add a ReleaseListeners() call to Redirect3Complete, the leak goes away.

nsHttpChannel handles this correctly.  It calls ReleaseListeners from nsHttpChannel::OpenRedirectChannel.  So non-e10s doesn't have the leak.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This doesn't bite other obvious cases, like XHR, because those update their channel pointer on redirect, and hence have no ref to the pre-redirect channel anymore, breaking the cycle...

That said nsPrefetchNode looks to me like it also updates its mChannel; see nsPrefetchNode::OnRedirectResult.  So I'm not sure what the story there is; going to stop debugging this now.

Comment 6

[Andrew McCreight [:mccr8]]

I was seeing a similar issue in bug 1345260. If you closed a page in the middle of a redirect from A to B, then A would get the OnClose, but B wouldn't, so the channel would never get closed in the child. If you had a listener thing, like you do with AdblockPlus, then the channel ends up leaking the window.

Comment 7

[Honza Bambas (:mayhemer)]

Looks like this is fixed as part of bug 1354220 now?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Possibly.  Someone needs to actually rerun the test and check whether it leaks or whether the patch there fixed it.

Comment 9

[Jason Duell]

James, any chance we could get you to re-run these tests with nightly and see if they were fixed by bug 1354220?

Comment 10

[James Graham [:jgraham]]

This is looking *significantly* better now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13842d2b6f9f9ac38ece917c46fdcd9e47d37935; I only (scare quotes implied) see problems with EME, serviceworkers and websockets now. Thanks!