New issue
Advanced search Search tips

Issue 717740 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

"RestartTest.PRE_LocalStorageClearedOnExit" is flaky

Project Member Reported by chromium...@appspot.gserviceaccount.com, May 2 2017

Issue description

"RestartTest.PRE_LocalStorageClearedOnExit" 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=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyNAsSBUZsYWtlIilSZXN0YXJ0VGVzdC5QUkVfTG9jYWxTdG9yYWdlQ2xlYXJlZE9uRXhpdAw.

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
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 3 2017

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

commit dd799306dbacf94b73b935d9870d0119f06c2304
Author: msramek <msramek@chromium.org>
Date: Wed May 03 14:35:48 2017

Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac

TBR=sky@chromium.org
NOTRY=True
BUG= 717740 

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

[modify] https://crrev.com/dd799306dbacf94b73b935d9870d0119f06c2304/chrome/browser/sessions/better_session_restore_browsertest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 3 2017

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

commit e0c3dab76ce9e7b066b4b591c9cf5b0a9a856f3e
Author: msramek <msramek@chromium.org>
Date: Wed May 03 14:50:59 2017

Revert of Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac (patchset #1 id:1 of https://codereview.chromium.org/2858683003/ )

Reason for revert:
Sigh... a typo. Reverting.

Original issue's description:
> Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac
>
> TBR=sky@chromium.org
> NOTRY=True
> BUG= 717740 
>
> Review-Url: https://codereview.chromium.org/2858683003
> Cr-Commit-Position: refs/heads/master@{#468963}
> Committed: https://chromium.googlesource.com/chromium/src/+/dd799306dbacf94b73b935d9870d0119f06c2304

TBR=sky@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 717740 

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

[modify] https://crrev.com/e0c3dab76ce9e7b066b4b591c9cf5b0a9a856f3e/chrome/browser/sessions/better_session_restore_browsertest.cc

Another disabling CL, this time without a typo, is on its way.
Status: Available (was: Untriaged)
The test hasn't been modified around the time of first failure. The trace looks like this:

=========================================================

[3531:54823:0502/051849.975286:FATAL:sqlite_persistent_cookie_store.cc(140)] Check failed: !db_.get(). Close should have already been called.
0   browser_tests                       0x000000010501bdac base::debug::StackTrace::StackTrace(unsigned long) + 28
1   browser_tests                       0x00000001050439a0 logging::LogMessage::~LogMessage() + 224
2   browser_tests                       0x00000001080b8070 net::SQLitePersistentCookieStore::Backend::~Backend() + 112
3   browser_tests                       0x00000001080b74d4 base::internal::BindState<void (net::SQLitePersistentCookieStore::Backend::*)(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, base::Callback<void (std::__1::vector<std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> >, std::__1::allocator<std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > > >), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&, base::Time const&), scoped_refptr<net::SQLitePersistentCookieStore::Backend>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, base::Callback<void (std::__1::vector<std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> >, std::__1::allocator<std::__1::unique_ptr<net::CanonicalCookie, std::__1::default_delete<net::CanonicalCookie> > > >), (base::internal::CopyMode)1, (base::internal::RepeatMode)1>, base::Time>::Destroy(base::internal::BindStateBase const*) + 68
4   browser_tests                       0x0000000105083ac2 base::PendingTask::~PendingTask() + 18
5   browser_tests                       0x00000001050c01c9 base::internal::SchedulerWorker::Thread::ThreadMain() + 329
6   browser_tests                       0x00000001050d173f base::(anonymous namespace)::ThreadFunc(void*) + 95
7   libsystem_pthread.dylib             0x00007fff8c28f899 _pthread_body + 138
8   libsystem_pthread.dylib             0x00007fff8c28f72a _pthread_struct_init + 0
9   libsystem_pthread.dylib             0x00007fff8c293fc9 thread_start + 13


=========================================================

But sqlite_persistent_cookie_store.cc has not been modified for a long time. Must be in a higher layer.
For the record, the disabling CL is https://codereview.chromium.org/2858683003/.
Project Member

Comment 6 by bugdroid1@chromium.org, May 3 2017

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

commit 666f9f44c4e0aa571a167f7b9b9a8c15a6a0e964
Author: msramek <msramek@chromium.org>
Date: Wed May 03 17:31:08 2017

Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac

TBR=sky@chromium.org
BUG= 717740 

Review-Url: https://codereview.chromium.org/2858683003
Cr-Original-Commit-Position: refs/heads/master@{#468963}
Committed: https://chromium.googlesource.com/chromium/src/+/dd799306dbacf94b73b935d9870d0119f06c2304
Review-Url: https://codereview.chromium.org/2858683003
Cr-Commit-Position: refs/heads/master@{#469021}

[modify] https://crrev.com/666f9f44c4e0aa571a167f7b9b9a8c15a6a0e964/chrome/browser/sessions/better_session_restore_browsertest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 3 2017

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

commit fe342d7b2c65097f714739bb6dabe33a1ec8b107
Author: msramek <msramek@chromium.org>
Date: Wed May 03 18:33:39 2017

Revert of Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac (patchset #3 id:60001 of https://codereview.chromium.org/2858683003/ )

Reason for revert:
And reverting again, because MAYBE_ and PRE_ directives cannot be used together.

Original issue's description:
> Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac
>
> TBR=sky@chromium.org
> BUG= 717740 
>
> Review-Url: https://codereview.chromium.org/2858683003
> Cr-Original-Commit-Position: refs/heads/master@{#468963}
> Committed: https://chromium.googlesource.com/chromium/src/+/dd799306dbacf94b73b935d9870d0119f06c2304
> Review-Url: https://codereview.chromium.org/2858683003
> Cr-Commit-Position: refs/heads/master@{#469021}
> Committed: https://chromium.googlesource.com/chromium/src/+/666f9f44c4e0aa571a167f7b9b9a8c15a6a0e964

TBR=sky@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 717740 

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

[modify] https://crrev.com/fe342d7b2c65097f714739bb6dabe33a1ec8b107/chrome/browser/sessions/better_session_restore_browsertest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 3 2017

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

commit dba5cb7d9fe8705651cb0d04895dbb8ea1691421
Author: msramek <msramek@chromium.org>
Date: Wed May 03 18:44:16 2017

Reland of Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac (patchset #1 id:1 of https://codereview.chromium.org/2862583003/ )

Reason for revert:
...and relanding, because I'm trigger-happy and misread the bug report. There wasn't a problem with this reland, but with the previous one, which was already reverted.

Original issue's description:
> Revert of Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac (patchset #3 id:60001 of https://codereview.chromium.org/2858683003/ )
>
> Reason for revert:
> And reverting again, because MAYBE_ and PRE_ directives cannot be used together.
>
> Original issue's description:
> > Disable the flaky RestartTest.PRE_LocalStorageClearedOnExit on Mac
> >
> > TBR=sky@chromium.org
> > BUG= 717740 
> >
> > Review-Url: https://codereview.chromium.org/2858683003
> > Cr-Original-Commit-Position: refs/heads/master@{#468963}
> > Committed: https://chromium.googlesource.com/chromium/src/+/dd799306dbacf94b73b935d9870d0119f06c2304
> > Review-Url: https://codereview.chromium.org/2858683003
> > Cr-Commit-Position: refs/heads/master@{#469021}
> > Committed: https://chromium.googlesource.com/chromium/src/+/666f9f44c4e0aa571a167f7b9b9a8c15a6a0e964
>
> TBR=sky@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 717740 
>
> Review-Url: https://codereview.chromium.org/2862583003
> Cr-Commit-Position: refs/heads/master@{#469040}
> Committed: https://chromium.googlesource.com/chromium/src/+/fe342d7b2c65097f714739bb6dabe33a1ec8b107

TBR=sky@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 717740 

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

[modify] https://crrev.com/dba5cb7d9fe8705651cb0d04895dbb8ea1691421/chrome/browser/sessions/better_session_restore_browsertest.cc

Cc: sky@chromium.org
Components: -Tests>Flaky UI>Browser>Sessions
Labels: -Sheriff-Chromium -Via-TryFlakes OS-Mac
sky@ do you know who the right person should be to own?
Owner: mmenke@chromium.org
Assigning to mmenke@, leaving sky@ on cc

This looks like a shutdown issue in SQLitePersistentCookieStore.

Based on the stack trace, net::SQLitePersistentCookieStore::Backend is getting destroyed before it has a chance to close itself properly. Backend is a scoped_refptr in SQLitePersistentCookieStore, and based on the stack trace, it gets destroyed when a task with a refernce to it gets destroyed. So it seems what happens is SQLitePersistentCookieStore::Backend::Close() is called, it posts a background task to call Backend::InternalBackgroundClose(), but then everything including the background_task_runner_ and the tasks it contains gets teared down during, before the task has a chance to execute.
Cc: mmenke@chromium.org
Components: Internals>Network>Cookies
Owner: ----
I just don't have time to learn this code.  I wouldn't be surprised if this is fallout from the task runner refactorings, making something not run on quit that used to run on quit.

Comment 12 by sky@chromium.org, May 4 2017

I'm not at all familiar with this code.
Owner: fdoray@chromium.org
Status: Assigned (was: Available)
[+fdoray] Suspecting https://codereview.chromium.org/2841683002, which changed the thread that cookie tasks are running on.  CL also landed on the 24th, which is when the test started flaking regularly.
Project Member

Comment 14 by bugdroid1@chromium.org, May 8 2017

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

commit cc174e5c89d6c5f8443ff8c4efcb58749029a8c8
Author: fdoray <fdoray@chromium.org>
Date: Mon May 08 15:35:30 2017

Use TaskShutdownBehavior::BLOCK_SHUTDOWN in content::CreateCookieStore().

The SequencedTaskRunner created in content::CreateCookieStore() is
used to run a task that closes an sql::Connection. That task cannot
be skipped on shutdown.

BUG= 717740 

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

[modify] https://crrev.com/cc174e5c89d6c5f8443ff8c4efcb58749029a8c8/chrome/browser/sessions/better_session_restore_browsertest.cc
[modify] https://crrev.com/cc174e5c89d6c5f8443ff8c4efcb58749029a8c8/content/browser/net/quota_policy_cookie_store.cc

Status: Fixed (was: Assigned)

Sign in to add a comment