Media code leaks the SystemURLRequestContextGetter |
||||||
Issue descriptionhttps://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).
,
Jan 25 2018
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)
,
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.
,
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.
,
Jan 25 2018
And we're happy to help review any CLs, rather.
,
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
,
Feb 22 2018
,
Feb 27 2018
Requesting merge to 65. This fixes a race condition and shutdown memory leak in the browser code.
,
Feb 27 2018
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
,
Feb 27 2018
Pls apply appropriate OSs label. Thank you.
,
Feb 27 2018
,
Feb 27 2018
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.
,
Feb 27 2018
- 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.
,
Feb 27 2018
Thank you imcheng@. Rejecting merge to M65 based on comment #13. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by mmenke@chromium.org
, Jan 25 2018