Miscellaneous test failures with bindings dispatch timing change applied |
|||
Issue descriptionThese 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
,
Oct 17
,
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
,
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
,
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
,
Oct 20
|
|||
►
Sign in to add a comment |
|||
Comment 1 by roc...@chromium.org
, Oct 17