New issue
Advanced search Search tips

Issue 922951 link

Starred by 5 users

Issue metadata

Status: Verified
Owner:
Closed: Jan 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug
Flaky-Test: external/wpt/web-locks/mode-exclusive.tentative.https.any.worker.html



Sign in to add a comment

Web tests start flaking when we change Mojo dispatch timing

Project Member Reported by rockot@google.com, Jan 17 (5 days ago)

Issue description

Many web tests have become flaky since landing https://chromium-review.googlesource.com/c/chromium/src/+/1145692, but that CL is making a completely valid change to how incoming Mojo message dispatch operates. That is, none of Mojo bindings' guarantees are being violated.

This indicates that something fundamental in the web test framework is probably broken. In my observation so far, every flaky case has been a matter of snapshot timing: the web test actually does what's expected if run independently in content_shell, but the web test runner is too eager about checking for its expected output and thus does so before the page state has stabilized.

The CL in question has taken several months to land due to the frequency and volume with which incorrect test code (general test code, not web test code) is landed across the tree, so ideally this problem could be fixed quicklt without a revert.
 

Comment 1 by rockot@google.com, Jan 17 (5 days ago)

Summary: Web tests start flaking when we change Mojo dispatch timing (was: Layout tests start flaking when we change Mojo dispatch timing)

Comment 2 by rockot@google.com, Jan 17 (5 days ago)

Description: Show this description

Comment 3 by rockot@google.com, Jan 17 (5 days ago)

Cc: rockot@google.com
 Issue 922886  has been merged into this issue.

Comment 4 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922888  has been merged into this issue.

Comment 5 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922891  has been merged into this issue.

Comment 6 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922896  has been merged into this issue.

Comment 7 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922903  has been merged into this issue.

Comment 8 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922916  has been merged into this issue.

Comment 9 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922930  has been merged into this issue.

Comment 10 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922931  has been merged into this issue.

Comment 11 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922887  has been merged into this issue.

Comment 12 by rockot@google.com, Jan 17 (5 days ago)

Issue 922949 has been merged into this issue.
Project Member

Comment 13 by Findit, Jan 17 (5 days ago)

Project Member

Comment 14 by ClusterFuzz, Jan 17 (5 days ago)

Labels: OS-Linux
Project Member

Comment 15 by ClusterFuzz, Jan 17 (5 days ago)

Components: Internals>Mojo
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

Comment 16 by rockot@google.com, Jan 17 (5 days ago)

Cc: -rockot@google.com haraken@chromium.org rbyers@chromium.org dcheng@chromium.org
Adding a few random Blink owners. Any idea who might be most familiar with the precise machinery that drives web tests and could help me sort this out ASAP?

Comment 17 by rockot@google.com, Jan 17 (5 days ago)

Labels: OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows

Comment 18 by rockot@google.com, Jan 17 (5 days ago)

Flaky-Test: ----

Comment 19 by rockot@google.com, Jan 17 (5 days ago)

Cc: rockot@google.com
 Issue 922963  has been merged into this issue.

Comment 20 by rockot@google.com, Jan 17 (5 days ago)

 Issue 922964  has been merged into this issue.
Project Member

Comment 21 by Findit, Jan 17 (5 days ago)

 Issue 922964  has been merged into this issue.

Comment 23 by rockot@google.com, Jan 17 (5 days ago)

Flaky-Test: (many different tests findit please stop setting this field)

Comment 24 by wanderview@chromium.org, Jan 17 (5 days ago)

FWIW, I took a brief look at one of the failures; mode-exclusive.tentative.https.any.js:

https://cs.chromium.org/chromium/src/third_party/blink/web_tests/external/wpt/web-locks/mode-exclusive.tentative.https.any.js

The flaky failure are for the assert on line 33:

FAIL Requests for distinct resources can be granted assert_array_equals: property 0, expected 2 but got 1

Based on this it seems likely web-locks is dependent on the old task coalescing behavior in mojo for its expected behavior.  It doesn't look like a test runner problem.

Would it be possible to create a way for mojo interfaces to annotate whether they need the deprecated coalescing behavior?  If so then you could opt out the problematic interfaces in order to land the new default.  Then we could burn down the interfaces relying on the old behavior.

+jsbell for web-locks.

Comment 25 by wanderview@chromium.org, Jan 17 (5 days ago)

Cc: jsb...@chromium.org

Comment 26 by rockot@google.com, Jan 17 (5 days ago)

Yeah there are bound to be a few cases like that as well. Many of the failing tests seem like a runner issue though.

Comment 27 by rockot@google.com, Jan 17 (5 days ago)

There is a way to annotate interfaces by the way. mojo::Binding::EnableBatchDispatch(). If we can find specific interfaces that rely on the behavior, I'm OK with them calling this. But we still need to fix them ASAP.

Comment 29 by jsb...@chromium.org, Jan 17 (5 days ago)

Cc: mek@chromium.org
This is a case where we need an associated binding so that the requests are well-orderered to match the spec. Looking into it...

Comment 30 by jsb...@chromium.org, Jan 17 (5 days ago)

Sorry, comment #29 was about the mode-exclusive.tentative.https.any.js failure specifically.
Project Member

Comment 32 by Findit, Jan 18 (5 days ago)

 Issue 899335  has been merged into this issue.
Project Member

Comment 36 by Findit, Jan 18 (5 days ago)

Cc: japhet@chromium.org yhirano@chromium.org dbeam@chromium.org
 Issue 923064  has been merged into this issue.
Project Member

Comment 37 by Findit, Jan 18 (5 days ago)

 Issue 923228  has been merged into this issue.

Comment 42 by dbeam@chromium.org, Jan 18 (5 days ago)

Cc: -dbeam@chromium.org
Project Member

Comment 45 by Findit, Jan 18 (4 days ago)

 Issue 923218  has been merged into this issue.
Project Member

Comment 50 by Findit, Jan 18 (4 days ago)

 Issue 923230  has been merged into this issue.
Project Member

Comment 56 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 08248a881c2773108e375756ce77f9c83271e74c
Author: Ken Rockot <rockot@google.com>
Date: Fri Jan 18 18:59:34 2019

Disable a bunch of broken web tests

These tests break or become flaky after landing
https://chromium-review.googlesource.com/c/chromium/src/+/1145692
which is a legitimate change to how incoming Mojo messages are
dispatched.

Rather than blocking this relatively important change from landing
indefinitely, the broken tests are disabled until they can be fixed.

Bug:  922951 
Change-Id: Ibab2c609aa0c30ff7eb64920170561e0ad9ca732
Reviewed-on: https://chromium-review.googlesource.com/c/1422497
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#624222}
[modify] https://crrev.com/08248a881c2773108e375756ce77f9c83271e74c/third_party/blink/web_tests/TestExpectations

Comment 57 by rockot@google.com, Jan 18 (4 days ago)

Cc: orinj@chromium.org
 Issue 923405  has been merged into this issue.
Project Member

Comment 58 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129
Author: Joshua Bell <jsbell@chromium.org>
Date: Fri Jan 18 20:18:16 2019

Web Locks: Make mojo interfaces use associated bindings

The spec [1] defines a queue for operations such as requests and
releases. Operations within a given execution context are well ordered.
A change to Mojo dispatch timing [2] revealed that the code did not
implement this explicitly, identified by a now failing test.

Switch the interfaces to be "associated" to use a single ordered
pipe for requests and responses.

[1] https://wicg.github.io/web-locks
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1145692

Bug:  922951 
Test: external/wpt/web-locks/mode-exclusive.tentative.https.any.js
Change-Id: I01eb72b2bdda24590cbec56784218f3c8d43fc17
Reviewed-on: https://chromium-review.googlesource.com/c/1419247
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624257}
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/content/browser/locks/lock_manager.cc
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/content/browser/locks/lock_manager.h
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/third_party/blink/public/platform/modules/locks/lock_manager.mojom
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/third_party/blink/renderer/modules/locks/lock.cc
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/third_party/blink/renderer/modules/locks/lock.h
[modify] https://crrev.com/bb3ba4e8a35f83f2d22af762a9fc5aac1c5ab129/third_party/blink/renderer/modules/locks/lock_manager.cc

Project Member

Comment 59 by ClusterFuzz, Jan 19 (3 days ago)

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5075337419554816 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment