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

Issue 902311 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

BrowserMainLoopTest.CreateThreadsInSingleProcess is failing on win7_chromium_rel_ng and win10_chromium_rel_ng

Project Member Reported by tikuta@chromium.org, Nov 6

Issue description

Cc: pmonette@chromium.org hbos@chromium.org dim...@chromium.org
Labels: -Pri-2 Pri-0
Owner: ksakamoto@chromium.org
Status: Assigned (was: Untriaged)
Summary: BrowserMainLoopTest.CreateThreadsInSingleProcess is failing on win7_chromium_rel_ng and win10_chromium_rel_ng (was: BrowserMainLoopTest.CreateThreadsInSingleProcess is flaky on win7_chromium_rel_ng)
This is not flaky. It fails consistently. https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win7_chromium_rel_ng?limit=200

Looking into the recent successful runs, they actually do not run content_unittests. When it runs, it always fails. Same on win10_chromium_rel_ng.

Assigning to the non-west-coast sheriff, cc: other sheriffs, increasing to P0.
Suspecting https://chromium-review.googlesource.com/c/chromium/src/+/1305822 - will test locally and submit a revert if I can confirm it's responsible.
Cc: ksakamoto@chromium.org
Owner: w...@chromium.org
Status: Started (was: Assigned)
Cc: yosin@chromium.org
+yosin, FYI, as the current sheriff, at time of writing.
After 123794, win7_chromium_rel_ng bot doesn't run tests tests.
I'm not sure why the bot stops running tests.

Anyway, BrowserMainLoopTest.CreateThreadsInSingleProcess is still failed.


123794 https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win7_chromium_rel_ng/123794
Confirmed that https://chromium-review.googlesource.com/c/chromium/src/+/1305822 breaks this test in Debug builds, so sent a revert to the CQ (https://chromium-review.googlesource.com/c/chromium/src/+/1322109).
Test fails with:

[ RUN      ] BrowserMainLoopTest.CreateThreadsInSingleProcess
[8032:2072:1106/165157.487:117421:FATAL:thread_restrictions.cc(34)] 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 (see its documentation for best practices).
Backtrace:
	base::debug::StackTrace::StackTrace [0x00007FF7800022F4+36]
	logging::LogMessage::~LogMessage [0x00007FF78001A172+98]
	base::internal::AssertBlockingAllowed [0x00007FF78007127B+123]
	net::GetNetworkList [0x00007FF78016DA41+65]
	net::NetworkChangeNotifier::ConnectionTypeFromInterfaces [0x00007FF78016A0D1+49]
	net::NetworkChangeNotifierWin::RecomputeCurrentConnectionType [0x00007FF780CFF258+632]
	net::NetworkChangeNotifierWin::NetworkChangeNotifierWin [0x00007FF780CFEDCE+174]
	net::NetworkChangeNotifier::Create [0x00007FF780169C72+50]
	network::NetworkService::NetworkService [0x00007FF77FE6C832+866]
	service_manager::Connector::BindInterface<network::mojom::NetworkService> [0x00007FF77E872AB3+723]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(mojo::InterfaceRequest<discardable_memory::mojom::DiscardableSharedMemoryManager>),mojo::InterfaceRequest<discardable_memory::mojom::DiscardableSharedMemoryManager> >,void __cdecl(void)>::R [0x00007FF77E79BB5E+62]
	base::debug::TaskAnnotator::RunTask [0x00007FF78154CEBC+364]
	base::MessageLoopImpl::RunTask [0x00007FF780C49C1F+287]
	base::MessageLoopImpl::DoWork [0x00007FF780C4A175+389]
	base::MessagePumpForIO::DoRunLoop [0x00007FF780024CD5+165]
	base::MessagePumpWin::Run [0x00007FF7800239FE+78]
	base::MessageLoopImpl::Run [0x00007FF780C4988B+139]
	base::RunLoop::Run [0x00007FF780040F49+249]
	base::Thread::Run [0x00007FF78006FCA8+200]
	content::BrowserProcessSubThread::IOThreadRun [0x00007FF77E609234+36]
	content::BrowserProcessSubThread::Run [0x00007FF77E6091B2+210]
	base::Thread::ThreadMain [0x00007FF78006FFCE+766]
	base::PlatformThread::GetCurrentThreadPriority [0x00007FF78006DAEC+748]
	BaseThreadInitThunk [0x00007FFB92DE2774+20]
	RtlUserThreadStart [0x00007FFB92EF0D51+33]

It's possible that the CL crossed-paths with the fix for the NetworkChangeNotifier getting leaked by tests?
Owner: yosin@chromium.org
Sheriff yosin@ to the rescue; we discussed & since revert of that CL doesn't work, we'll disable the BrowserMainLoopTest.CreateThreadsInSingleProcess test to unblock CQ & let test owners resolve it.
The revert CL[1] failed on below tests on Windows.
 * RenderThreadImplBrowserTest.NonResourceDispatchIPCTasksDontGoThroughScheduler
 * UserScriptListenerTest.NavigationWaitsForContentScriptsToLoad

Temporary disable above tests to make CQ unblock since CQ is blocked 16 hours.



[1] http://crrev.com/c/1322109 Revert "Reland "Start ServiceManger before creating BrowserMainLoop.""
Committing... http://crrev.com/c/1322343

Slowness of committing is caused by: android-kitkat-arm-rel
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/119935
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 7

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

commit faa86b61f1f34002e7bdfc923541d453b9e4223f
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Wed Nov 07 09:55:04 2018

Revert "Reland "Start ServiceManger before creating BrowserMainLoop.""

This reverts commit f481306ad989755ebe61cfed4ab2a4fa53044b29 and
disables following tests on Windows:
 * RenderThreadImplBrowserTest.NonResourceDispatchIPCTasksDontGoThroughScheduler
 * UserScriptListenerTest.NavigationWaitsForContentScriptsToLoad

Reason for revert: Causes BrowserMainLoopTest, CreateThreadsInSingleProcess to fail every time content_unittests is run, if DCHECKs are enabled.

Original change's description:
> Reland "Start ServiceManger before creating BrowserMainLoop."
>
> This relands commit aa60c219407f1158214858e83e5456e42dcfe891. The original CL
> (https://crrev.com/c/1113802) got reverted becuase FeatureList and field
> trials are not setup properly in early startup in Android WebView, but
> ServiceMangerContext has checked features. See  crbug.com/899376 . The fix is
> in a separate CL: https://crrev.com/c/1305876. Another related precursor CL
> is: https://crrev.com/c/1308096.
>
> This CL is an exact copy of the original CL.
>
> The original cl description is:
> This CL instantiates the ServiceManagerContext before creating
> the BrowserMainRunner. It splits the startup path into two,
> with/without starting the full browser. The changes are implemented
> behind a flag "allow-start-service-manager-only".
>
> Bug:  846846 ,729596
> TBR: jam@chromium.org
> Change-Id: I5214af850d4ef256c9d223db059ed009a42de714
> Reviewed-on: https://chromium-review.googlesource.com/c/1305822
> Commit-Queue: Xi Han <hanxi@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#604969}

TBR=gab@chromium.org,jam@chromium.org,hanxi@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  846846 , 729596,  902311 
Change-Id: I8b46711602a240023f4a03a69aa70f66d4eee726
Reviewed-on: https://chromium-review.googlesource.com/c/1322343
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606004}
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/chrome/browser/extensions/user_script_listener_browsertest.cc
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/app/DEPS
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/app/content_main_runner_impl.h
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/browser_main_loop.cc
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/browser_main_loop.h
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/browser_main_loop_unittest.cc
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/service_manager/service_manager_context.h
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/browser/startup_data_impl.h
[modify] https://crrev.com/faa86b61f1f34002e7bdfc923541d453b9e4223f/content/renderer/render_thread_impl_browsertest.cc

Cc: -hanxi@chromium.org
Labels: -Pri-0 -Sheriff-Chromium Pri-1
Owner: hanxi@chromium.org
Status: Assigned (was: Started)
Lower to Pri-1 since culprit CL[1] is reverted with disabling some tests.
Assign to hanxi@ who is author of the CL[1]

hanxi@, please take look and re-enable following tests once you fixed.
 * RenderThreadImplBrowserTest.NonResourceDispatchIPCTasksDontGoThroughScheduler
 * UserScriptListenerTest.NavigationWaitsForContentScriptsToLoad

Thanks!

[1] http://crrev.com/c/1305822 Reland "Start ServiceManger before creating BrowserMainLoop."
 Issue 902720  has been merged into this issue.
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 27

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

commit 3575eb72d8073cbf789dc5436446fe5f888eea52
Author: Xi Han <hanxi@google.com>
Date: Tue Nov 27 21:39:29 2018

Reland "Start ServiceManger before creating BrowserMainLoop."

This relands commit f481306ad989755ebe61cfed4ab2a4fa53044b29. The CL
got reverted because BrowserMainLoopTest.CreateThreadsInSingleProcess
is falling on Windows. In this CL, we remove the call of
BrowserMainLoop#InitilaizeMojo() which isn't necessary for the test.

Beside, also re-enable two tests which were disabled when the reverting
CL landed:
 * RenderThreadImplBrowserTest.NonResourceDispatchIPCTasksDontGoThroughScheduler

The original cl description is:
This CL instantiates the ServiceManagerContext before creating
the BrowserMainRunner. It splits the startup path into two,
with/without starting the full browser. The changes are implemented
behind a flag "allow-start-service-manager-only".

Bug:  846846 , 902311 
Change-Id: I6e3f6518e414e1298e57b55bd188879461d8f342
Reviewed-on: https://chromium-review.googlesource.com/c/1327413
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611340}
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/app/DEPS
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/app/content_main_runner_impl.h
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/browser/browser_main_loop.cc
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/browser/browser_main_loop.h
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/browser/service_manager/service_manager_context.h
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/browser/startup_data_impl.h
[modify] https://crrev.com/3575eb72d8073cbf789dc5436446fe5f888eea52/content/renderer/render_thread_impl_browsertest.cc

Cc: hanxi@chromium.org
 Issue 902903  has been merged into this issue.
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 29

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

commit 71a188e053a19e3ce3528d33eef1a30166c878e4
Author: Xi Han <hanxi@google.com>
Date: Thu Nov 29 19:58:13 2018

Re-enable UserScriptListenerTest.NavigationWaitsForContentScriptsToLoad.

Bug:  902311 
Change-Id: I6f39faecc38ad002a70bfda7fd355ab83afc0889
Reviewed-on: https://chromium-review.googlesource.com/c/1337487
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612328}
[modify] https://crrev.com/71a188e053a19e3ce3528d33eef1a30166c878e4/chrome/browser/extensions/user_script_listener_browsertest.cc

Status: Fixed (was: Assigned)
Since the two tests have been re-enabled, mark this as fixed. Feel free to reopen it if anything goes wrong.

Sign in to add a comment