Flaky failures in fast/selectors/selection-window-inactive.html |
||||
Issue descriptionI've got a simple CL that resets state between WebKit test runs: https://chromium-review.googlesource.com/c/chromium/src/+/1246723 This causes semi-deterministic failures in fast/selectors/selection-window-inactive.html on Windows. [e.g. adding logging causes the failures to go away]. From visual inspection, the problem seems to be that the test doesn't wait for the effects of setWindowIsKey() to propagate and update rendering. """ <!DOCTYPE html> <style> ::selection { background-color: blue; color: yellow; } ::selection:window-inactive { background-color: rgba(63, 128, 33, 0.95); /* green; alpha < 1 so that we don't blend the background color with white. */ color: black; } </style> <span>Any textual selection in this sentence should have a green background when the window is inactive.</span> <script> var span = document.querySelector("span"); window.getSelection().setBaseAndExtent(span, 0, span, 1); if (window.testRunner) testRunner.setWindowIsKey(false); </script> """
,
Sep 27
Another set of track traces, this time including the browser process. This shows the following series of events:
0) [BROWSER] Missing stack trace [forgot to add logs], but we know browser calls GetLayoutTestControlPtr(frame)->SetTestConfiguration.
1) [BROWSER] content::BlinkTestController::PrepareForLayoutTest
--> content::Shell::LoadURL
--> aura::Window::Focus
2) [BROWSER] content::BlinkTestController::PrepareForLayoutTest
--> content::RenderWidgetHostImpl::Focus
3) [RENDERER] content::LayoutTestRenderFrameObserver::SetTestConfiguration [sets focus = 1]
4) [RENDERER] TestRunnerForSpecificView::SetWindowIsKey [sets focus = 0]
5) [RENDERER] content::WidgetInputHandlerImpl::SetFocus [sets focus = 1]
6) [RENDERER] content::WidgetInputHandlerImpl::SetFocus [sets focus = 1]
,
Sep 27
The first thing I tried was to change the ordering of the calls to SetFocus() [relative to LoadURL] in Shell::LoadURLForFrame and BlinkTestController::PrepareForLayoutTest Unfortunately, this is insufficient to remove the race. The problem is that the messages are being sent across different mojom interfaces and there are no ordering guarantees between them.
,
Sep 27
+ dtapuska -- feedback requested. Two possible solutions: 1) Add a flush() to WidgetInputHandler and LayoutTestControl. Make sure to flush them before calling LoadURL(). Will need some modifications to content shell to make sure that we aren't calling any methods after those flushes. 2) Use associated interfaces to get deterministic ordering on these multiple mojo pipes. + rockot -- FYI
,
Sep 27
,
Sep 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b7c87c15bf0a5e717c343c8195d93c3960cd7757 commit b7c87c15bf0a5e717c343c8195d93c3960cd7757 Author: erikchen <erikchen@chromium.org> Date: Thu Sep 27 23:14:57 2018 Mark fast/selectors/selection-window-inactive.html as flaky. The test depends on ordering of IPCs across unassociated Mojo channels. This ordering is not deterministic. Bug: 889952 Change-Id: I23f9d69a5b2fc3e43eefe4241a151ccf1f20cfc1 Reviewed-on: https://chromium-review.googlesource.com/1250008 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#594912} [modify] https://crrev.com/b7c87c15bf0a5e717c343c8195d93c3960cd7757/third_party/WebKit/LayoutTests/TestExpectations
,
Sep 28
Chatted with rockot@ off thread. Solution (2) doesn't actually work. Associated interfaces "require you to know all of the mutually associated endpoints run between the same two processes". We can't guarantee this assumption between browser<->renderer, especially as new services get added [e.g. network service]. rockot@ suggests adding flushes() on a case-by-case basis. I'll begin to do so.
,
Oct 2
I agree with rockot@ if you made these associated interfaces that would likely mean that the messages would need to goto the main thread first. The desire is that input hits the compositor thread first.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/026da0aaf6c13516addf2fb7744d501919f1dac9 commit 026da0aaf6c13516addf2fb7744d501919f1dac9 Author: erikchen <erikchen@chromium.org> Date: Tue Oct 02 19:28:34 2018 Uniform initialization for WebContents in layout tests. This CL begins the process of removing initialization races from Blink layout tests. Prior to this CL, a newly created main_window_ used a different navigation mechanism than a reused main_window_. The latter would use a navigation with ui::PAGE_TRANSITION_LINK [with an already constructed renderer]. The former would spawn a renderer during the ui::PAGE_TRANSITION_TYPED navigation to the layout test URL. This caused races between IPCs that occur during renderer construction, and IPCs that occur as a result of loading the layout test. This CL doesn't fix these races yet -- it simply converts initialization to use the same mechanism for both cases [using ui::PAGE_TRANSITION_LINK with an already constructed renderer]. Change-Id: I5a21bfdd9a47425f1eb9579235da5ffb4fbc19f7 Bug: 889036 , 889952 Reviewed-on: https://chromium-review.googlesource.com/1257683 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#595944} [modify] https://crrev.com/026da0aaf6c13516addf2fb7744d501919f1dac9/content/shell/browser/layout_test/blink_test_controller.cc
,
Oct 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0908efa5c766779e14e3d0a5472b9d0058ce9e85 commit 0908efa5c766779e14e3d0a5472b9d0058ce9e85 Author: erikchen <erikchen@chromium.org> Date: Wed Oct 03 19:32:13 2018 Flush WidgetInputHandler messages before running layout tests. Layout test initialization is non-deterministic. WidgetInputHandler::OnFocus() races against the contents of the layout test, which may also attempt to set/unset focus -- the ordering between the two is non-deterministic. This CL adds a FlushForTesting() message to WidgetInputHandler to make the ordering explicit. Bug: 889036 , 889952 Change-Id: I26adca82915e75a9941c93b60a555f0c16084014 Reviewed-on: https://chromium-review.googlesource.com/c/1255782 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#596321} [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/common/input/input_handler.mojom [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/public/browser/render_widget_host.h [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/renderer/input/widget_input_handler_impl.cc [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/renderer/input/widget_input_handler_impl.h [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/shell/browser/layout_test/blink_test_controller.cc [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/test/mock_widget_input_handler.cc [modify] https://crrev.com/0908efa5c766779e14e3d0a5472b9d0058ce9e85/content/test/mock_widget_input_handler.h
,
Oct 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/10175580ca11afa28314a5851c2868d9d9906084 commit 10175580ca11afa28314a5851c2868d9d9906084 Author: Daniel Cheng <dcheng@chromium.org> Date: Wed Oct 10 20:50:08 2018 Use the builtin FlushForTesting rather than defining new Mojo messages. Bug: 889036 , 889952 Change-Id: If4f52524e2378589c457f7bb0b41511d8356c05b Reviewed-on: https://chromium-review.googlesource.com/c/1272024 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Commit-Queue: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#598491} [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/common/input/input_handler.mojom [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/public/browser/render_widget_host.h [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/renderer/input/widget_input_handler_impl.cc [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/renderer/input/widget_input_handler_impl.h [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/shell/browser/layout_test/blink_test_controller.cc [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/shell/browser/layout_test/blink_test_controller.h [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/shell/common/layout_test.mojom [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/shell/renderer/layout_test/layout_test_render_frame_observer.cc [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/shell/renderer/layout_test/layout_test_render_frame_observer.h [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/test/mock_widget_input_handler.cc [modify] https://crrev.com/10175580ca11afa28314a5851c2868d9d9906084/content/test/mock_widget_input_handler.h
,
Oct 17
|
||||
►
Sign in to add a comment |
||||
Comment 1 by erikc...@chromium.org
, Sep 27