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

Issue 631547 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

"ProfileWindowBrowserTest.GuestClearsCookies" is flaky due to late WeakPtr invalidation on incorrect thread

Project Member Reported by chromium...@appspot.gserviceaccount.com, Jul 26 2016

Issue description

"ProfileWindowBrowserTest.GuestClearsCookies" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyNgsSBUZsYWtlIitQcm9maWxlV2luZG93QnJvd3NlclRlc3QuR3Vlc3RDbGVhcnNDb29raWVzDA.

Flaky tests should be disabled within 30 minutes unless culprit CL is found and reverted. Please see more details here: https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues#triaging-auto-filed-flakiness-bugs

This flaky test/step was previously tracked in  issue 629335 .
 
Components: UI>Browser>Profiles
Cc: robliao@chromium.org gab@chromium.org fdoray@chromium.org
Labels: -Sheriff-Chromium
Summary: "ProfileWindowBrowserTest.GuestClearsCookies" is flaky due to late WeakPtr invalidation on incorrect thread (was: "ProfileWindowBrowserTest.GuestClearsCookies" is flaky)
[3516:1287:0726/110438:FATAL:weak_ptr.cc(20)] Check failed: sequence_checker_.CalledOnValidSequencedThread() || HasOneRef(). WeakPtrs must be invalidated on the same sequenced thread.
0   browser_tests                       0x0000000107a3b313 _ZN4base5debug10StackTraceC1Ev + 19
1   browser_tests                       0x0000000107a5b197 _ZN7logging10LogMessageD2Ev + 71
2   browser_tests                       0x0000000107a6b2ae _ZN4base8internal13WeakReference4Flag10InvalidateEv + 126
3   browser_tests                       0x0000000107a6b557 _ZN4base8internal18WeakReferenceOwnerD2Ev + 23
4   browser_tests                       0x000000010b025981 _ZN4base9SingletonIN5pnacl9PnaclHostENS_22DefaultSingletonTraitsIS2_EES2_E6OnExitEPv + 49
5   browser_tests                       0x0000000107a34a4b _ZN4base13AtExitManager19ProcessCallbacksNowEv + 219
6   browser_tests                       0x0000000107a34886 _ZN4base13AtExitManagerD2Ev + 118
7   browser_tests                       0x0000000107b33661 _ZN4base9TestSuiteD2Ev + 65
8   browser_tests                       0x0000000107a2a1d9 _ZN21ChromeTestSuiteRunner12RunTestSuiteEiPPc + 41
9   browser_tests                       0x0000000108125752 _ZN7content11LaunchTestsEPNS_20TestLauncherDelegateEiiPPc + 386
10  browser_tests                       0x00000001051db79a main + 90
11  browser_tests                       0x00000001051db734 start + 52
12  ???                                 0x0000000000000009 0x0 + 9
 Issue 631533  has been merged into this issue.

Comment 4 by gab@chromium.org, Jul 26 2016

Seems like a bug in that feature? Besides improve the checks on WeakPtr to catch this reliably rather than flakily I don't see what else we can do here (besides fixing this specific bug of course).
Yup. It's indeed a bug in that feature.

Comment 6 by gab@chromium.org, Aug 2 2016

 Issue 633466  has been merged into this issue.

Comment 7 by gab@chromium.org, Aug 2 2016

Labels: Sheriff-Chromium
Owner: gab@chromium.org
Status: Started (was: Untriaged)
CL to address this up @ https://codereview.chromium.org/2207683002/

Leaving Sheriff-Chromium label up as similar failures keep being filed by chromium-try-flakes@. Any failure to do with PnaclHost::DefaultSingletonTraits::OnExit is related to this (no easy test to disable, it's a race for all browser tests AFAICT).
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 2 2016

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

commit 459e9aa8dd4682aaaf5b8bcbb0fd030632b2d787
Author: gab <gab@chromium.org>
Date: Tue Aug 02 22:52:42 2016

Use a raw pointer instead of Singleton for PnaclHost and leak it.

This CL was brought up by  issue 631547  which highlights that PnaclHost is
sometimes deleted off-thread (on the main thread at static unitilization instead
of the IO thread it's bound to) with live WeakPtrs.

 Issue 631547  results in random flakes for all browser_tests exercising this
code one way or another, it should be fixed by this.

The WeakPtrs served no purpose as the task running system is long gone by the time
static destruction kicks in and no task would thus ever have been cancelled through
them.

It already leaked its state on static destruction from AtExitManager.
It could instead have used base::LeakySingletonTraits but it really doesn't
need to even be a Singleton per being intended for single-threaded usage.

So this CL:
 - Replaces WeakPtrs usage with Unretained(this).
 - Adds missing thread_checker_ calls now that they will no longer be implicitly
   provided by GetWeakPtr() calls.
 - Replaces Singleton with a leaked static raw pointer.
 - Replace callback_forward.h include by callback.h (typedefs need it
   and were getting it transitively through singleton.h->at_exit.h).
 - The attempted un-initialization through DeInitIfSafe() remains the same.

BUG= 631547 

Review-Url: https://codereview.chromium.org/2207683002
Cr-Commit-Position: refs/heads/master@{#409365}

[modify] https://crrev.com/459e9aa8dd4682aaaf5b8bcbb0fd030632b2d787/components/nacl/browser/pnacl_host.cc
[modify] https://crrev.com/459e9aa8dd4682aaaf5b8bcbb0fd030632b2d787/components/nacl/browser/pnacl_host.h
[modify] https://crrev.com/459e9aa8dd4682aaaf5b8bcbb0fd030632b2d787/components/nacl/browser/pnacl_host_unittest.cc

Labels: -Sheriff-Chromium
Can this be closed now? :)
Cc: bradnelson@chromium.org anthonyvd@chromium.org mlerman@chromium.org se...@chromium.org
 Issue 631926  has been merged into this issue.
 Issue 633322  has been merged into this issue.

Comment 12 by gab@chromium.org, Aug 3 2016

Status: Fixed (was: Started)
 Issue 659807  has been merged into this issue.
 Issue 659793  has been merged into this issue.

Sign in to add a comment