New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 829345 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

SupervisedUserBlockModeTest.Unblock/0 Flaky

Project Member Reported by jonr...@chromium.org, Apr 5 2018

Issue description

OS: Windows
Test Suite: viz_browser_tests
           (browser_tests -enable-features=VizDisplayCompositor)
Test Case: SupervisedUserBlockModeTest.Unblock/0 Flaky

Example failing build: https://ci.chromium.org/buildbot/tryserver.chromium.win/win7_chromium_rel_ng/138083

Stack Trace of the failure:

[4816:2184:0404/225710.274:FATAL:interstitial_page_impl.cc(644)] Check failed: false.
Backtrace:
	base::debug::StackTrace::StackTrace [0x03722230+32]
	base::debug::StackTrace::StackTrace [0x036E734D+13]
	logging::LogMessage::~LogMessage [0x0368DE93+83]
	content::InterstitialPageImpl::Proceed [0x02556282+98]
	SupervisedUserService::OnSiteListUpdated [0x056A246F+79]
	SupervisedUserURLFilter::SetContents [0x056A56B0+176]
	base::internal::FunctorTraits<void (__thiscall content::VideoDecoderShim::*)(std::unique_ptr<content::VideoDecoderShim::PendingFrame,std::default_delete<content::VideoDecoderShim::PendingFrame> >),void>::Invoke<base::WeakPtr<content::VideoDecoderShim>,std [0x0572965B+47]
	base::internal::Invoker<base::internal::BindState<void (__thiscall policy::URLBlacklistManager::*)(std::unique_ptr<policy::URLBlacklist,std::default_delete<policy::URLBlacklist> >),base::WeakPtr<policy::URLBlacklistManager> >,void __cdecl(std::unique_ptr< [0x038BFDCD+61]
	base::OnceCallback<void __cdecl(std::unique_ptr<SupervisedUserURLFilter::Contents,std::default_delete<SupervisedUserURLFilter::Contents> >)>::Run [0x056A711C+48]
	base::internal::ReplyAdapter<std::unique_ptr<SupervisedUserURLFilter::Contents,std::default_delete<SupervisedUserURLFilter::Contents> >,std::unique_ptr<SupervisedUserURLFilter::Contents,std::default_delete<SupervisedUserURLFilter::Contents> > > [0x056A6F8C+44]
	base::internal::FunctorTraits<void (__cdecl*)(base::OnceCallback<bool __cdecl(void)>,bool *),void>::Invoke<base::OnceCallback<bool __cdecl(void)>,bool *> [0x0281D1DB+43]
	base::internal::Invoker<base::internal::BindState<void (__cdecl*)(base::OnceCallback<void __cdecl(bool,history::URLRow const &,std::vector<history::VisitRow,std::allocator<history::VisitRow> > const &)>,history::QueryURLResult const *),base::OnceCallback< [0x0281D199+41]
	base::internal::PostTaskAndReplyImpl::PostTaskAndReply [0x037338A4+1348]
	base::debug::TaskAnnotator::RunTask [0x0371E8FD+237]
	base::internal::IncomingTaskQueue::RunTask [0x03741D49+105]
	base::MessageLoop::RunTask [0x036C2E27+519]
...

I've added the SiteIsolation tag to help triage from the InterstitialPageImpl aspect. However that method has been stable for years.

bauerb@ as an owner of chrome/browser/supervised_user could you help triage this?

Marking this as P1 as the flakes have been affecting a significant number of jobs on the CQ
 
Cc: bauerb@chromium.org
Components: Services>SupervisedUser
Owner: carlosil@chromium.org
Status: Assigned (was: Untriaged)
+Carlos

This might get fixed with committed interstitials.
It will, since InterstitialPageImpl no longer plays a role with committed interstitials, but considering this is a P1 and supervised user committed interstitials won't be launched until 67 (and the non-committed code, including this test, removed once we launch at 100%), a separate fix should probably be used for the non-committed case in the meantime. 
It looks like the issue is that InterstitialPageImpl::Proceed() is getting called more than once for a single interstitial. Looks like Proceed() is only called from the URL filter change callback (https://cs.chromium.org/chromium/src/chrome/browser/supervised_user/supervised_user_interstitial.cc?rcl=de5d56c9ab59e532f61deb09937315b0346be021&l=334), are there any warranties that the callback would only get called once? Otherwise it might make sense to add a flag for 'proceed already called', and check that before calling proceed.
Status: Started (was: Assigned)
Hm, one would think that the first call to Proceed() would remove the interstitial -- I'm not sure why that doesn't happen.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 6 2018

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

commit 5d0b6a0755c81454481a2b548175dc36ecb1de55
Author: Carlos IL <carlosil@chromium.org>
Date: Fri Apr 06 17:33:46 2018

Add proceed check to supervised_user_interstitial

SupervisedUserInterstitial now keeps track of whether it called
Proceed() on interstitial_page_ and doesn't call it again if it has
already been called for the current interstitial.

Bug:  829345 
Change-Id: I2e5a66884bf9b9aa7182fdc70ea6629c38cb097b
Reviewed-on: https://chromium-review.googlesource.com/998062
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548841}
[modify] https://crrev.com/5d0b6a0755c81454481a2b548175dc36ecb1de55/chrome/browser/supervised_user/supervised_user_interstitial.cc
[modify] https://crrev.com/5d0b6a0755c81454481a2b548175dc36ecb1de55/chrome/browser/supervised_user/supervised_user_interstitial.h

Status: Fixed (was: Started)
That check should've fixed the flaky-ness, please reopen if you still see some failures.

Sign in to add a comment