Closed Bug 1341673 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: jgraham, Assigned: mcmanus)

References

Details

(Keywords: memory-leak)

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
Blocks: 1333114
Keywords: mlk
Component: Networking: HTTP → Security: PSM
Component: Security: PSM → Security
Is this an error in the implementation of the mixed-content handler or the tests?
Flags: needinfo?(james)
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.
Flags: needinfo?(james)
Blocks: 1352355
No longer blocks: 1333114
Depends on: 1354220
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?
Component: Security → Networking: HTTP
Flags: needinfo?(josh)
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(honzab.moz)
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.
Summary: More leaks in mixed-content tests → More leaks in mixed-content tests due to HttpChannelChild not dropping callback refs on redirect (e10s leak)
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.
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.
Looks like this is fixed as part of bug 1354220 now?
Possibly.  Someone needs to actually rerun the test and check whether it leaks or whether the patch there fixed it.
James, any chance we could get you to re-run these tests with nightly and see if they were fixed by bug 1354220?
Flags: needinfo?(jduell.mcbugs) → needinfo?(james)
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!
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(james)
Resolution: --- → FIXED
Flags: needinfo?(honzab.moz)
Assignee: nobody → mcmanus
Flags: needinfo?(josh)
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.