Closed Bug 1333106 Opened 8 years ago Closed 8 years ago

Leaks in mixed-content tests

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox-esr45 --- affected
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jgraham, Assigned: bzbarsky)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

Stop leaking in the prefetch service if the prefetch is blocked by security checks
2.51 KB, patch
mcmanus
: review+
jcristau
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
So I 'm trying to enable leak checking in web-platform-tests. It seems that some tests leak (not surprising). In particular mixed-content tests seem to be very commonly leaked: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53497702c4a5d1ff17229725d5675dd3935318fe Leaked URLs (across several jobs): [task 2017-01-19T18:21:59.236361Z] 18:21:59 INFO - PROCESS | 870 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/no-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:21:59.236815Z] 18:21:59 INFO - PROCESS | 870 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/no-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:21:59.237409Z] 18:21:59 INFO - PROCESS | 870 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=de21312f-7a1a-4e5e-b06f-ac00e20bb50b&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:21:59.238111Z] 18:21:59 INFO - PROCESS | 870 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/swap-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:21:59.238640Z] 18:21:59 INFO - PROCESS | 870 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/swap-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:21:59.239237Z] 18:21:59 INFO - PROCESS | 870 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=swap-scheme-redirect&action=purge&key=3a0eac20-cc4d-455c-a6bd-702eeefd0eb4&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:21:59.239806Z] 18:21:59 INFO - PROCESS | 870 | nsStringStats [task 2017-01-19T18:15:54.852046Z] 18:15:54 INFO - PROCESS | 879 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:15:54.852642Z] 18:15:54 INFO - PROCESS | 879 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:15:54.853303Z] 18:15:54 INFO - PROCESS | 879 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=714af48e-9b9c-4bf9-8625-6c391815f746&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:15:54.853994Z] 18:15:54 INFO - PROCESS | 879 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/same-host-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:15:54.854255Z] 18:15:54 INFO - PROCESS | 879 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/same-host-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:15:54.854894Z] 18:15:54 INFO - PROCESS | 879 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=2a173268-5d38-4a7b-99ab-581e31daf2fd& [task 2017-01-19T18:18:45.878679Z] 18:18:45 INFO - PROCESS | 877 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/swap-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:18:45.879196Z] 18:18:45 INFO - PROCESS | 877 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/swap-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:18:45.879390Z] 18:18:45 INFO - PROCESS | 877 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=swap-scheme-redirect&action=purge&key=146ac786-3710-4b50-89e7-555cf1a1b1db&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:21:44.596303Z] 18:21:44 INFO - PROCESS | 884 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/keep-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:21:44.596898Z] 18:21:44 INFO - PROCESS | 884 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/cross-origin-http/link-prefetch-tag/top-level/keep-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:21:44.597453Z] 18:21:44 INFO - PROCESS | 884 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=40ac91c5-758b- [task 2017-01-19T18:37:31.339987Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/keep-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:37:31.340274Z] 18:37:31 INFO - PROCESS | 876 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/keep-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:37:31.340874Z] 18:37:31 INFO - PROCESS | 876 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=f2a1e836-e66d-4f78-bb24-41a48533aac4&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:16:42.486181Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/same-host-http/link-prefetch-tag/top-level/swap-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.486857Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/http-csp/same-host-http/link-prefetch-tag/top-level/swap-scheme-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.487566Z] 18:16:42 INFO - PROCESS | 878 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=swap-scheme-redirect&action=purge&key=ab5d67d3-df36-4c3f-b039-7857a7917ded&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:16:42.488257Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/meta-csp/cross-origin-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.488951Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/meta-csp/cross-origin-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.489636Z] 18:16:42 INFO - PROCESS | 878 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=b10572f4-7d97-4748-bf96-12f85b02f424&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:16:42.490373Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/meta-csp/same-host-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.490923Z] 18:16:42 INFO - PROCESS | 878 | https://web-platform.test:8443/mixed-content/optionally-blockable/meta-csp/same-host-http/link-prefetch-tag/top-level/no-redirect/opt-in-blocks.https.html [task 2017-01-19T18:16:42.491494Z] 18:16:42 INFO - PROCESS | 878 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=8ab6deea-6296-4156-8f19-b479a9530217&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:18:28.704139Z] 18:18:28 INFO - PROCESS | 888 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/no-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:18:28.704591Z] 18:18:28 INFO - PROCESS | 888 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/no-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:18:28.705384Z] 18:18:28 INFO - PROCESS | 888 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=no-redirect&action=purge&key=65945dda-92c9-4ea2-8437-60cdad3952e8&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:17:28.538016Z] 18:17:28 INFO - PROCESS | 874 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/swap-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:17:28.538839Z] 18:17:28 INFO - PROCESS | 874 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/cross-origin-http/link-prefetch-tag/top-level/swap-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:17:28.539670Z] 18:17:28 INFO - PROCESS | 874 | http://www1.web-platform.test:8000/mixed-content/generic/expect.py?redirection=swap-scheme-redirect&action=purge&key=f5fe428e-78d7-43c5-86ab-1cd98be054c7&path=%2Fmixed-content&content_type=text/html [task 2017-01-19T18:17:28.540336Z] 18:17:28 INFO - PROCESS | 874 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/keep-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:17:28.540991Z] 18:17:28 INFO - PROCESS | 874 | https://web-platform.test:8443/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/keep-scheme-redirect/no-opt-in-allows.https.html [task 2017-01-19T18:17:28.541702Z] 18:17:28 INFO - PROCESS | 874 | http://web-platform.test:8000/mixed-content/generic/expect.py?redirection=keep-scheme-redirect&action=purge&key=0b364b6b-16ff-4419-b945-f56960ba0436&path=%2Fmixed-content&content_type=text/html
Blocks: 1333114
Hmm. So I can reproduce a leak by just loading https://w3c-test.org/mixed-content/optionally-blockable/no-opt-in/same-host-http/link-prefetch-tag/top-level/no-redirect/no-opt-in-allows.https.html in a debug build. Code inspection suggests that the bug is in the combination of nsHttpChannel::AsyncOpen2 and nsPrefetchNode::OpenChannel. Specifically, nsPrefetchNode::OpenChannel does the following: 1) Create a channel with mCallbacks set to "this" and store it in mChannel. 2) Call AsyncOpen2 on the channel. nsHttpChannel::AsyncOpen2 does a security check (which fails in this case, for mixed content) and if that fails returns immediately. The net result is that we have a reference cycle: the nsPrefetchNode owns the channel via mChannel and the channel owns the nsPrefetchNode via mCallbacks. I'm not sure which one is really in the wrong here, because I don't see clear documentation on what the ownership model here is. If AsyncOpen2 returns success, the responsibility for breaking the cycle is explicitly on the channel. But if AsyncOpen2 fails... it's not clear. AsyncOpen2 itself doesn't clean up (e.g. by calling HttpBaseChannel::ReleaseListeners). But some parts of AsyncOpen do. And some don't. For example, !gHttpHandler->Active() doesn't clean up, but NS_CheckPortSafety(mURI) failing does. There are no other failure paths out of nsHttpChannel::AsyncOpen. I guess the safe thing would be to handle this on both ends.
Component: web-platform-tests → Networking: HTTP
Product: Testing → Core
I filed bug 1333142 on auditing some other code that might have the same problem.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Comment on attachment 8829535 [details] [diff] [review] Stop leaking in the prefetch service if the prefetch is blocked by security checks Review of attachment 8829535 [details] [diff] [review]: ----------------------------------------------------------------- appreciate this
Attachment #8829535 - Flags: review?(mcmanus) → review+
Note that this is a regression from bug 1192948. We should probably backport this to branches...
Blocks: 1192948
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fa41bd31be Stop leaking in the prefetch service if the prefetch is blocked by security checks. r=mcmanus
Comment on attachment 8829535 [details] [diff] [review] Stop leaking in the prefetch service if the prefetch is blocked by security checks Approval Request Comment [Feature/Bug causing the regression]: Bug 1192948 [User impact if declined]: Leaks whenever <link rel="prefetch"> is blocked by security policy. [Is this code covered by automated tests?]: The leaking part is not, but jgraham is working on that. [Has the fix been verified in Nightly?]: In my local build, yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None, I think. [Is the change risky?]: No. [Why is the change risky/not risky?]: Just maintains existing invariants around httpchannel dropping its strong refs to callbacks on failure codepaths... [String changes made/needed]: None.
Attachment #8829535 - Flags: approval-mozilla-beta?
Attachment #8829535 - Flags: approval-mozilla-aurora?
esr45 is affected too. Not sure whether we care about backporting all the way there...
status-firefox-esr45: --- → affected
https://hg.mozilla.org/mozilla-central/rev/c1fa41bd31be
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8829535 [details] [diff] [review] Stop leaking in the prefetch service if the prefetch is blocked by security checks leak fix, aurora53+, beta52+
Attachment #8829535 - Flags: approval-mozilla-beta?
Attachment #8829535 - Flags: approval-mozilla-beta+
Attachment #8829535 - Flags: approval-mozilla-aurora?
Attachment #8829535 - Flags: approval-mozilla-aurora+
Setting qe-verify- per Comment 7.
Flags: qe-verify-
Blocks: 1341673
Blocks: 1352355
No longer blocks: 1333114
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: