Issue metadata
Sign in to add a comment
|
CaptivePortalBlockingPage::GetWiFiSSID() indirectly calls net::GetNetworkList() on the wrong thread. |
||||||||||||||||||||||
Issue descriptionIn net/base/network_interfaces.h, net::GetNetworkList() has a comment saying "Can be called only on a thread that allows IO." but there was nothing to enforce that. I have a pending CL [1] that ends up affecting net::internal::AddressTrackerLinux::GetInterfaceName() and now it checks to make sure it's on a task runner that allows IO. As a result, many browser tests, e.g. SSLUIDynamicInterstitialTest.MatchUnknownCertError, now fail locally for me: [252018:252018:0227/162926.838748:FATAL:thread_restrictions.cc(29)] Check failed: !g_blocking_disallowed.Get().Get(). Function marked as blocking was called from a scope that disallows blocking! If this task is running inside the TaskScheduler, it needs to have MayBlock() in its TaskTraits. Otherwise, consider making this blocking work asynchronous or, as a last resort, you may use ScopedAllowBlocking in a narrow scope. #0 base::debug::StackTrace::StackTrace() #1 base::debug::StackTrace::StackTrace() #2 logging::LogMessage::~LogMessage() #3 base::AssertBlockingAllowed() #4 base::internal::ScopedFDCloseTraits::Free() #5 base::ScopedGeneric<>::FreeIfNecessary() #6 base::ScopedGeneric<>::~ScopedGeneric() #7 net::internal::AddressTrackerLinux::GetInterfaceName() #8 net::internal::AddressTrackerLinux::IsTunnelInterface() #9 net::internal::AddressTrackerLinux::HandleMessage() #10 net::internal::AddressTrackerLinux::ReadMessages() #11 net::internal::AddressTrackerLinux::Init() #12 net::GetNetworkList() #13 net::GetWifiSSID() #14 CaptivePortalBlockingPage::GetWiFiSSID() #15 CaptivePortalBlockingPage::PopulateInterstitialStrings() #16 security_interstitials::SecurityInterstitialPage::GetHTMLContents() #17 content::InterstitialPageImpl::Show() #18 security_interstitials::SecurityInterstitialPage::Show() [1] https://crrev.com/c/935837
,
Mar 2 2018
I'll take a look.
,
Mar 2 2018
My impression is the "I/O" that things like AddressTrackerLinux do don't really block more than a typical non-blocking syscall does. The way AddressTrackerLinux is used by net::GetNetworkList() is nearly identical to how linux's typical getifaddrs() is implemented in libc. They use a netlink socket to request some information from the kernel. No IPC or actual network activity transpires, just some syscalls to communicate with the kernel.
,
Mar 2 2018
-eroman, who just moved GetNetworkList(). sergeyu added it in r78780, but that was a long time ago. Yes, given comment 3, we could probably whitelist the I/O in AddressTrackerLinux. My impression is the behavior of GetNetworkList() is platform specific and may block on some platforms. Thus it is best to just call it on the right thread in the first place.
,
Mar 2 2018
Hmm, looks like we've refactored SSLErrorHandler and friends recently. carlosil, spqchan, would any of you be interested in this bug?
,
Mar 3 2018
Agreed with thestig. On Windows, GetNetworkList() calls GetAdaptersAddresses() which is horribly slow.
,
Mar 5 2018
From the Captive Portal interstitial code, it looks like the call to GetWifiSSID only happens for Linux (https://cs.chromium.org/chromium/src/chrome/browser/ssl/captive_portal_blocking_page.cc?rcl=c27595f5c34ecd0d00e4638832fcf8f478eea048&l=125), so it might make sense to just do a ScopedAllowBlocking for the linux call if we know that the call is not really blocking on Linux (as per #3).
,
Aug 21
Issue 876322 has been merged into this issue.
,
Aug 21
Issue 876322 highlights that AddressTrackerLinux::ReadMessages() is running on the main thread. This is really bad. Can we prioritize this fix?
,
Aug 22
I noticed that Issue 876322 is caused by a different ScopedBlockingCall. In any case, the issue is that AddressTrackerLinux is blocking on the main thread. Following on carlosil@ idea, if the call is not really blocking, can we add appropriate ScopedAllowBlocking and a UMA metric to report latency that will support this claim. I'm also wondering if the ScopedBlockingCall is really necessary in that case. pauljensen@ any thoughts on this?
,
Aug 22
I don't think the ScopedBlockingCall in AddressTrackerLinux is appropriate when |tracking_| is false.
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d98cee7a16c1982b77e3e6fa58469f731b1d844c commit d98cee7a16c1982b77e3e6fa58469f731b1d844c Author: Etienne Pierre-doray <etiennep@chromium.org> Date: Fri Aug 24 17:55:11 2018 Conditional ScopedBlockingCall in AddressTrackerLinux. AddressTrackerLinux::ReadMessage is invoked from the main thread. It should not be possible to instantiate ScopedBlockingCall in that case (adding AssertBlockingAllowed breaks browser_tests). pauljensen@ pointed out that blocking does not happen when |tracking_| is false. This CL is a short-term fix for bug 817151, instantiating ScopedAllowBlocking only when |tracking_| is true. Bug: 817151 Change-Id: Ic5970b4e1c4fefb5231a2d666948b8cc2b7cdf35 Reviewed-on: https://chromium-review.googlesource.com/1187388 Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/heads/master@{#585904} [modify] https://crrev.com/d98cee7a16c1982b77e3e6fa58469f731b1d844c/net/base/address_tracker_linux.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by thestig@chromium.org
, Mar 2 2018