LocalNTPTests failing on Site Isolation bots |
|||||
Issue descriptionStarting in https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/14065, the following tests have been consistently failing on Site Isolation FYI bots, as well as linux_site_isolation try bot: LocalNTPTest.LoadsIframe LocalNTPTest.SimpleJavascriptTests In addition, LocalNTPTest.FakeboxRedirectsToOmnibox has started being flaky (failing twice), which might be related. This is most likely caused by https://codereview.chromium.org/2695813012, which added LocalNTPTest.LoadsIframe. treib@: can you please take a look? You can run these with --site-per-process for a local repro. We should get this fixed or disabled soon to get the bots green, as this might be potentially blocking the CQ. We can use site-per-process.interactive_ui_tests.filter if we need to disable. Failure logs: LocalNTPTest.LoadsIframe ../../chrome/browser/ui/search/local_ntp_browsertest.cc:209: Failure Expected: (total_thumbs) > (0), actual: 0 vs 0 [16267:16267:0220/072920.946316:WARNING:url_request_context_getter.cc(43)] URLRequestContextGetter leaking due to no owning thread. [ FAILED ] LocalNTPTest.LoadsIframe, where TypeParam = and GetParam() = (743 ms) LocalNTPTest.SimpleJavascriptTests [ RUN ] LocalNTPTest.SimpleJavascriptTests Xlib: extension "RANDR" missing on display ":99". Xlib: extension "RANDR" missing on display ":99". [16084:16084:0220/072918.881882:WARNING:audio_manager.cc(322)] Multiple instances of AudioManager detected [16084:16084:0220/072918.881928:WARNING:audio_manager.cc(281)] Multiple instances of AudioManager detected [16084:16084:0220/072918.921993:WARNING:password_store_factory.cc(247)] Using basic (unencrypted) store for password storage. See https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_storage.md for more information about password storage options. [16084:16104:0220/072919.151581:WARNING:simple_synchronous_entry.cc(1054)] Could not open platform files for entry. [16084:16085:0220/072919.204075:WARNING:embedded_test_server.cc(219)] Request not handled. Returning 404: /favicon.ico [16084:16084:0220/072919.236899:WARNING:render_frame_host_impl.cc(2220)] OnDidStopLoading was called twice. [16084:16084:0220/072919.243606:FATAL:web_contents_observer_sanity_checker.cc(293)] Check failed: !is_loading_. #0 0x000002b55897 base::debug::StackTrace::StackTrace() #1 0x000002b6f14b logging::LogMessage::~LogMessage() #2 0x0000023302cd content::WebContentsObserverSanityChecker::DidStartLoading() #3 0x000001036564 content::WebContentsImpl::LoadingStateChanged() #4 0x00000103bf27 content::WebContentsImpl::DidStartLoading() #5 0x000000d139b0 content::FrameTreeNode::DidStartLoading() #6 0x000000d3ec01 _ZN3IPC8MessageTI33FrameHostMsg_DidStartLoading_MetaSt5tupleIJbEEvE8DispatchIN7content19RenderFrameHostImplES7_vMS7_FvbEEEbPKNS_7MessageEPT_PT0_PT1_T2_ #7 0x000000d35cdb content::RenderFrameHostImpl::OnMessageReceived() #8 0x000000f0eea6 content::RenderProcessHostImpl::OnMessageReceived() #9 0x000002f0c135 IPC::ChannelProxy::Context::OnDispatchMessage() #10 0x000000b385da _ZN4base8internal7InvokerINS0_9BindStateINS0_18IgnoreResultHelperIMN7content20BrowserMessageFilter8InternalEFbRKN3IPC7MessageEEEEJ13scoped_refptrIS6_ES8_EEEFvvEE3RunEPNS0_13BindStateBaseE #11 0x000002c09759 base::debug::TaskAnnotator::RunTask() #12 0x000002b766fd base::MessageLoop::RunTask() #13 0x000002b76db6 base::MessageLoop::DoWork() #14 0x000002b78ba9 base::MessagePumpGlib::Run() #15 0x000002b76453 base::MessageLoop::RunHandler() #16 0x000002b9e57c base::RunLoop::Run() #17 0x00000232b41d content::MessageLoopRunner::Run() #18 0x000002323c6a content::DOMMessageQueue::WaitForMessage() #19 0x0000023234ce content::(anonymous namespace)::ExecuteScriptHelper() #20 0x000002323a9c content::ExecuteScriptAndExtractBool() #21 0x0000005bcf7d InstantTestBase::GetBoolFromJS() #22 0x0000005bd56c LocalNTPTest_SimpleJavascriptTests_Test::RunTestOnMainThread() #23 0x000001e870ef InProcessBrowserTest::RunTestOnMainThreadLoop() #24 0x000002321ca6 content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() #25 0x0000021daa8f ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #26 0x0000021d9961 ChromeBrowserMainParts::PreMainMessageLoopRun() #27 0x000000c1a885 content::BrowserMainLoop::PreMainMessageLoopRun() #28 0x000001000306 content::StartupTaskRunner::RunAllTasksNow() #29 0x000000c1871b content::BrowserMainLoop::CreateStartupTasks() #30 0x000000c1e4be content::BrowserMainRunnerImpl::Initialize() #31 0x000000c15b98 content::BrowserMain() #32 0x000001e6f0b7 content::RunNamedProcessTypeMain() #33 0x000001e6f986 content::ContentMainRunnerImpl::Run() #34 0x000001e6e2a0 content::ContentMain() #35 0x0000023215b0 content::BrowserTestBase::SetUp() #36 0x000001e85f01 InProcessBrowserTest::SetUp() #37 0x000002a6474e testing::Test::Run() #38 0x000002a65310 testing::TestInfo::Run() #39 0x000002a65687 testing::TestCase::Run() #40 0x000002a6c767 testing::internal::UnitTestImpl::RunAllTests() #41 0x000002a6c3e7 testing::UnitTest::Run() #42 0x000002b3fed1 base::TestSuite::Run() #43 0x0000005e1933 InteractiveUITestSuiteRunner::RunTestSuite() #44 0x00000232754c content::LaunchTests() #45 0x0000005e18b8 main #46 0x7f0d135e5f45 __libc_start_main #47 0x00000048c3e1 <unknown> LocalNTPTest.FakeboxRedirectsToOmnibox [ RUN ] LocalNTPTest.FakeboxRedirectsToOmnibox ../../chrome/browser/ui/search/local_ntp_browsertest.cc:118: Failure Value of: omnibox()->model()->focus_state() Actual: 1 Expected: OMNIBOX_FOCUS_INVISIBLE Which is: 2 [9807:9807:0220/081028.457181:WARNING:render_frame_host_impl.cc(2220)] OnDidStopLoading was called twice. ../../chrome/browser/ui/search/local_ntp_browsertest.cc:122: Failure Value of: result Actual: false Expected: true ../../chrome/browser/ui/search/local_ntp_browsertest.cc:136: Failure Value of: result Actual: true Expected: false [9807:9807:0220/081028.621880:WARNING:url_request_context_getter.cc(43)] URLRequestContextGetter leaking due to no owning thread. [ FAILED ] LocalNTPTest.FakeboxRedirectsToOmnibox, where TypeParam = and GetParam() = (868 ms)
,
Feb 23 2017
Not the FYI bots, but the linux_site_isolation trybot is required by the CQ for changes in content/browser/frame_host and optional for other changes, though it's strongly recommended to run it for CLs touching any navigation or iframe logic. Several test suites also run on the CQ with --site-per-process; interactive_ui_tests is not one of those yet, but hopefully will be soon. With --site-per-process, cross-site iframes render in separate processes [1]. What are the URLs of the iframe and its parent in these tests? If it's an http URL and the parent is a chrome:// URL, for example, the iframe will be in a separate process. Is there anything in your test that relies on them being in the same process? We've had some issues with NTP and site isolation which might be relevant (issues 566091 and 456420). I'll disable these tests on our bots for now while we investigate. [1] More background here: https://www.chromium.org/developers/design-documents/site-isolation and https://www.chromium.org/developers/design-documents/oop-iframes
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/714692af707ca89a3279f3e6df96cb19975956ea commit 714692af707ca89a3279f3e6df96cb19975956ea Author: alexmos <alexmos@chromium.org> Date: Fri Feb 24 00:54:54 2017 Disable LocalNTPTests that are failing with --site-per-process. BUG= 695221 NOTRY=true Review-Url: https://codereview.chromium.org/2715533009 Cr-Commit-Position: refs/heads/master@{#452698} [modify] https://crrev.com/714692af707ca89a3279f3e6df96cb19975956ea/testing/buildbot/filters/site-per-process.interactive_ui_tests.filter
,
Feb 28 2017
Thanks! In the test, the iframe is from chrome-search://most-visited, while the parent is from some https URL (from an embedded test server). This is similar to the setup of the remote NTP, so this indeed seems to be the same problem as bug 566091. I'll investigate and probably come back with more questions :)
,
Feb 28 2017
,
Mar 2 2017
I think making this work in OOPIF should be relatively straightforward:
1. In chrome_content_renderer_client.cc remove the IsMainFrame check:
if (command_line->HasSwitch(switches::kInstantProcess) &&
render_frame->IsMainFrame()) {
new SearchBox(render_frame);
}
This will cause each frame to connect to SearchIPCRouter and maintain its own copy of the browser state (e.g. the most visited items).
2. In search_ipc_router.h change:
mojo::AssociatedBinding<chrome::mojom::Instant> binding_;
to something like:
struct ClientConn {
chrome::mojom::SearchBoxAssociatedPtr search_box_;
mojo::AssociatedBinding<chrome::mojom::Instant> binding_;
};
std::unordered_map<RenderFrameHost*, std::unique_ptr<ClientConn>> connections_;
and in SearchBoxClientFactoryImpl::Connect add newly connected frames to this map (also install an set_connection_error_handler to remove frames that disconnect).
This is required to allow a) connections from more than one frame and 2) push browser state updates to all the frames.
3. Change uses of SearchBoxAssociatedPtr in SearchIPCRouter to iterate over the map and send the browser state update to every frame.
4. Change SearchBoxExtensionWrapper::GetRenderFrame & co to just ask the local frame instead of the main frame.
treib@ would you like to take a stab at it?
,
Mar 2 2017
Thanks! I had already looked into it a bit, and figured out more or less everything except for 2., which to me looks like arcane incantations :) I'll try if I can get things to work, and send either a CL or further questions your way! Thanks again for your help!
,
Mar 3 2017
,
Oct 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1cb33e753b82ff20cc809f1b3a7d46162761e47d commit 1cb33e753b82ff20cc809f1b3a7d46162761e47d Author: Marc Treib <treib@chromium.org> Date: Wed Oct 11 18:05:25 2017 Re-enable LocalNTPTests in --site-per-process mode A recent workaround to disable OOPIFs on the NTP (crrev.com/504845) lets these tests pass with --site-per-process, so let's re-enable them to make sure nothing else gets broken. Bug: 695221 Change-Id: Ic7335d27a0036b9d80d466d55a6c79c1c92a69f0 Reviewed-on: https://chromium-review.googlesource.com/712916 Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#508036} [modify] https://crrev.com/1cb33e753b82ff20cc809f1b3a7d46162761e47d/chrome/browser/ui/search/local_ntp_browsertest.cc [modify] https://crrev.com/1cb33e753b82ff20cc809f1b3a7d46162761e47d/testing/buildbot/filters/site-per-process.interactive_ui_tests.filter
,
Oct 12 2017
Closing this since the tests now work, thanks to creis@'s workaround. Bug 566091 remains open for the actual solution. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by treib@chromium.org
, Feb 23 2017