Android layout tests fail to run after stdio streams aren't redirected |
||||||||
Issue descriptionThis test fails for AVDA (and MCVD which is why I found it). I'm not sure how important it is to fix, but I thought it might be indicating a real bug. It fails with: "assert_false: sourceBuffer.updating synchronously after duration reduction". If I removed that line and it then passed (thought beware, it was difficult to get the test to start using the new .js file after editing it; it seems to be cached somewhere). To repro is kind of painful: ninja -C out/a content_shell_apk_incremental && out/a/bin/install_content_shell_apk_incremental adb reverse tcp:8000 tcp:8000 adb reverse tcp:8080 tcp:8080 ./third_party/WebKit/Tools/Scripts/run-blink-httpd adb_content_shell_command_line --ignore-autoplay-restrictions adb_run_content_shell http://127.0.0.1:8000/media/media-source/mediasource-config-change-mp4-av-framesize.html
,
Aug 9 2017
A tiny bit of further info: --enable-experimental-web-platform-features is triggered to be added later (at https://cs.chromium.org/chromium/src/content/shell/app/shell_main_delegate.cc?rcl=a5547d985132311c42cc03a910c277f37c074c1e&l=186) by the layout test runner, which gives content_shell --run-layout-test as a parameter. In this case, experimental features are always tested *unless* --stable-release-mode is also included in the cmdline.
,
Aug 9 2017
+dpranke@ - it is weird that the nexus 4 webkit bot is not running any layout tests. Is this a known issue?
,
Aug 9 2017
Yes, it's weird, and no, it's not an issue I know about. There's at least two issues. First, it looks like content_shell isn't starting up properly. Second, it looks like that failure is getting swallowed, instead of turning the step red :(. qyearsley@ can you look into the second problem, since that's actually the worse of the two? jbudorick@, any suggestions on the first?
,
Aug 9 2017
,
Aug 9 2017
Didn't get time to start looking at this today, will follow up tomorrow. I'm assuming that in general, steps should generally fail as long as the command has a non-zero exit code, right?
,
Aug 9 2017
Yes, but in this case run-webkit-tests looks to have returned an exit code of zero, even though it shouldn't have (we didn't run any tests, probably because we didn't have any devices).
,
Aug 10 2017
re 1: Yep, that was it :) Thanks. I'll retarget this bug towards the bot issue.
,
Aug 10 2017
Meanwhile for the issue where it had an exit code of zero when it shouldn't have, there is a proposed fix at https://chromium-review.googlesource.com/c/610486.
,
Aug 11 2017
I've got this reproducing reliably locally. Something strange is going on with stream redirection. More to come.
,
Aug 11 2017
We're never creating the IO thread? - ScopedAndroidConfiguration posts three tasks to the IO thread to redirect stdout, stderr, and stdin to/from sockets. Those tasks are never run, though, causing ../layout_tests/port/android.py to time out waiting for #READY. - Those tasks are never run because the IO thread is never created. It *should* be created in BrowserMainLoop::CreateThreads: https://codesearch.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=c39438cf45dcc73c901103aa7a828e21b8e052a5&l=1084 - BrowserMainLoop::CreateThreads is posted to BrowserMainLoop::startup_task_runner_: https://codesearch.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=f156b499f700f849d110801e8a303c00c98ae5bc&l=915 - ... but that task never runs. :( There are a few #if defined(OS_ANDROID)s in browser_main_loop.cc that I'm experimenting with locally. e.g., forcing the startup_task_runner_ to run all tasks at the end of CreateStartupTasks (basically, removing this OS_ANDROID: https://codesearch.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=f156b499f700f849d110801e8a303c00c98ae5bc&l=927) lets layout tests proceed as normal.
,
Aug 11 2017
+cc boliu: this is probably related to https://chromium.googlesource.com/chromium/src/+/79c92c3bf366a37804325c8c8021f8ab62e80339 Adding a call to BrowserMainLoop::SynchronouslyFlushStartupTasks in layout_test_browser_main.cc also works. Uploading a CL to do so shortly.
,
Aug 11 2017
oh, layout tests relies on the default value of not allowing async start up, which no longer exists and code now defaults to async. yeah adding flush is the right thing then start up is so complicated :/
,
Aug 14 2017
watk@ noted in #8 that we can retarget this bug towards the failure itself; CL for that is https://chromium-review.googlesource.com/c/612442/
,
Aug 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d commit a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d Author: John Budorick <jbudorick@chromium.org> Date: Mon Aug 14 20:03:03 2017 [android] Ensure startup tasks run in layout tests. Bug: 753702 Change-Id: I4007f416df41e09ec0a229f78d7259b6fa7440c7 Reviewed-on: https://chromium-review.googlesource.com/612442 Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Commit-Queue: John Budorick <jbudorick@chromium.org> Cr-Commit-Position: refs/heads/master@{#494151} [modify] https://crrev.com/a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d/content/browser/browser_main_runner.cc [modify] https://crrev.com/a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d/content/public/browser/browser_main_runner.h [modify] https://crrev.com/a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d/content/shell/browser/layout_test/layout_test_browser_main.cc [modify] https://crrev.com/a797150b70431416aa5f6b4ffcc5f0eb0a9c7a3d/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/driver.py
,
Aug 14 2017
Updating issue title to reflect the scope of the problem -- tests weren't just unable to run on the N4 bot, they were unable to run at all. The fix in #15 should address this and will be live on the N4 bot as of build #68444. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by wolenetz@chromium.org
, Aug 9 2017Status: Unconfirmed (was: Available)