jlklein@ - please take a look and see if your CL is involved.

Somehow this flakiness is specific to single_process_mash_browser_tests. Some Mash specific changes may be the cause. I'll filter the test for Mash for now.
https://chromium-review.googlesource.com/c/chromium/src/+/1280854

Thanks, mukai@!

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

commit c27e10f26efe5aeb3aabf645687d2b6172bf8d1e
Author: Jun Mukai <mukai@chromium.org>
Date: Mon Oct 15 18:25:40 2018

filter 3 more flaky tests on single_process_mash

The following tests are flaky only on SingleProcessMash.

BUG= 895070 ,  895110 ,  895068 
TEST=None
TBR=sky@chromium.org

Change-Id: I38c1ce550e00472933dbd16fcddec1d6b7af0e54
Reviewed-on: https://chromium-review.googlesource.com/c/1280854
Reviewed-by: Jun Mukai <mukai@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599685}
[modify] https://crrev.com/c27e10f26efe5aeb3aabf645687d2b6172bf8d1e/testing/buildbot/filters/chromeos.single_process_mash.browser_tests.filter

Assigned the new rotation of mustash sheriff.

So far I couldn't reproduce the failure locally, maybe we can re-enable it and see how it goes.

Assigned the new rotation of mustash sheriff.

So far I couldn't reproduce the failure locally, maybe we can re-enable it and see how it goes.

I confirmed that findit was right. This flake is because of https://chromium-review.googlesource.com/c/chromium/src/+/1279067 . 
I was able to reproduce locally by building the same as the bots:

ffmpeg_branding = "ChromeOS"
is_component_build = false
is_debug = false
proprietary_codecs = true
strip_absolute_paths_from_debug_symbols = true
target_os = "chromeos"
use_goma = true
use_vaapi = true

At the revision https://chromium-review.googlesource.com/c/chromium/src/+/1279067 landed ( b2868aaedd272b1f32c002076960f95386caa8ba ). 

And running the test:

--enable-features=SingleProcessMash --override-use-software-gl-for-tests --gtest_filter=WakeEventPageTest.NoBackgroundPage

Going back to the previous revision, bd35effdb85b3df315a8e3f7eeb02d591af5241e, and rerunning did *not* result in the same failure. I tried the test 50 times on bd35effdb85b3df315a8e3f7eeb02d591af5241e . I'm going to revert the failing patch.

I couldn't revert as there is a conflict. Kyle, could you please look into this?

This also applies to ContentScriptApiTest.ContentScriptExtensionAPIs.

 Issue 895110  has been merged into this issue.

Okay, I'll take a look at it.

I haven't worked with single_process_mash_browser_tests before - does this mean I just build the normal browser_tests test target and run it with the flags you provided in comment #8?

That's right.
Also, make sure you use the arguments I pasted in comment 8 when doing a build. I was unable to repro with a debug or release build that had flags other than those pasted in comment 8. I didn't try to track down which flag made the difference. It could have been nacl as I normally don't build with nacl.

Thanks - I was able to reproduce the issues locally as well.

I tried modifying InProcessBrowserTest so that it stubs out some of the multi-device functionality in its *constructor* rather than in SetUp(), since some derived types (e.g., ExtensionBrowserTest) do some work in their constructors, before SetUp() is run. See my in-progress CL at [1].

This *does* fix WakeEventPageTest.NoBackgroundPage, but it doesn't seem to affect ContentScriptApiTest.ContentScriptExtensionAPIs. For that test, it seems the failure (see [2]) is here:

  ResultCatcher catcher;
  ui_test_utils::NavigateToURL(
      browser(),
      embedded_test_server()->GetURL(
          "/extensions/api_test/content_scripts/extension_api/functions.html"));
  EXPECT_TRUE(catcher.GetNextResult());

The GetNextResult() call never returns, so the test times out and fails. That being said, I am having a hard time figuring out why this is failing. Adding in some //chrome/browser/extensions OWNERS to see if they have any ideas.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1285320
[2] https://cs.chromium.org/chromium/src/chrome/browser/extensions/content_script_apitest.cc?l=366-371

+noel@, whom benwells@ pointed to as someone with knowledge of these tests. Noel, any ideas about how I can fix this issue?

Ben was maybe thinking of something else (I've never seen or heard of these tests before :) 

Anyhow, I did confirm sky@'s first result in #8, but using these gn args 

 target_os = "chromeos"
 is_debug = false
 dcheck_always_on = true
 is_component_build = false
 ffmpeg_branding="ChromeOS"
 proprietary_codecs=true
 use_goma = true

at crrev.com/600722, running the browser tests with:

 --enable-features=SingleProcessMash \
 --gtest_filter=WakeEventPageTest.NoBackgroundPage \
 --gtest_repeat=30

and I see the TIMEOUT failures sometimes (flake).

Also patched-in the change you mentioned [1] in #14, where you moved the stubs to the InProcessBrowserTest *constructor* rather than its SetUp(), and re-ran the browser tests again as above  ...

and I still see a TIMEOUT failure sometimes (flake), sorry to say.

I then removed the video-related gn args, namely:

 target_os = "chromeos"
 is_debug = false
 dcheck_always_on = true
 is_component_build = false
 use_goma = true

and built your patch from #14 with those.  Same result: saw TIMEOUT flakes :/

 Issue 895070  has been merged into this issue.

I've been looking into this more, but I still don't have a solid grasp of how my CL is causing this issue. I've also reached out to lazyboy@ and finnur@ (other //extensions/OWNERS), but I haven't made progress.

Is there someone with intimate knowledge of these tests? I'd like to schedule a VC to go through this since I'm having trouble making progress independently. Thanks!

Re #17 I was in fact referring to you Noel! You're now an expert in deflaking extensions API based tests so might have some experience to lend in this instance.

I don't know these tests and haven't worked on extensions in quite a while.
Devlin is probably the person you want. 

Update: I met with rdevlin.cronin@, and we dug into this problem a bit more. We realized that the vast majority of browser_tests do pass, but the specific ones that are failing all have one thing in common; namely, they deal with empty HTML files.

I experimented by adding HTML to the files tested by this test (functions.html and empty.html). As long as those files are *not* empty, the tests pass. So it seems that the issue is that the extensions do not behave properly when navigating to HTML pages without any content. The strange thing is that these tests used to pass reliably before my CL, and they continue to pass on all bots except for single-process Mash. We're continuing to investigate further.

I tested out a few more things:

First, I tried adding "run_at": "document_start" to the extension manifest at [1], but that did not make any difference. Then, I added a LOG statement to ScriptInjectionManager::RFOHelper::DidCreateDocumentElement() [2] to see if the document was actually created; with the two of these changes combined, the tests passed! However, oddly enough, removing one or both of these changes caused the test to fail again.

Thus, my conclusion is that this is some sort of race condition, since theoretically adding a log should not cause a change in logic. I've reached out to xiyuan@ (cc'ed), who is knowledgeable about single-process Mash, in order to make further progress about how/why this race condition is occurring and what the solution might be.

[1] https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/content_scripts/extension_api/manifest.json?l=9
[2] https://cs.chromium.org/chromium/src/extensions/renderer/script_injection_manager.cc?q=DidCreateDocumentElement

I finally got a repro on my box. My observation of the race is between ScriptInjectionManager::RFOHelper::StartInjectScripts [1] and UserScriptSetManager::OnUpdateUserScripts [2]. The first is called when we need to inject script at document_start/end and idle. The 2nd is called when browser sends the script to the renderer. 

ContentScriptExtensionAPIs has two scripts. Browser sends the both on extension load. When the test fails for me, my added loggin lines tells me that renderer receives the script after it attempts to inject.

Not sure how this is related to single process mash though.


[1] https://cs.chromium.org/chromium/src/extensions/renderer/script_injection_manager.cc?rcl=e6901925dd5b37d55accbac55564f639bf4f788a&l=254
[2] https://cs.chromium.org/chromium/src/extensions/renderer/user_script_set_manager.cc?rcl=e6901925dd5b37d55accbac55564f639bf4f788a&l=91

If I add a delay in ExtensionUserScriptLoader::LoadScripts [1], I could get this repro 100% in regular browser test as well (i.e. NOT single process mash).

[1] https://cs.chromium.org/chromium/src/extensions/browser/extension_user_script_loader.cc?rcl=5e70c69e2c55074c3a6ebfeafd7508938f134bdd&l=247

I chatted with rdevlin.cronin@, and he says that conceptually, we need to wait for the update to be received by the renderer before proceeding with the test.

Ideally, this should be part of the test infrastructure: we should wait for content script updates as part of adding extensions in tests. I've filed issue 898682 for this task.

In the interim, I'm working on a CL which adds a workaround that forces the test to evaluate some JS on the page before continuing with the test. This deflakes the tests until the real solution can be completed.

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

commit a61b89311be5de95b7b915a5452a1fe85e3f77f0
Author: Kyle Horimoto <khorimoto@google.com>
Date: Thu Oct 25 20:31:45 2018

Fix flaky ExtensionBrowserTests which load empty HTML files.

ExtensionBrowserTests which load empty HTML files are prone to race
conditions where the test proceeds to run before the renderer loads
relevant content scripts. This situation was exacerbated by a recent CL
I landed:

https://chromium-review.googlesource.com/c/chromium/src/+/1279067.

The true fix is to wait for an extension's content scripts to execute
before considering the extension loaded (see https://crbug.com/898682).
Implementing this fix is non-trivial, so this CL adds a temporary
solution to deflake the relevant tests in the meantime. Specifically,
this CL waits for the NOTIFICATION_USER_SCRIPTS_UPDATED event to be
fired after an extension is loaded before adding any test verifications.

Bug:  895068 
Change-Id: I65b364a72e7fdb435fcc5ec94a926837dadc5255
Reviewed-on: https://chromium-review.googlesource.com/c/1299578
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602839}
[modify] https://crrev.com/a61b89311be5de95b7b915a5452a1fe85e3f77f0/chrome/browser/extensions/content_script_apitest.cc
[modify] https://crrev.com/a61b89311be5de95b7b915a5452a1fe85e3f77f0/chrome/browser/extensions/wake_event_page_apitest.cc
[modify] https://crrev.com/a61b89311be5de95b7b915a5452a1fe85e3f77f0/testing/buildbot/filters/chromeos.single_process_mash.browser_tests.filter