New issue
Advanced search Search tips

Issue 689055 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
droger@chromium.org
bsazonov@chromium.org
msarda@chromium.org
tangltom@chromium.org
jlebel@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Task



Sign in to add a comment

TestingProfile::CreateHistoryService/DestroyHistoryService are not safe

Project Member Reported by treib@chromium.org, Feb 6 2017 
These methods create/destroy a HistoryService without going through BrowserContextDependencyManager, i.e. they bypass the proper order of service construction/destruction. That means that any previously-created service that depends on HistoryService will be left with a dangling pointer.

It's somewhat miraculous that this works at all; it's certainly very fragile. It only works due to hacks that remove certain dependency edges in tests (e.g. TopSitesFactory::ServiceIsNULLWhileTesting), and adding new edges can break totally unrelated tests (see e.g. https://codereview.chromium.org/2660333007/#ps1).

Somewhat related: bug 120220
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 7 2017 

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

commit d8eebb4748e2c2bc2efe5bbcfd6a0d8fa0d0ddd4
Author: treib <treib@chromium.org>
Date: Tue Feb 07 09:00:52 2017

Remove some unnecessary calls to TestingProfile::DestroyHistoryService

Explicitly destroying the HistoryService may leave dependent services
with dangling pointers, and is generally not required (it'll get
destroyed during profile shutdown anyway).

This also adds a warning to TestingProfile::CreateHistoryService and
DestroyHistoryService, since they're generally not safe to use.

BUG=689055

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

[modify] https://crrev.com/d8eebb4748e2c2bc2efe5bbcfd6a0d8fa0d0ddd4/chrome/browser/safe_browsing/browser_feature_extractor_unittest.cc
[modify] https://crrev.com/d8eebb4748e2c2bc2efe5bbcfd6a0d8fa0d0ddd4/chrome/browser/safe_browsing/threat_details_unittest.cc
[modify] https://crrev.com/d8eebb4748e2c2bc2efe5bbcfd6a0d8fa0d0ddd4/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
[modify] https://crrev.com/d8eebb4748e2c2bc2efe5bbcfd6a0d8fa0d0ddd4/chrome/test/base/testing_profile.h
Project Member

Comment 2 by sheriffbot@chromium.org, Feb 6 2018

Cc: droger@chromium.org bsazonov@chromium.org ew...@chromium.org jlebel@chromium.org tangltom@chromium.org sabineb@chromium.org msarda@chromium.org
--Chrome Identity automated triaging--

This bug is Available and has gone one year without any activity. If another month passes without any activity, this bug will be closed out. Please provide an update with the latest status for this bug. Please see https://goo.gl/78kbny for more details. Please remove the Services>SignIn or UI>Browser>Profiles components if this bug isn't related to Chrome Identity.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 3 by sheriffbot@chromium.org, Jun 11 2018

Status: Archived (was: Available)
--Chrome Identity automated triaging--

This available, signin or profiles-related bug has gone at least 30 days since the last automated post without any further update. This bug will be closed out due to inactivity. Please re-open the bug and provide an update if it is still a valid or reproducible bug. Please see https://goo.gl/78kbny for more details. Please remove the Services>SignIn or UI>Browser>Profiles components if this bug isn't related to Chrome Identity.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by treib@chromium.org, Jun 11 2018

Cc: -ew...@chromium.org -sabineb@chromium.org
Labels: -Type-Bug Type-Task
Status: Available (was: Archived)
Reopening since this is still valid. Also removing PMs from CC :)
►

Sign in to add a comment