Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 2852333004: Fix possible deadlock during shutdown by not using the FILE thread. (Closed)

Created:
2 years ago by Marijn Kruisselbrink
Modified:
2 years ago
Reviewers:
gab, jam
CC:
chromium-reviews, darin-cc_chromium.org, fdoray, jam, robliao
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix possible deadlock during shutdown by not using the FILE thread. The leveldb service needs to be able to call code in the file service, even during browser shutdown. If the file service runs on the file thread, this will fail, leading to deadlock/timeout. Instead move the file service to a new SingleThreadTaskRunner managed by the task scheduler. Also re-enable tests that hopefully should be fixed by this. BUG=712872 BUG=586194 Review-Url: https://codereview.chromium.org/2852333004 Cr-Commit-Position: refs/heads/master@{#470169} Committed: https://chromium.googlesource.com/chromium/src/+/17399e67ce617b8936b5088dba92abb92b0cb8ee

Patch Set 1 #

Patch Set 2 : temporary change to show problem with task scheduler shutdown #

Total comments: 1

Patch Set 3 : undo temporary change, and add comment linking to mojo bug #

Total comments: 5

Patch Set 4 : use new way of specifying task traits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -19 lines) Patch
M content/browser/browser_context.cc View 1 2 3 2 chunks +8 lines, -4 lines 0 comments Download
M content/browser/dom_storage/dom_storage_browsertest.cc View 3 chunks +1 line, -15 lines 0 comments Download

Messages

Total messages: 38 (19 generated)
Marijn Kruisselbrink
gab: this change seems like a fairly straight forward replacing of a usage of a ...
2 years ago (2017-05-03 19:09:21 UTC) #8
gab
On 2017/05/03 19:09:21, Marijn Kruisselbrink wrote: > gab: this change seems like a fairly straight ...
2 years ago (2017-05-03 20:18:57 UTC) #9
Marijn Kruisselbrink
On 2017/05/03 at 20:18:57, gab wrote: > On 2017/05/03 19:09:21, Marijn Kruisselbrink wrote: > > ...
2 years ago (2017-05-03 21:06:29 UTC) #10
gab
On 2017/05/03 21:06:29, Marijn Kruisselbrink wrote: > On 2017/05/03 at 20:18:57, gab wrote: > > ...
2 years ago (2017-05-04 18:45:48 UTC) #11
gab
On 2017/05/04 18:45:48, gab wrote: > On 2017/05/03 21:06:29, Marijn Kruisselbrink wrote: > > On ...
2 years ago (2017-05-04 18:47:43 UTC) #12
gab
Improved documentation and check in https://codereview.chromium.org/2860063003/ so that you should now get the stack traces ...
2 years ago (2017-05-04 21:43:11 UTC) #13
Marijn Kruisselbrink
On 2017/05/04 at 18:45:48, gab wrote: > Looks like this other failure you're hitting is ...
2 years ago (2017-05-05 00:01:06 UTC) #14
gab
On 2017/05/05 00:01:06, Marijn Kruisselbrink wrote: > On 2017/05/04 at 18:45:48, gab wrote: > > ...
2 years ago (2017-05-05 11:23:46 UTC) #19
Marijn Kruisselbrink
On 2017/05/05 at 11:23:46, gab wrote: > I noticed that when I tried locally yesterday ...
2 years ago (2017-05-05 16:44:20 UTC) #20
Marijn Kruisselbrink
On 2017/05/05 at 16:44:20, Marijn Kruisselbrink wrote: > On 2017/05/05 at 11:23:46, gab wrote: > ...
2 years ago (2017-05-05 16:58:47 UTC) #21
Marijn Kruisselbrink
On 2017/05/05 at 16:58:47, Marijn Kruisselbrink wrote: > On 2017/05/05 at 16:44:20, Marijn Kruisselbrink wrote: ...
2 years ago (2017-05-05 17:21:49 UTC) #22
Marijn Kruisselbrink
+jam for content OWNERS
2 years ago (2017-05-08 17:07:46 UTC) #24
gab
https://codereview.chromium.org/2852333004/diff/40001/content/browser/browser_context.cc File content/browser/browser_context.cc (right): https://codereview.chromium.org/2852333004/diff/40001/content/browser/browser_context.cc#newcode489 content/browser/browser_context.cc:489: base::TaskTraits() See brand new way to specify TaskTraits as ...
2 years ago (2017-05-08 17:14:33 UTC) #25
gab
On 2017/05/05 17:21:49, Marijn Kruisselbrink wrote: > On 2017/05/05 at 16:58:47, Marijn Kruisselbrink wrote: > ...
2 years ago (2017-05-08 17:30:58 UTC) #26
Marijn Kruisselbrink
On 2017/05/08 at 17:30:58, gab wrote: > On 2017/05/05 17:21:49, Marijn Kruisselbrink wrote: > > ...
2 years ago (2017-05-08 18:18:01 UTC) #29
gab
On 2017/05/08 18:18:01, Marijn Kruisselbrink wrote: > On 2017/05/08 at 17:30:58, gab wrote: > > ...
2 years ago (2017-05-08 19:25:14 UTC) #30
jam
lgtm
2 years ago (2017-05-08 20:58:36 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2852333004/60001
2 years ago (2017-05-09 00:18:57 UTC) #35
commit-bot: I haz the power
2 years ago (2017-05-09 03:51:41 UTC) #38
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/17399e67ce617b8936b5088dba92...

Powered by Google App Engine
This is Rietveld 408576698