Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904273002:
Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks (Closed)
|
Created:
4 years, 8 months ago by Patrick Monette Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, manzagop (departed) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks
CONTINUE_ON_SHUTDOWN means the thread running the task will not be
joined on shutdown and may not have a valid singleton/lazy_instance
at that moment.
Also made DhcpcsvcInitSingleton explicitly leaky to avoid
the AssertSingletonAllowed() dcheck.
BUG=478209
Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b
Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0
Committed: https://crrev.com/7eebe86636dc5f9bbe9c7bec367e99569894d34a
Cr-Original-Original-Commit-Position: refs/heads/master@{#390125}
Cr-Original-Commit-Position: refs/heads/master@{#406053}
Cr-Commit-Position: refs/heads/master@{#406293}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Comments #
Total comments: 1
Patch Set 3 : Rebase #Patch Set 4 : Gab comment #Patch Set 5 : Rebase #Patch Set 6 : Made a Singleton leaky in test_support_android.cc #
Messages
Total messages: 63 (25 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. BUG=478209 ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made a LazyInstance explicitly leaky. BUG=478209 ==========
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made a LazyInstance explicitly leaky. BUG=478209 ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky. BUG=478209 ==========
pmonette@chromium.org changed reviewers: + jam@chromium.org, mef@chromium.org
mef@ PTAL at net/ change. jam@ PTAL at sequenced_worker_pool.cc
On 2016/04/21 23:07:43, Patrick Monette wrote: > mef@ PTAL at net/ change. > jam@ PTAL at sequenced_worker_pool.cc I'm not a good reviewer for that file, i'm just in the owners file for thread_restrictions.h
pmonette@chromium.org changed reviewers: + danakj@chromium.org
Hey danakj@ I'm trying to find an owner for base/threading/sequenced_worker_pool.cc. PTAL or please help me find an owner. Thanks!
jam@chromium.org changed reviewers: - jam@chromium.org
danakj@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
+gab,robliao any thoughts?
This currently seems reasonable to me in my reading of the situation. A task that is CONTINUE_ON_SHUTDOWN behaves like a non joinable thread. Regular singletons care about non joinable threads and assert when they are run on one. https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:251: // that implements base::Singleton. This will trigger a DCHECK to warn of Put why this is the case in the comment as well. (e.g. the singleton is generally destroyed at exit).
https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (right): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:24: // Worker pool threads that use the DHCP API may still be running. Leak instance s/running/running at shutdown/
@danakj: yes, this makes sense. We are in fact planning to add this exact logic to base/task_scheduler and knew this was a problem with SequencedWorkerPool. Patrick needed to fix some shutdown tasks and was worried about the lack of such a check in the existing code so I suggested he add one to the SequencedWorkerPool pending base/task_scheduler support and migration. Idea lgtm % code comments https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (left): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:23: ~DhcpcsvcInitSingleton() { Make the destructor private w/ a comment of why it should always be leaked. https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (right): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:27: g_dhcpcsvc_init_singleton = LAZY_INSTANCE_INITIALIZER; Is the change in this file required to land the other one? Feels unrelated to CONTINUE_ON_SHUTDOWN?
manzagop@chromium.org changed reviewers: + manzagop@chromium.org
https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:251: // that implements base::Singleton. This will trigger a DCHECK to warn of On 2016/04/25 23:03:30, robliao wrote: > Put why this is the case in the comment as well. (e.g. the singleton is > generally destroyed at exit). Also mention leaky versions are fine to access from nonjoinable threads?
PTAnL https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:251: // that implements base::Singleton. This will trigger a DCHECK to warn of On 2016/04/25 23:03:30, robliao wrote: > Put why this is the case in the comment as well. (e.g. the singleton is > generally destroyed at exit). Done. https://codereview.chromium.org/1904273002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.cc:251: // that implements base::Singleton. This will trigger a DCHECK to warn of On 2016/04/26 13:13:44, manzagop wrote: > On 2016/04/25 23:03:30, robliao wrote: > > Put why this is the case in the comment as well. (e.g. the singleton is > > generally destroyed at exit). > > Also mention leaky versions are fine to access from nonjoinable threads? Done. https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (left): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:23: ~DhcpcsvcInitSingleton() { On 2016/04/26 11:53:18, gab wrote: > Make the destructor private w/ a comment of why it should always be leaked. The comment that explain that is Worker pool threads that use the DHCP API may still be running. Leak instance and skip cleanup. Do you think it's not clean enough? https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (right): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:24: // Worker pool threads that use the DHCP API may still be running. Leak instance On 2016/04/25 23:05:25, robliao wrote: > s/running/running at shutdown/ Done. https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:27: g_dhcpcsvc_init_singleton = LAZY_INSTANCE_INITIALIZER; On 2016/04/26 11:53:18, gab wrote: > Is the change in this file required to land the other one? Feels unrelated to > CONTINUE_ON_SHUTDOWN? Threads with CONTINUE_ON_SHUTDOWN behavior were accessing this singleton and the DCHECK in sequenced_worker_pool was firing. I could have made this change in a separate CL before the other one, but it is such a small change and it gives context as to why I'm doing it.
https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (left): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:23: ~DhcpcsvcInitSingleton() { On 2016/04/26 19:05:28, Patrick Monette wrote: > On 2016/04/26 11:53:18, gab wrote: > > Make the destructor private w/ a comment of why it should always be leaked. > > The comment that explain that is Worker pool threads that use the DHCP API may > still be running. Leak instance and skip cleanup. > > Do you think it's not clean enough? clear*
Sorry for the noise, still lgtm w/ request to undo one of my previous comments... https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (right): https://codereview.chromium.org/1904273002/diff/20001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:27: g_dhcpcsvc_init_singleton = LAZY_INSTANCE_INITIALIZER; On 2016/04/26 19:05:27, Patrick Monette wrote: > On 2016/04/26 11:53:18, gab wrote: > > Is the change in this file required to land the other one? Feels unrelated to > > CONTINUE_ON_SHUTDOWN? > > Threads with CONTINUE_ON_SHUTDOWN behavior were accessing this singleton and the > DCHECK in sequenced_worker_pool was firing. > > I could have made this change in a separate CL before the other one, but it is > such a small change and it gives context as to why I'm doing it. Ah I see, it wasn't obvious to me how the two were linked (hadn't realized LazyInstance::Leaky had a special case skipping the AssertSingletonAllowed() check :-)). Perhaps explain this in the CL description? https://codereview.chromium.org/1904273002/diff/40001/net/proxy/dhcpcsvc_init... File net/proxy/dhcpcsvc_init_win.cc (right): https://codereview.chromium.org/1904273002/diff/40001/net/proxy/dhcpcsvc_init... net/proxy/dhcpcsvc_init_win.cc:24: ~DhcpcsvcInitSingleton() {} Actually never mind the private destructor, I'd misunderstood the bug as highlighted in my other comment (I thought somehow running the destructor was the issue and thus that we should make sure it doesn't run). The issue is with the instance going away and thus it makes more sense to document this via the Leaky instance and forgo the private destructor (which is an unnecessary burden that could one day prevent proper unit-testing).
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky. BUG=478209 ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 ==========
Patchset #3 (id:60001) has been deleted
mef@ & danakj@ PTAL as I need owners review
lgtm
LGTM
pmonette@chromium.org changed reviewers: + rsleevi@chromium.org
rsleevi@ PTAL. Need owners review for dhcpcsvc_init_win.cc
net/ lgtm
Thanks mef@. I won't be needing your review anymore Ryan!
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/1904273002/#ps100001 (title: "Gab comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904273002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904273002/100001
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/1929543003/ by robertshield@chromium.org. The reason for reverting is: This seems to have broken some android unit tests: I 181.858s run_tests_on_device(01ab756b0330210f) [ RUN ] SupervisedUserServiceTest.ChangesIncludedSessionOnChangedSettings I 181.858s run_tests_on_device(01ab756b0330210f) [INFO:profile_sync_service.cc(2370)] ConfigureDataTypeManager not invoked because backend is not initialized I 181.858s run_tests_on_device(01ab756b0330210f) [FATAL:thread_restrictions.cc(57)] LazyInstance/Singleton is not allowed to be used on this thread. Most likely it's because this thread is not joinable, so AtExitManager may have deleted the object on shutdown, leading to a potential shutdown crash. .
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125}
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ==========
Message was sent while issue was closed.
So I just realized this code isn't in the codebase as I expected, looks like it was reverted and never re-landed?
Message was sent while issue was closed.
It got reverted here https://chromium.googlesource.com/chromium/src/+/70c6564dbec07c540c6e7125f9c9... It broke some android tests and I didn't have the time to go back to it.
Message was sent while issue was closed.
On 2016/06/22 17:59:58, Patrick Monette wrote: > It got reverted here > https://chromium.googlesource.com/chromium/src/+/70c6564dbec07c540c6e7125f9c9... > > It broke some android tests and I didn't have the time to go back to it. Got it, seems like the check in the logs should not apply to LazyInstance::Leaky (as AtExitManager doesn't act on it), should probably correct that check and re-land. Thanks, Gab
Message was sent while issue was closed.
On 2016/06/22 20:47:59, gab wrote: > On 2016/06/22 17:59:58, Patrick Monette wrote: > > It got reverted here > > > https://chromium.googlesource.com/chromium/src/+/70c6564dbec07c540c6e7125f9c9... > > > > It broke some android tests and I didn't have the time to go back to it. > > Got it, seems like the check in the logs should not apply to LazyInstance::Leaky > (as AtExitManager doesn't act on it), should probably correct that check and > re-land. Hey Pat, think this is something you could get around to during no meetings weeks? It will block migration of SequencedWorkerPool to TaskScheduler. Thanks!
Message was sent while issue was closed.
Sure. Looking into it! On Thu, Jul 7, 2016 at 10:51 AM <gab@chromium.org> wrote: > On 2016/06/22 20:47:59, gab wrote: > > On 2016/06/22 17:59:58, Patrick Monette wrote: > > > It got reverted here > > > > > > > https://chromium.googlesource.com/chromium/src/+/70c6564dbec07c540c6e7125f9c9... > > > > > > It broke some android tests and I didn't have the time to go back to > it. > > > > Got it, seems like the check in the logs should not apply to > LazyInstance::Leaky > > (as AtExitManager doesn't act on it), should probably correct that check > and > > re-land. > > Hey Pat, think this is something you could get around to during no meetings > weeks? It will block migration of SequencedWorkerPool to TaskScheduler. > > Thanks! > > https://codereview.chromium.org/1904273002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Any luck? Maybe try re-landing and get fresh logs? On Thu, Jul 7, 2016 at 1:06 PM Patrick Langlois Monette <pmonette@google.com> wrote: > Sure. Looking into it! > > On Thu, Jul 7, 2016 at 10:51 AM <gab@chromium.org> wrote: > >> On 2016/06/22 20:47:59, gab wrote: >> > On 2016/06/22 17:59:58, Patrick Monette wrote: >> > > It got reverted here >> > > >> > >> >> https://chromium.googlesource.com/chromium/src/+/70c6564dbec07c540c6e7125f9c9... >> > > >> > > It broke some android tests and I didn't have the time to go back to >> it. >> > >> > Got it, seems like the check in the logs should not apply to >> LazyInstance::Leaky >> > (as AtExitManager doesn't act on it), should probably correct that >> check and >> > re-land. >> >> Hey Pat, think this is something you could get around to during no >> meetings >> weeks? It will block migration of SequencedWorkerPool to TaskScheduler. >> >> Thanks! >> >> https://codereview.chromium.org/1904273002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ==========
Yup relanding this now.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, danakj@chromium.org, robliao@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1904273002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Cr-Commit-Position: refs/heads/master@{#390125} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Commit-Position: refs/heads/master@{#406053}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/2163613002/ by grt@chromium.org. The reason for reverting is: Reverting due to Android failures (https://crbug.com/607317). I hope the logs prove useful (here's one: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...). Looks like the stacks aren't symbolized, so you may have to go the manual route to track this down..
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ==========
On 2016/07/19 08:29:04, grt (UTC plus 2) wrote: > A revert of this CL (patchset #5 id:120001) has been created in > https://codereview.chromium.org/2163613002/ by mailto:grt@chromium.org. > > The reason for reverting is: Reverting due to Android failures > (https://crbug.com/607317). I hope the logs prove useful (here's one: > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg...). > Looks like the stacks aren't symbolized, so you may have to go the manual route > to track this down.. The stack traces for an Android build is available in the stack_tool_with_logcat_dump step. I fixed it and I'm relanding this again.
The CQ bit was checked by pmonette@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, danakj@chromium.org, robliao@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1904273002/#ps140001 (title: "Made a Singleton leaky in test_support_android.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Cr-Original-Commit-Position: refs/heads/master@{#390125} Cr-Commit-Position: refs/heads/master@{#406053} ========== to ========== Enforce singleton not allowed behavior for CONTINUE_ON_SHUTDOWN tasks CONTINUE_ON_SHUTDOWN means the thread running the task will not be joined on shutdown and may not have a valid singleton/lazy_instance at that moment. Also made DhcpcsvcInitSingleton explicitly leaky to avoid the AssertSingletonAllowed() dcheck. BUG=478209 Committed: https://crrev.com/de7e9038663d69356e558764d5a7b215e8b49b5b Committed: https://crrev.com/68e03e7337bc3252f874bcf9e50a5830a54eecb0 Committed: https://crrev.com/7eebe86636dc5f9bbe9c7bec367e99569894d34a Cr-Original-Original-Commit-Position: refs/heads/master@{#390125} Cr-Original-Commit-Position: refs/heads/master@{#406053} Cr-Commit-Position: refs/heads/master@{#406293} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7eebe86636dc5f9bbe9c7bec367e99569894d34a Cr-Commit-Position: refs/heads/master@{#406293} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
