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

Issue 721975 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
a...@chromium.org
jam@chromium.org
mfo...@chromium.org
kcc@chromium.org
clamy@chromium.org
creis@chromium.org
horo@chromium.org
█ nasko@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ui/views/controls/webview/webview_unittest.cc is linked into chrome/'s unittests, accidentally has chrome/ dependencies

Project Member Reported by p...@chromium.org, May 13 2017 
This is causing test failures on the "CFI Linux ToT" bot, e.g. https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/6239

$ cat args.gn
is_cfi = true
is_clang = true
is_component_build = false
is_debug = false
use_cfi_cast = true
use_cfi_diag = true
$ ninja unit_tests
$ ./unit_tests --single-process-tests --gtest_filter=WebViewUnitTest.DetachedWebViewDestructor
[...]
../../chrome/browser/profiles/profile.cc:54:10: runtime error: control flow integrity check for type 'Profile' failed during base-to-derived cast (vtable address 0x0000096c2e90)
0x0000096c2e90: note: vtable is of type 'content::TestBrowserContext'

backtrace:

#0  0x0000000008121444 in __ubsan_handle_cfi_check_fail ()
#1  0x0000000003e739f0 in Profile::FromBrowserContext(content::BrowserContext*) ()
#2  0x0000000005804e71 in ChromeWebUIControllerFactory::GetWebUIType(content::BrowserContext*, GURL const&) const ()
#3  0x000000000580726e in ChromeWebUIControllerFactory::UseWebUIForURL(content::BrowserContext*, GURL const&) const ()
#4  0x00000000022c3214 in content::WebUIControllerFactoryRegistry::UseWebUIForURL(content::BrowserContext*, GURL const&) const ()
#5  0x0000000002126b59 in content::RenderProcessHost::ShouldUseProcessPerSite(content::BrowserContext*, GURL const&) ()
#6  0x000000000223a269 in content::SiteInstanceImpl::GetProcess() ()
#7  0x0000000002284868 in content::WebContentsImpl::Init(content::WebContents::CreateParams const&) ()
#8  0x0000000002279e48 in content::WebContentsImpl::CreateWithOpener(content::WebContents::CreateParams const&, content::FrameTreeNode*) ()
#9  0x0000000002279c95 in content::WebContents::Create(content::WebContents::CreateParams const&) ()
#10 0x00000000015b804e in views::WebViewUnitTest::CreateWebContents() const ()
#11 0x00000000015b7f01 in views::WebViewUnitTest_DetachedWebViewDestructor_Test::TestBody() ()
#12 0x0000000001743791 in testing::Test::Run() ()
#13 0x00000000017452b4 in testing::TestInfo::Run() ()
#14 0x00000000017455d2 in testing::TestCase::Run() ()
#15 0x00000000017481b2 in testing::internal::UnitTestImpl::RunAllTests() ()
#16 0x0000000001747d4d in testing::UnitTest::Run() ()
#17 0x0000000003069511 in base::TestSuite::Run() ()
#18 0x000000000306069e in int base::internal::Invoker<base::internal::BindState<int (content::UnitTestTestSuite::*)(), base::internal::UnretainedWrapper<content::UnitTestTestSuite> >, int ()>::RunImpl<int (content::UnitTestTestSuite::* const&)(), std::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite> > const&, 0ul>(int (content::UnitTestTestSuite::* const&)(), std::tuple<base::internal::UnretainedWrapper<content::UnitTestTestSuite> > const&, base::IndexSequence<0ul>) ()
#19 0x000000000306bdff in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, int, int, bool, base::Callback<void (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) ()
#20 0x000000000306bc72 in base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) ()
#21 0x0000000003060461 in main ()

This appears to have been caused by https://codereview.chromium.org/2861433002 (i.e. r470740); if I revert locally the problem goes away. I think the root cause is that this change causes the function RenderProcessHost::ShouldUseProcessPerSite to be called in more cases than before, which leads to a bad cast from TestBrowserContext to Profile in Profile::FromBrowserContext.

clamy, can you please take a look?

Comment 1 by p...@chromium.org, May 13 2017

Description: Show this description

Comment 2 by p...@chromium.org, May 17 2017 

clamy: Ping, our bot is still red due to this.

Comment 3 by creis@chromium.org, May 19 2017

Cc: a...@chromium.org jam@chromium.org clamy@chromium.org
Components: Internals>Core
Owner: creis@chromium.org
Status: Started (was: Untriaged)
I think clamy@ is OOO.  I've got a CL (https://codereview.chromium.org/2887153008) that should avoid the problem, by short-circuiting the conditional the way we used to.

Note that I'm not fixing the underlying problem, which is here:

Profile* Profile::FromBrowserContext(content::BrowserContext* browser_context) {
  // This is safe; this is the only implementation of the browser context.
  return static_cast<Profile*>(browser_context);
}

Apparently that comment is wrong, given TestBrowserContext.  Maybe someone else wants to tackle that part?  :)

Comment 4 by a...@chromium.org, May 19 2017 

When I split BrowserContext from Profile in https://codereview.chromium.org/7464009 (r94317), there was no such thing as TestBrowserContext. It was added in https://codereview.chromium.org/7708010 (r97875).

Ouch. I didn't even know about TestBrowserContext until just now.
Project Member

Comment 5 by bugdroid1@chromium.org, May 19 2017 

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

commit a9f04baa366660121d53d211df8d30b43b42e796
Author: creis <creis@chromium.org>
Date: Fri May 19 22:27:42 2017

Avoid CFI error due to SiteInstanceImpl::GetProcess change.

This short-circuits a conditional the way it used to be, avoiding
a bad cast in a unit test.

BUG=721975
TEST=CFI bot goes green.

Review-Url: https://codereview.chromium.org/2887153008
Cr-Commit-Position: refs/heads/master@{#473366}

[modify] https://crrev.com/a9f04baa366660121d53d211df8d30b43b42e796/content/browser/site_instance_impl.cc

Comment 6 by creis@chromium.org, May 22 2017

Cc: mfo...@chromium.org horo@chromium.org
Status: Fixed (was: Started)
All the WebViewUnitTests are fixed since r473366 landed in  https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/6266, so I think this particular bug is fixed.

I filed  issue 725276  for the underlying bad cast from comments 3-4.

I think the other red on the bot might be unrelated?  
1) The MediaRouterUITest.SetsForcedCastModeWithPresentationURLs failure might be from mfoltz's https://codereview.chromium.org/2862393002.
2) The WorkerFetchTest failures started at a different build (https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/6259), perhaps due to horo's https://codereview.chromium.org/2889603007.

pcc@: I'll close this and maybe you can file new bugs for the other failures?

Comment 7 by a...@chromium.org, May 23 2017

Status: Assigned (was: Fixed)
Summary: ui/views/controls/webview/webview_unittest.cc is linked into chrome/'s unittests, accidentally has chrome/ dependencies (was: chrome/browser/profiles/profile.cc:54:10: runtime error: control flow integrity check for type 'Profile' failed during base-to-derived cast)
FYI, the real bug here is that WebViewUnitTest, while living in ui/views/controls/webview/webview_unittest.cc, is linked into chrome/'s unit_tests. Its allocation:

    browser_context_.reset(new content::TestBrowserContext);

is flat-up wrong because of this fact. Anything linked into a chrome/ test will end up needing full chrome/ test classes, and the correct test class here is chrome/'s TestingProfile (which can't be used due to DEPS, but that makes the real issue clear). Your fix put this test back into the "accidentally works" category.

Look at the stack trace:

#2  0x0000000005804e71 in **Chrome**WebUIControllerFactory::GetWebUIType(content::BrowserContext*, GURL const&) const ()
#3  0x000000000580726e in **Chrome**WebUIControllerFactory::UseWebUIForURL(content::BrowserContext*, GURL const&) const ()

If webview_unittest.cc is a ui/ test, it should be compiled into a ui/ test binary, not the chrome/ binary.

If webview_unittest.cc is a chrome/ test, it should live in the chrome/ directory, not the ui/ directory.

The fact that is tries to be both (https://cs.chromium.org/chromium/src/chrome/test/BUILD.gn?rcl=007ec6f38ea45bdb1f6adccb088fabc61c34670c&l=4729) is the real bug here.

Comment 8 by creis@chromium.org, May 25 2017

Owner: ananta@chromium.org
ananta@: Can you handle comment 7?  I won't have time for it in the near future, and it looks like you introduced webview_unittest.cc in https://codereview.chromium.org/569153005.  (Or Avi, feel free to take it if you know what change is needed.)  Thanks!
Project Member

Comment 9 by bugdroid1@chromium.org, May 31 2017 

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

commit fadbe57adf56100e2f7995ac7c94a9205fbc6aef
Author: avi <avi@chromium.org>
Date: Wed May 31 20:41:40 2017

Explicitly catch tests in chrome/ not using proper test profiles.

Pretty much all chrome/ code assumes that BrowserContexts are Profiles because those are the BrowserContexts that Chrome instantiates. Explicitly catch test code that violates that assumption.

BUG= 725276 , 721975

Review-Url: https://codereview.chromium.org/2899113002
Cr-Commit-Position: refs/heads/master@{#476007}

[modify] https://crrev.com/fadbe57adf56100e2f7995ac7c94a9205fbc6aef/chrome/browser/profiles/profile.cc

Comment 10 by thestig@chromium.org, Nov 18 2017

Owner: ----
Status: Untriaged (was: Assigned)
What's the status on this?
►

Sign in to add a comment