New issue
Advanced search Search tips

Issue 759761 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

[iOS Clean] Reenable TabCoordinator unittests.

Project Member Reported by kkhorimoto@chromium.org, Aug 28 2017

Issue description

The TabCoordinator uses the location bar, which has code that checks the current WebThread.  Currently, tab_coordinator_unittest.mm attempts to satisfy this check by using a base::MessageLoop and web::TestWebThread, but this causes another DCHECK when trying to create the message loop.
 

Comment 1 by pkl@chromium.org, Aug 28 2017

Components: Tests
Labels: ios-clean-skeleton
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 29 2017

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

commit d65ba5faf97ad45fe61ad1f0909bebf470cf0e62
Author: Kurt Horimoto <kkhorimoto@chromium.org>
Date: Tue Aug 29 15:55:12 2017

[iOS Clean] Disable TabCoordinator unittests.

These tests DHCECK when attempting to create the MessageLoop.

Bug:  759761 
Change-Id: Ibf65b4363560810f99cc097bd96e0b05c3a8f5c8
Reviewed-on: https://chromium-review.googlesource.com/638837
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498122}
[modify] https://crrev.com/d65ba5faf97ad45fe61ad1f0909bebf470cf0e62/ios/clean/chrome/browser/ui/tab/tab_coordinator_unittest.mm

Comment 3 by edchin@chromium.org, Sep 18 2017

[ RUN      ] TabCoordinatorTest.DefaultTabContainer
[86163:1027:0918/084721.353860:678148946068394:FATAL:chrome_browser_state.mm(82)] Check failed: ::web::WebThread::CurrentlyOn(web::WebThread::UI). Must be called on Web_UIThread; actually called on Unknown Thread.
0   ios_clean_chrome_unittests          0x000000010d8a0e4d base::debug::StackTrace::StackTrace(unsigned long) + 157
1   ios_clean_chrome_unittests          0x000000010d8a0e8d base::debug::StackTrace::StackTrace(unsigned long) + 29
2   ios_clean_chrome_unittests          0x000000010d89f94c base::debug::StackTrace::StackTrace() + 28
3   ios_clean_chrome_unittests          0x000000010d905cdf logging::LogMessage::~LogMessage() + 479
4   ios_clean_chrome_unittests          0x000000010d903835 logging::LogMessage::~LogMessage() + 21
5   ios_clean_chrome_unittests          0x000000010c68ea27 ios::ChromeBrowserState::GetRequestContext() + 343
6   ios_clean_chrome_unittests          0x000000010cdbd9aa OmniboxPopupViewIOS::OmniboxPopupViewIOS(ios::ChromeBrowserState*, OmniboxEditModel*, OmniboxPopupViewSuggestionsDelegate*, id<OmniboxPopupPositioner>) + 1306
7   ios_clean_chrome_unittests          0x000000010cdbeeb5 OmniboxPopupViewIOS::OmniboxPopupViewIOS(ios::ChromeBrowserState*, OmniboxEditModel*, OmniboxPopupViewSuggestionsDelegate*, id<OmniboxPopupPositioner>) + 69
8   ios_clean_chrome_unittests          0x000000010cdcf0b2 decltype(std::make_unique<OmniboxPopupViewIOS>(std::forward<ios::ChromeBrowserState*, OmniboxEditModel*, OmniboxViewIOS*, id<OmniboxPopupPositioner> __strong&>(fp))) base::MakeUnique<OmniboxPopupViewIOS, ios::ChromeBrowserState*, OmniboxEditModel*, OmniboxViewIOS*, id<OmniboxPopupPositioner> __strong&>(OmniboxPopupViewIOS&&) + 354
9   ios_clean_chrome_unittests          0x000000010cdcdf66 OmniboxViewIOS::OmniboxViewIOS(OmniboxTextFieldIOS*, WebOmniboxEditController*, ios::ChromeBrowserState*, id<PreloadProvider>, id<OmniboxPopupPositioner>) + 2166
10  ios_clean_chrome_unittests          0x000000010cdcf394 OmniboxViewIOS::OmniboxViewIOS(OmniboxTextFieldIOS*, WebOmniboxEditController*, ios::ChromeBrowserState*, id<PreloadProvider>, id<OmniboxPopupPositioner>) + 132
11  ios_clean_chrome_unittests          0x000000010cdae73a decltype(std::make_unique<OmniboxViewIOS>(std::forward<OmniboxTextFieldIOS* __strong&, LocationBarControllerImpl*, ios::ChromeBrowserState*&, id<PreloadProvider> __strong&, id<OmniboxPopupPositioner> __strong&>(fp))) base::MakeUnique<OmniboxViewIOS, OmniboxTextFieldIOS* __strong&, LocationBarControllerImpl*, ios::ChromeBrowserState*&, id<PreloadProvider> __strong&, id<OmniboxPopupPositioner> __strong&>(id<OmniboxPopupPositioner> __strong&&&) + 346
12  ios_clean_chrome_unittests          0x000000010cdae0e7 LocationBarControllerImpl::LocationBarControllerImpl(OmniboxTextFieldIOS*, ios::ChromeBrowserState*, id<PreloadProvider>, id<OmniboxPopupPositioner>, id<LocationBarDelegate>, id<BrowserCommands>) + 391
13  ios_clean_chrome_unittests          0x000000010cdaf47b LocationBarControllerImpl::LocationBarControllerImpl(OmniboxTextFieldIOS*, ios::ChromeBrowserState*, id<PreloadProvider>, id<OmniboxPopupPositioner>, id<LocationBarDelegate>, id<BrowserCommands>) + 187
14  ios_clean_chrome_unittests          0x000000010d841e4d decltype(std::make_unique<LocationBarControllerImpl>(std::forward<OmniboxTextFieldIOS* __strong, ios::ChromeBrowserState*, std::nullptr_t, std::nullptr_t, LocationBarMediator* __strong, std::nullptr_t>(fp))) base::MakeUnique<LocationBarControllerImpl, OmniboxTextFieldIOS* __strong, ios::ChromeBrowserState*, std::nullptr_t, std::nullptr_t, LocationBarMediator* __strong, std::nullptr_t>(OmniboxTextFieldIOS* __strong&&, ios::ChromeBrowserState*&&, std::nullptr_t&&, std::nullptr_t&&, LocationBarMediator* __strong&&, std::nullptr_t&&) + 381
15  ios_clean_chrome_unittests          0x000000010d841208 -[LocationBarCoordinator start] + 1096
16  ios_clean_chrome_unittests          0x000000010d829462 -[ToolbarCoordinator start] + 1938
17  ios_clean_chrome_unittests          0x000000010d7f5fd4 -[TabCoordinator start] + 3940

Comment 4 by edchin@chromium.org, Sep 18 2017

From emails with fdoray@

It is likely that you are trying to instantiate two of these classes in the same scope: base::MessageLoop, base::test::ScopedTaskEnvironment, web::WebThreadBundle. You can only instantiate one of this class in a scope. Use base::MessageLoop if you only need to run tasks on the main thread. Use base::test::ScopedTaskEnvironment if you need to run tasks on the main thread and post tasks via base/task_scheduler/post_task.h. Use web::WebThreadBundle if you need to run tasks on the main thread, post tasks via base/task_scheduler/post_task.h and post tasks via ios/web/public/web_thread.h.

To resolve the "Check failed: ::web::WebThread::CurrentlyOn(web::WebThread::UI). Must be called on Web_UIThread; actually called on Unknown Thread." error that you shared with me by email, you would need to add a TestWebThreadBundle to your test and make sure that the code with a DCHECK_CURRENTLY_ON(web::WebThread::UI) runs on the main thread.

Comment 5 by edchin@chromium.org, Sep 19 2017

Cc: rohitrao@chromium.org
Stack trace after using web::TestWebThreadBundle thread_bundle_;

[98594:1027:0919/150112.791526:762643349185799:FATAL:message_loop.cc(304)] Check failed: !current(). should only have one message loop per thread
0   ios_clean_chrome_unittests          0x000000010a53de4d base::debug::StackTrace::StackTrace(unsigned long) + 157
1   ios_clean_chrome_unittests          0x000000010a53de8d base::debug::StackTrace::StackTrace(unsigned long) + 29
2   ios_clean_chrome_unittests          0x000000010a53c94c base::debug::StackTrace::StackTrace() + 28
3   ios_clean_chrome_unittests          0x000000010a5a2cdf logging::LogMessage::~LogMessage() + 479
4   ios_clean_chrome_unittests          0x000000010a5a0835 logging::LogMessage::~LogMessage() + 21
5   ios_clean_chrome_unittests          0x000000010a5c7828 base::MessageLoop::BindToCurrentThread() + 1928
6   ios_clean_chrome_unittests          0x000000010a5c7d0b base::MessageLoop::MessageLoop(base::MessageLoop::Type) + 75
7   ios_clean_chrome_unittests          0x000000010a8b3d4a base::test::ScopedTaskEnvironment::ScopedTaskEnvironment(base::test::ScopedTaskEnvironment::MainThreadType, base::test::ScopedTaskEnvironment::ExecutionMode) + 154
8   ios_clean_chrome_unittests          0x000000010a8b4951 base::test::ScopedTaskEnvironment::ScopedTaskEnvironment(base::test::ScopedTaskEnvironment::MainThreadType, base::test::ScopedTaskEnvironment::ExecutionMode) + 33
9   ios_clean_chrome_unittests          0x0000000109981cc8 decltype(std::make_unique<base::test::ScopedTaskEnvironment>(std::forward<base::test::ScopedTaskEnvironment::MainThreadType>(fp))) base::MakeUnique<base::test::ScopedTaskEnvironment, base::test::ScopedTaskEnvironment::MainThreadType>(base::test::ScopedTaskEnvironment::MainThreadType&&) + 88
10  ios_clean_chrome_unittests          0x00000001099807b0 web::TestWebThreadBundle::Init(int) + 96
11  ios_clean_chrome_unittests          0x000000010998073f web::TestWebThreadBundle::TestWebThreadBundle() + 479
12  ios_clean_chrome_unittests          0x00000001099810d5 web::TestWebThreadBundle::TestWebThreadBundle() + 21
13  ios_clean_chrome_unittests          0x0000000108c760eb (anonymous namespace)::TabCoordinatorTest::TabCoordinatorTest() + 75
14  ios_clean_chrome_unittests          0x0000000108c7607f TabCoordinatorTest_DefaultTabContainer_Test::TabCoordinatorTest_DefaultTabContainer_Test() + 31
15  ios_clean_chrome_unittests          0x0000000108c76055 TabCoordinatorTest_DefaultTabContainer_Test::TabCoordinatorTest_DefaultTabContainer_Test() + 21

Comment 6 by edchin@chromium.org, Sep 20 2017

A fix for this is in-flight.
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 20 2017

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

commit 5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7
Author: edchin <edchin@chromium.org>
Date: Wed Sep 20 22:36:28 2017

[ios] Fix threading issue in tab_coordinator_unittest

Code in chrome_browser_state.mm relies on named threads. This CL
replaces use of base::test::ScopedTaskEnvironment in
BrowserCoordinatorTest class with web::TestWebThreadBundle
so that named threads can be used by legacy code.

Bug:  759761 
Change-Id: Ib4769ccdf66c0270e1f289dd5db2d78a5a9974f3
Reviewed-on: https://chromium-review.googlesource.com/674383
Commit-Queue: edchin <edchin@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503273}
[modify] https://crrev.com/5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7/ios/chrome/browser/ui/coordinators/BUILD.gn
[modify] https://crrev.com/5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7/ios/chrome/browser/ui/coordinators/browser_coordinator_test.h
[modify] https://crrev.com/5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7/ios/clean/chrome/browser/ui/tab/tab_coordinator.mm
[modify] https://crrev.com/5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7/ios/clean/chrome/browser/ui/tab/tab_coordinator_unittest.mm
[modify] https://crrev.com/5fd1d5bcaa1f6fc95c1b409b439a237f50544ed7/ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm

Status: WontFix (was: Assigned)
Clean is now deprecated.

Sign in to add a comment