New issue
Advanced search Search tips

Issue 895693 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 20
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 866708



Sign in to add a comment

Miscellaneous test failures with bindings dispatch timing change applied

Project Member Reported by roc...@chromium.org, Oct 16

Issue description

These need to be resolved to land https://chromium-review.googlesource.com/c/chromium/src/+/1145692

All failures indicate either incorrect prod code or incorrect test expectations, usually the latter.

Failing tests in content_unittests (all platforms):

  MediaSessionImplTest.SessionInfoState
  ClipboardHostImplTest.ReentrancyInSyncCall
  FrameSinkVideoCaptureDeviceTest.CapturesAndDeliversFrames
 
Owner: rockot@google.com
Description: Show this description
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 17

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

commit 967d73003eea4e497b7ed90f93456575cfa77a84
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Oct 17 18:38:26 2018

Fix ClipboardHostImplTest.ReentrancyInSyncCall

The CL https://chromium-review.googlesource.com/c/chromium/src/+/1145692
changes Mojo dispatch behavior and ends up breaking this test.

The test is making invalid assumptions about when an InterfacePtr can
expect encountered_error() to return true.

This CL fixes the test be ensuring that it explicitly waits for an
error to be raised on the clipboard interface before continuing.

Bug:  895693 
Change-Id: I17ea7f994ea9c7d672d0f200aeeaa2c404e04f3f
Reviewed-on: https://chromium-review.googlesource.com/c/1286203
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600491}
[modify] https://crrev.com/967d73003eea4e497b7ed90f93456575cfa77a84/content/browser/renderer_host/clipboard_host_impl_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 17

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

commit e2ea2d5bba4d1f829cf427bdd5c37dd247efcae3
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Oct 17 23:43:43 2018

Fix FrameSinkVideoCaptureDevice tests

Fixes the behavior of some of these tests with
https://chromium-review.googlesource.com/c/chromium/src/+/1145692
applied. That CL changes how Mojo bindings dispatch messages, but
only timing (not ordering) is changed. This can break subtle test
expecations, as is the case here.

This change replaces use of deprecated RunAllPendingInMessageLoop with
either RunLoop usage or TestBrowserThreadBundle::RunIOThreadUntilIdle.
In the latter case, in order to achieve synchronization parity with the
code before this change, note that it is also necessary to have
WAIT_FOR_DEVICE_TASKS wait for the UI thread to idle after the IO
thread has idled, due to the way RunAllPendingInMessageLoop worked.

Bug:  895693 
Change-Id: I15552766245eda0bdce141c40643ee4b44cc90a8
Reviewed-on: https://chromium-review.googlesource.com/c/1286432
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600601}
[modify] https://crrev.com/e2ea2d5bba4d1f829cf427bdd5c37dd247efcae3/content/browser/media/capture/frame_sink_video_capture_device_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 20

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

commit e793e1bc879cfdbec97428a91e12f7984550f588
Author: Ken Rockot <rockot@chromium.org>
Date: Sat Oct 20 00:02:53 2018

Fix AudioFocusDelegateDefault behavior

This was revealed by
https://chromium-review.googlesource.com/c/chromium/src/+/1145692
which makes subtle changes to Mojo message dispatch timing and can
thus expose incorrect assumptions in various bits of code.

In this case, there is a race between AbandonAudioFocus and
receiving a reply from a previous call to RequestAudioFocus on
the audio_focus_ptr_. The callback which handles the latter reply
DCHECKs that request_client_ptr_ is bound, but it's possible for
AbandonAudioFocus to have been called before that reply is
dispatched.

The above Mojo CL in question causes some unit tests to flake by
hitting this DCHECK occasionally.

This CL corrects the issue by also resetting audio_focus_ptr_ in
AbandonAudioFocus, which ensures that any pending replies on that
interface will *not* be dispatched.

This also fixes a bug in AudioFocusManagerMetricsHelper (also
revealed by this CL) where it was retaining a *reference* to an
unowned std::string that could be deleted before the helper. This
is because the string was ultimately owned by a binding endpooint,
but the helper is owned by a StackRow which can outlive any given
binding endpoint. Because the name is effectively only useful at
helper construction time, this simply changes the helper to retain
a copy instead of a reference.

TBR=mlamouri@chromium.org

Bug:  895693 
Change-Id: I958b783d005cb85dc9dc9f7371df100883d774cb
Reviewed-on: https://chromium-review.googlesource.com/c/1286202
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601362}
[modify] https://crrev.com/e793e1bc879cfdbec97428a91e12f7984550f588/content/browser/media/session/audio_focus_delegate_default.cc
[modify] https://crrev.com/e793e1bc879cfdbec97428a91e12f7984550f588/services/media_session/audio_focus_manager_metrics_helper.h

Status: Fixed (was: Assigned)

Sign in to add a comment