Commit b796d00e authored by Findit's avatar Findit

Revert "AppCache: clean up WeakPtrFactory members in classes with children."

This reverts commit 638ab304.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 601431 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzYzOGFiMzA0MDhjZmNkOTk3MDM5MTYwYzVmZmIxMjZjNWE4MmVmZjMM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20ASan%20LSan%20Tests%20%281%29/51310

Sample Failed Step: content_unittests

Original change's description:
> AppCache: clean up WeakPtrFactory members in classes with children.
> 
> This CL marks AppCache clases with WeakPtrFactory members as final, and
> removes WeakPtrFactory members from classes that have subclasses.
> 
> The superclasses of AppCache classes with WeakPtrFactory members were
> checked for WeakPtrFactory members, resulting in minor cleanups.
> 
> This CL does not tackle AppCacheUrlRequestJob, because net:URLRequestJob
> (which has a WeakPtrFactory) has 35 subclasses. This case will be
> addressed separately.
> 
> TBR=kinuko
> 
> Change-Id: I04bb0757c34fa03620517d04571cba5a0e4a77a5
> Reviewed-on: https://chromium-review.googlesource.com/c/1267337
> Commit-Queue: Victor Costan <pwnall@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601431}

Change-Id: I61dd61aee9e5289784403ebfb085c90809d25cde
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/1293044
Cr-Commit-Position: refs/heads/master@{#601438}
parent d6c330d0
......@@ -26,7 +26,7 @@ class ChromeAppCacheService;
// its child processes. There is a distinct host for each child process.
// Messages are handled on the IO thread. The RenderProcessHostImpl creates
// an instance and delegates calls to it.
class AppCacheDispatcherHost final : public mojom::AppCacheBackend {
class AppCacheDispatcherHost : public mojom::AppCacheBackend {
public:
AppCacheDispatcherHost(ChromeAppCacheService* appcache_service,
int process_id);
......
......@@ -49,7 +49,7 @@ class CONTENT_EXPORT AppCacheGroup
public:
// Called just after an appcache update has completed.
virtual void OnUpdateComplete(AppCacheGroup* group) = 0;
virtual ~UpdateObserver() = default;
virtual ~UpdateObserver() {}
};
enum UpdateAppCacheStatus {
......
......@@ -76,7 +76,7 @@ class CONTENT_EXPORT AppCacheHost
// Called just prior to the instance being deleted.
virtual void OnDestructionImminent(AppCacheHost* host) = 0;
virtual ~Observer() = default;
virtual ~Observer() {}
};
AppCacheHost(int host_id, AppCacheFrontend* frontend,
......
......@@ -141,19 +141,18 @@ std::unique_ptr<base::ListValue> GetListValueForAppCacheResourceInfoVector(
AppCacheInternalsUI::Proxy::Proxy(
base::WeakPtr<AppCacheInternalsUI> appcache_internals_ui,
const base::FilePath& partition_path)
: appcache_internals_ui_(std::move(appcache_internals_ui)),
: appcache_internals_ui_(appcache_internals_ui),
partition_path_(partition_path) {}
void AppCacheInternalsUI::Proxy::Initialize(
scoped_refptr<ChromeAppCacheService> chrome_appcache_service) {
const scoped_refptr<ChromeAppCacheService>& chrome_appcache_service) {
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
base::PostTaskWithTraits(
FROM_HERE, {BrowserThread::IO},
base::BindOnce(&Proxy::Initialize, this,
std::move(chrome_appcache_service)));
base::BindOnce(&Proxy::Initialize, this, chrome_appcache_service));
return;
}
appcache_service_ = chrome_appcache_service->GetWeakPtr();
appcache_service_ = chrome_appcache_service->AsWeakPtr();
shutdown_called_ = false;
preparing_response_ = false;
}
......
......@@ -32,7 +32,7 @@ namespace content {
// This implementation is based on the WebUI API and consists of a controller on
// The UI thread which communicates (through a Proxy) with the AppCacheService
// and AppCache storage which live on the IO thread.
class AppCacheInternalsUI final : public WebUIController {
class AppCacheInternalsUI : public WebUIController {
public:
explicit AppCacheInternalsUI(WebUI* web_ui);
~AppCacheInternalsUI() override;
......@@ -76,7 +76,7 @@ class AppCacheInternalsUI final : public WebUIController {
scoped_refptr<net::IOBuffer> response_data,
int net_result_code);
void Initialize(
scoped_refptr<ChromeAppCacheService> chrome_appcache_service);
const scoped_refptr<ChromeAppCacheService>& chrome_appcache_service);
void Shutdown();
base::WeakPtr<AppCacheInternalsUI> appcache_internals_ui_;
......
......@@ -44,7 +44,7 @@ class AppCacheHost;
// given the opportunity to hijack the request along the way. Callers
// should use AppCacheHost::CreateRequestHandler to manufacture instances
// that can retrieve resources for a particular host.
class CONTENT_EXPORT AppCacheRequestHandler final
class CONTENT_EXPORT AppCacheRequestHandler
: public base::SupportsUserData::Data,
public AppCacheHost::Observer,
public AppCacheServiceImpl::Observer,
......
......@@ -392,7 +392,8 @@ AppCacheServiceImpl::AppCacheServiceImpl(
quota_client_(nullptr),
quota_manager_proxy_(quota_manager_proxy),
request_context_(nullptr),
force_keep_session_state_(false) {
force_keep_session_state_(false),
weak_factory_(this) {
if (quota_manager_proxy_.get()) {
quota_client_ = new AppCacheQuotaClient(this);
quota_manager_proxy_->RegisterClient(quota_client_);
......
......@@ -79,7 +79,7 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
// ref provided.
virtual void OnServiceReinitialized(
AppCacheStorageReference* old_storage_ref) {}
virtual ~Observer() = default;
virtual ~Observer() {}
};
// If not using quota management, the proxy may be NULL.
......@@ -159,7 +159,9 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
AppCacheStorage* storage() const { return storage_.get(); }
virtual base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() = 0;
base::WeakPtr<AppCacheServiceImpl> AsWeakPtr() {
return weak_factory_.GetWeakPtr();
}
// Disables the exit-time deletion of session-only data.
void set_force_keep_session_state() { force_keep_session_state_ = true; }
......@@ -216,6 +218,8 @@ class CONTENT_EXPORT AppCacheServiceImpl : public AppCacheService {
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
private:
base::WeakPtrFactory<AppCacheServiceImpl> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AppCacheServiceImpl);
};
......
......@@ -89,22 +89,6 @@ class MockResponseReader : public AppCacheResponseReader {
int data_size_;
};
class TestAppCacheServiceImpl : public AppCacheServiceImpl {
public:
explicit TestAppCacheServiceImpl(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy), weak_factory_(this) {}
~TestAppCacheServiceImpl() override = default;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override {
return weak_factory_.GetWeakPtr();
}
private:
base::WeakPtrFactory<TestAppCacheServiceImpl> weak_factory_;
};
} // namespace
......@@ -114,7 +98,7 @@ class AppCacheServiceImplTest : public testing::Test {
: kOriginURL("http://hello/"),
kOrigin(url::Origin::Create(kOriginURL)),
kManifestUrl(kOriginURL.Resolve("manifest")),
service_(std::make_unique<TestAppCacheServiceImpl>(nullptr)),
service_(new AppCacheServiceImpl(nullptr)),
delete_result_(net::OK),
delete_completion_count_(0) {
// Setup to use mock storage.
......@@ -355,8 +339,8 @@ TEST_F(AppCacheServiceImplTest, ScheduleReinitialize) {
const base::TimeDelta kOneHour(base::TimeDelta::FromHours(1));
// Do things get initialized as expected?
std::unique_ptr<AppCacheServiceImpl> service =
std::make_unique<TestAppCacheServiceImpl>(nullptr);
std::unique_ptr<AppCacheServiceImpl> service(
new AppCacheServiceImpl(nullptr));
EXPECT_TRUE(service->last_reinit_time_.is_null());
EXPECT_FALSE(service->reinit_timer_.IsRunning());
EXPECT_EQ(kNoDelay, service->next_reinit_delay_);
......
......@@ -24,7 +24,8 @@ AppCacheStorage::AppCacheStorage(AppCacheServiceImpl* service)
: last_cache_id_(kUnitializedId),
last_group_id_(kUnitializedId),
last_response_id_(kUnitializedId),
service_(service) {}
service_(service),
weak_factory_(this) {}
AppCacheStorage::~AppCacheStorage() {
DCHECK(delegate_references_.empty());
......@@ -97,6 +98,10 @@ void AppCacheStorage::LoadResponseInfo(const GURL& manifest_url,
info_load->StartIfNeeded();
}
base::WeakPtr<AppCacheStorage> AppCacheStorage::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void AppCacheStorage::UpdateUsageMapAndNotify(const url::Origin& origin,
int64_t new_usage) {
DCHECK_GE(new_usage, 0);
......
......@@ -87,7 +87,7 @@ class CONTENT_EXPORT AppCacheStorage {
const GURL& mainfest_url) {}
protected:
virtual ~Delegate() = default;
virtual ~Delegate() {}
};
explicit AppCacheStorage(AppCacheServiceImpl* service);
......@@ -203,9 +203,6 @@ class CONTENT_EXPORT AppCacheStorage {
// Returns true if the AppCacheStorage instance is initialized.
virtual bool IsInitialized() = 0;
// Returns a weak pointer reference to the AppCacheStorage instance.
virtual base::WeakPtr<AppCacheStorage> GetWeakPtr() = 0;
// Generates unique storage ids for different object types.
int64_t NewCacheId() { return ++last_cache_id_; }
int64_t NewGroupId() { return ++last_group_id_; }
......@@ -219,6 +216,9 @@ class CONTENT_EXPORT AppCacheStorage {
// Simple ptr back to the service object that owns us.
AppCacheServiceImpl* service() { return service_; }
// Returns a weak pointer reference to the AppCacheStorage instance.
base::WeakPtr<AppCacheStorage> GetWeakPtr();
protected:
friend class content::AppCacheQuotaClientTest;
friend class content::AppCacheResponseTest;
......@@ -340,6 +340,10 @@ class CONTENT_EXPORT AppCacheStorage {
content::appcache_storage_unittest::AppCacheStorageTest,
UsageMap);
// The WeakPtrFactory below must occur last in the class definition so they
// get destroyed last.
base::WeakPtrFactory<AppCacheStorage> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(AppCacheStorage);
};
......
......@@ -1740,10 +1740,6 @@ bool AppCacheStorageImpl::IsInitialized() {
return IsInitTaskComplete();
}
base::WeakPtr<AppCacheStorage> AppCacheStorageImpl::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void AppCacheStorageImpl::DelayedStartDeletingUnusedResponses() {
// Only if we haven't already begun.
if (!did_start_deleting_responses_) {
......
......@@ -33,7 +33,7 @@ namespace content {
class AppCacheStorageImplTest;
class ChromeAppCacheServiceTest;
class CONTENT_EXPORT AppCacheStorageImpl final : public AppCacheStorage {
class AppCacheStorageImpl : public AppCacheStorage {
public:
explicit AppCacheStorageImpl(AppCacheServiceImpl* service);
~AppCacheStorageImpl() override;
......@@ -76,7 +76,6 @@ class CONTENT_EXPORT AppCacheStorageImpl final : public AppCacheStorage {
void DeleteResponses(const GURL& manifest_url,
const std::vector<int64_t>& response_ids) override;
bool IsInitialized() override;
base::WeakPtr<AppCacheStorage> GetWeakPtr() override;
private:
// The AppCacheStorageImpl class methods and datamembers may only be
......@@ -155,7 +154,7 @@ class CONTENT_EXPORT AppCacheStorageImpl final : public AppCacheStorage {
const GURL& manifest_url);
// Don't call this when |is_disabled_| is true.
AppCacheDiskCache* disk_cache();
CONTENT_EXPORT AppCacheDiskCache* disk_cache();
// The directory in which we place files in the file system.
base::FilePath cache_directory_;
......
......@@ -158,22 +158,6 @@ class IOThread : public base::Thread {
std::unique_ptr<net::URLRequestContext> request_context_;
};
class TestAppCacheServiceImpl : public AppCacheServiceImpl {
public:
explicit TestAppCacheServiceImpl(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy), weak_factory_(this) {}
~TestAppCacheServiceImpl() override = default;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override {
return weak_factory_.GetWeakPtr();
}
private:
base::WeakPtrFactory<TestAppCacheServiceImpl> weak_factory_;
};
std::unique_ptr<base::test::ScopedTaskEnvironment> scoped_task_environment;
std::unique_ptr<IOThread> io_thread;
std::unique_ptr<base::Thread> background_thread;
......@@ -409,7 +393,7 @@ class AppCacheStorageImplTest : public testing::Test {
void SetUpTest() {
DCHECK(io_thread->task_runner()->BelongsToCurrentThread());
service_ = std::make_unique<TestAppCacheServiceImpl>(nullptr);
service_.reset(new AppCacheServiceImpl(nullptr));
service_->Initialize(base::FilePath());
mock_quota_manager_proxy_ = new MockQuotaManagerProxy();
service_->quota_manager_proxy_ = mock_quota_manager_proxy_;
......@@ -1726,7 +1710,7 @@ class AppCacheStorageImplTest : public testing::Test {
}
// Recreate the service to point at the db and corruption on disk.
service_ = std::make_unique<TestAppCacheServiceImpl>(nullptr);
service_.reset(new AppCacheServiceImpl(nullptr));
service_->set_request_context(io_thread->request_context());
service_->Initialize(temp_directory_.GetPath());
mock_quota_manager_proxy_ = new MockQuotaManagerProxy();
......
......@@ -34,8 +34,8 @@ namespace {
//
// This class owns and scopes the lifetime of the AppCacheRequestHandler
// for the duration of a subresource load.
class SubresourceLoader final : public network::mojom::URLLoader,
public network::mojom::URLLoaderClient {
class SubresourceLoader : public network::mojom::URLLoader,
public network::mojom::URLLoaderClient {
public:
SubresourceLoader(
network::mojom::URLLoaderRequest url_loader_request,
......@@ -67,7 +67,7 @@ class SubresourceLoader final : public network::mojom::URLLoader,
}
private:
~SubresourceLoader() override = default;
~SubresourceLoader() override {}
void OnConnectionError() { delete this; }
......
......@@ -25,7 +25,7 @@ class AppCacheRequestHandler;
class AppCacheServiceImpl;
// Implements the URLLoaderFactory mojom for AppCache subresource requests.
class CONTENT_EXPORT AppCacheSubresourceURLFactory final
class CONTENT_EXPORT AppCacheSubresourceURLFactory
: public network::mojom::URLLoaderFactory {
public:
~AppCacheSubresourceURLFactory() override;
......
......@@ -42,7 +42,7 @@ class AppCacheUpdateJobTest;
class HostNotifier;
// Application cache Update algorithm and state.
class CONTENT_EXPORT AppCacheUpdateJob final
class CONTENT_EXPORT AppCacheUpdateJob
: public AppCacheStorage::Delegate,
public AppCacheHost::Observer,
public AppCacheServiceImpl::Observer {
......
......@@ -18,7 +18,7 @@ namespace content {
// URLRequest subclass for the UpdateRequestBase class. Provides functionality
// to update the AppCache using functionality provided by the URLRequest class.
class AppCacheUpdateJob::UpdateURLRequest final
class AppCacheUpdateJob::UpdateURLRequest
: public AppCacheUpdateJob::UpdateRequestBase,
public net::URLRequest::Delegate {
public:
......
......@@ -32,10 +32,9 @@ class AppCacheURLLoaderRequest;
// AppCacheJob wrapper for a network::mojom::URLLoader implementation which
// returns responses stored in the AppCache.
class CONTENT_EXPORT AppCacheURLLoaderJob final
: public AppCacheJob,
public AppCacheStorage::Delegate,
public network::mojom::URLLoader {
class CONTENT_EXPORT AppCacheURLLoaderJob : public AppCacheJob,
public AppCacheStorage::Delegate,
public network::mojom::URLLoader {
public:
~AppCacheURLLoaderJob() override;
......
......@@ -15,7 +15,7 @@ namespace content {
// AppCacheRequest wrapper for the ResourceRequest class for users of
// URLLoader.
class CONTENT_EXPORT AppCacheURLLoaderRequest final : public AppCacheRequest {
class CONTENT_EXPORT AppCacheURLLoaderRequest : public AppCacheRequest {
public:
// Factory function to create an instance of the AppCacheResourceRequest
// class.
......
......@@ -29,10 +29,9 @@ class AppCacheURLRequestJobTest;
// A net::URLRequestJob derivative that knows how to return a response stored
// in the appcache.
class CONTENT_EXPORT AppCacheURLRequestJob final
: public AppCacheJob,
public AppCacheStorage::Delegate,
public net::URLRequestJob {
class CONTENT_EXPORT AppCacheURLRequestJob : public AppCacheJob,
public AppCacheStorage::Delegate,
public net::URLRequestJob {
public:
~AppCacheURLRequestJob() override;
......
......@@ -17,9 +17,7 @@ namespace content {
ChromeAppCacheService::ChromeAppCacheService(
storage::QuotaManagerProxy* quota_manager_proxy)
: AppCacheServiceImpl(quota_manager_proxy),
resource_context_(nullptr),
weak_factory_(this) {}
: AppCacheServiceImpl(quota_manager_proxy), resource_context_(nullptr) {}
void ChromeAppCacheService::InitializeOnIOThread(
const base::FilePath& cache_path,
......@@ -69,10 +67,6 @@ void ChromeAppCacheService::UnregisterBackend(
AppCacheServiceImpl::UnregisterBackend(backend_impl);
}
base::WeakPtr<AppCacheServiceImpl> ChromeAppCacheService::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void ChromeAppCacheService::Shutdown() {
bindings_.CloseAllBindings();
}
......
......@@ -8,7 +8,6 @@
#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/sequenced_task_runner_helpers.h"
#include "content/browser/appcache/appcache_backend_impl.h"
#include "content/browser/appcache/appcache_policy.h"
......@@ -42,7 +41,7 @@ struct ChromeAppCacheServiceDeleter;
//
// TODO(dpranke): Fix dependencies on AppCacheServiceImpl so that we don't have
// to worry about clients calling AppCacheServiceImpl methods.
class CONTENT_EXPORT ChromeAppCacheService final
class CONTENT_EXPORT ChromeAppCacheService
: public base::RefCountedThreadSafe<ChromeAppCacheService,
ChromeAppCacheServiceDeleter>,
public AppCacheServiceImpl,
......@@ -77,7 +76,6 @@ class CONTENT_EXPORT ChromeAppCacheService final
// AppCacheServiceImpl override
void UnregisterBackend(AppCacheBackendImpl* backend_impl) override;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override;
protected:
~ChromeAppCacheService() override;
......@@ -97,8 +95,6 @@ class CONTENT_EXPORT ChromeAppCacheService final
// A map from a process_id to a binding_id.
std::map<int, mojo::BindingId> process_bindings_;
base::WeakPtrFactory<ChromeAppCacheService> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(ChromeAppCacheService);
};
......
......@@ -13,27 +13,17 @@
namespace content {
MockAppCacheService::MockAppCacheService()
: AppCacheServiceImpl(nullptr),
mock_delete_appcaches_for_origin_result_(net::OK),
delete_called_count_(0),
weak_factory_(this) {
storage_.reset(new MockAppCacheStorage(this));
static void DeferredCallCallback(net::CompletionOnceCallback callback, int rv) {
std::move(callback).Run(rv);
}
MockAppCacheService::~MockAppCacheService() = default;
void MockAppCacheService::DeleteAppCachesForOrigin(
const url::Origin& origin,
net::CompletionOnceCallback callback) {
++delete_called_count_;
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback),
FROM_HERE, base::BindOnce(&DeferredCallCallback, std::move(callback),
mock_delete_appcaches_for_origin_result_));
}
base::WeakPtr<AppCacheServiceImpl> MockAppCacheService::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
} // namespace content
......@@ -15,16 +15,18 @@ namespace content {
// For use by unit tests.
class MockAppCacheService : public AppCacheServiceImpl {
public:
MockAppCacheService();
~MockAppCacheService() override;
MockAppCacheService()
: AppCacheServiceImpl(NULL),
mock_delete_appcaches_for_origin_result_(net::OK),
delete_called_count_(0) {
storage_.reset(new MockAppCacheStorage(this));
}
// Just returns a canned completion code without actually
// removing groups and caches in our mock storage instance.
void DeleteAppCachesForOrigin(const url::Origin& origin,
net::CompletionOnceCallback callback) override;
base::WeakPtr<AppCacheServiceImpl> GetWeakPtr() override;
void set_quota_manager_proxy(storage::QuotaManagerProxy* proxy) {
quota_manager_proxy_ = proxy;
}
......@@ -38,7 +40,6 @@ class MockAppCacheService : public AppCacheServiceImpl {
private:
int mock_delete_appcaches_for_origin_result_;
int delete_called_count_;
base::WeakPtrFactory<MockAppCacheService> weak_factory_;
};
} // namespace content
......
......@@ -203,10 +203,6 @@ bool MockAppCacheStorage::IsInitialized() {
return false;
}
base::WeakPtr<AppCacheStorage> MockAppCacheStorage::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
void MockAppCacheStorage::ProcessGetAllInfo(
scoped_refptr<DelegateReference> delegate_ref) {
if (delegate_ref->delegate)
......
......@@ -51,7 +51,7 @@ class AppCacheUpdateJobTest;
// Note: This class is also being used to bootstrap our development efforts.
// We can get layout tests up and running, and back fill with real storage
// somewhat in parallel.
class MockAppCacheStorage final : public AppCacheStorage {
class MockAppCacheStorage : public AppCacheStorage {
public:
explicit MockAppCacheStorage(AppCacheServiceImpl* service);
~MockAppCacheStorage() override;
......@@ -87,7 +87,6 @@ class MockAppCacheStorage final : public AppCacheStorage {
void DeleteResponses(const GURL& manifest_url,
const std::vector<int64_t>& response_ids) override;
bool IsInitialized() override;
base::WeakPtr<AppCacheStorage> GetWeakPtr() override;
private:
friend class AppCacheRequestHandlerTest;
......
......@@ -49,7 +49,7 @@ class CONTENT_EXPORT AppCacheService {
net::CompletionOnceCallback callback) = 0;
protected:
virtual ~AppCacheService() = default;
virtual ~AppCacheService() {}
};
} // namespace content
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment