New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 805728 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Media code leaks the SystemURLRequestContextGetter

Project Member Reported by mmenke@chromium.org, Jan 25 2018

Issue description

https://chromium-review.googlesource.com/c/chromium/src/+/821490 introduced a leaky singleton which hols onto a reference to the SystemURLRequestContextGetter, and is leaking it.

Despite being refcounted, the SystemURLREquestContextGetter is not expected to be leaked.

This is unlikely to cause crashes. It may be that we can just annotate the SystemURLREquestContextGetter as being leaked, as it won't exist in a network service world.  Or we could even take this opportunity to move it over to using SimpleURLLoader, the network service replacement for URLFetcher (Which also, conveniently, works in production code when the network service is disabled).
 

Comment 1 by mmenke@chromium.org, Jan 25 2018

[imcheng]:  I suggest just switching over to URLLoaderFactory instead.  You can call g_browser_process->system_network_context_manager()->GetContext()->CreateNetworkContext(), and you get an object that you can keep (And even detach it from the current thread and use on another).  You can use SimpleURLLoader if you need a simplified API (Highly recommended).

The only caveat is you should handle network service crashes - if your URLLoaderFactory dies (Can watch it for death, or probe it), you'll need to grab another.

Alternatively, could just hop over to the UI thread and grab one before you need to make a network request (If you live on the UI thread, anyways, you don't even need to make one - just call GetURLLoaderFactory() on the SystemNetworkContextManager.
Thanks for the pointer mmenke@. I read the SimplerURLLoader API and I think we should be able to use it to replace the URLFetcher logic in the IO thread.

We also use SystemURLREquestContextGetter for its NetLog for CastSocketOpenParams. Could you advise on a possible replacement for that? (granted, it's not as important as we rarely use the NetLog functionality)

Comment 3 by sorin@chromium.org, Jan 25 2018

I understand that it was a known problem that a leak was introduced by https://chromium-review.googlesource.com/c/chromium/src/+/821490. However, we did not see any crbug filed on this issue. This made us waste several engineer hours if not days  on our side, trying to bisect the issue to identify the root cause several weeks after the commit had landed. Ignoring the issue was unproductive in my opinion and I hope we will do better in the future.

Comment 4 by mmenke@chromium.org, Jan 25 2018

[imcheng]:  xunjieli is currently working on a TCP socket API that goes through the network service.  You'll need to use that (The NetLog itself will be out of process on some platforms, so you can't access it directly).  The CL (Currently just interfaces) is at https://chromium-review.googlesource.com/c/chromium/src/+/868711.  A UDP interface was just landed in https://chromium-review.googlesource.com/#/c/796933/ (No consumers of it on trunk yet).

Please don't hesitate to reach out to us if you run into any problems, and help review any CLs.

Comment 5 by mmenke@chromium.org, Jan 25 2018

And we're happy to help review any CLs, rather.
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd6b757d953001eae759c4786b5afe9c7e00916f

commit cd6b757d953001eae759c4786b5afe9c7e00916f
Author: Derek Cheng <imcheng@chromium.org>
Date: Wed Feb 21 20:15:23 2018

[Media Router] Fix URLRequestContext leak and race condition.

Remove usage of SystemURLRequestContextGetter which has led to several
issues including shutdown leak and test setup problems. This means 2
things:
- No more NetLog for DIAL and Cast sockets (for now. When we switch to
NetworkService we can reconsider adding back support depending on
our needs)
- DIAL device description / app info fetch now replaced with URLLoader
APIs that's part of network service.

The race condition issue caused by calling CMSSImpl::GetWeakPtr on two
different sequence is fixed by not using a WeakPtr by assuming
CMSSImpl outlives DialMSSImpl, which is true due to their ordering in
DualMediaSinkService. Also removed the PostTask trampoline by assuming
they both run on the same sequence.

Bug:  810676 , 805728 , 698940 , 779892 , 811226 
Change-Id: Ib3ebb4f7f9d3690c952d2ddcbd579bb5d876906e
Reviewed-on: https://chromium-review.googlesource.com/912561
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bin Zhao <zhaobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538195}
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/extensions/api/dial/dial_api.cc
[add] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/DEPS
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_fetcher.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_fetcher.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_fetcher_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_service.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_service.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/device_description_service_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_discovery_service.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_discovery_service.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_discovery_service_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_info_fetcher.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_info_fetcher.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_app_info_fetcher_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/dial/dial_media_sink_service_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/mojo/media_router_desktop_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/providers/cast/dual_media_sink_service.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/providers/cast/dual_media_sink_service.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/providers/cast/dual_media_sink_service_unittest.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/test/test_helper.cc
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/browser/media/router/test/test_helper.h
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/chrome/test/BUILD.gn
[modify] https://crrev.com/cd6b757d953001eae759c4786b5afe9c7e00916f/components/cast_channel/cast_socket.cc

Owner: imch...@chromium.org
Status: Fixed (was: Untriaged)
Labels: Merge-Request-65
Requesting merge to 65. This fixes a race condition and shutdown memory leak in the browser code.
Project Member

Comment 9 by sheriffbot@chromium.org, Feb 27 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls apply appropriate OSs label. Thank you.
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows

Before we approve merge to M65, could you pls confirm followings?

Is this M65 regression and critical to merge?
Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge?
Any other imp details to justify the merge.

Please note M65 is going to stable next week so merge bar is VERY high and patch listed at #6 is a big patch to merge in this late in release cycle.

- This isn't a M65 regression, the bug has been here since M64. This fixes a shutdown leak and a race condition, both of which should not be a big issue in practice.
- I manually tested this locally. The tests on buildbots also have coverage. I haven't received any fuzzer or tsan alerts since landing this patch.

I understand this will be a challenging merge given size of patch and the timeline. I don't think it is absolutely critical to merge this back from stability perspective.
Labels: -Merge-Review-65 Merge-Rejected-65
Thank you imcheng@. Rejecting merge to M65 based on comment #13. 

Sign in to add a comment