Break pointer cycle between CookieManager and content::URLRequestContextGetterEfl.

These classes hold reference-counted pointers to each other. This was causing memory
leak.
Reviewed by: Daniel Waślicki, Janusz Majnert, Piotr Tworek, arno renevier

Change-Id: I2ea821715aac813c9e6e5f9d8c3122cf46d8ac75
Signed-off-by: Wojciech Wiśniewski <w.wisniewski@samsung.com>

diff --git a/tizen_src/impl/cookie_manager.cc b/tizen_src/impl/cookie_manager.cc
index fbd5169..4af79e8 100644 (file)
--- a/tizen_src/impl/cookie_manager.cc
+++ b/tizen_src/impl/cookie_manager.cc
@@ -80,31 +80,15 @@ CookieManager::CookieManager()
 {
 }
 
-void CookieManager::SetRequestContextGetter(content::URLRequestContextGetterEfl* getter) {
+void CookieManager::SetRequestContextGetter(base::WeakPtr<content::URLRequestContextGetterEfl> getter) {
   request_context_getter_ = getter;
 }
 
-void CookieManager::DeleteCookiesAsync(const std::string& url,
-                                                       const std::string& cookie_name)
-{
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                                    base::Bind(&CookieManager::DeleteCookiesOnIOThread,
-                                                 scoped_refptr<CookieManager>(this),
-                                                 url,
-                                                 cookie_name));
-}
-
-void CookieManager::DeleteCookiesOnIOThread(const std::string& url,
-                                              const std::string& cookie_name) {
+static void DeleteCookiesOnIOThread(scoped_refptr<net::CookieMonster> cookie_monster,
+                                    const std::string& url,
+                                    const std::string& cookie_name) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  if (!request_context_getter_.get())
-    return;
-
-  scoped_refptr<net::CookieMonster> cookie_monster =
-      request_context_getter_->GetURLRequestContext()->
-              cookie_store()->GetCookieMonster();
   if (url.empty()) { // Delete all cookies.
     cookie_monster->DeleteAllAsync(net::CookieMonster::DeleteCallback());
   }
@@ -121,16 +105,47 @@ void CookieManager::DeleteCookiesOnIOThread(const std::string& url,
   }
 }
 
-void CookieManager::SetStoragePath(const std::string& path,
-                                               bool persist_session_cookies,
-                                               bool file_storage_type) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+void CookieManager::DeleteCookiesAsync(const std::string& url,
+                                       const std::string& cookie_name)
+{
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  scoped_refptr<net::CookieMonster> cookie_monster = GetCookieMonster();
+  if (cookie_monster.get()) {
     BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                                    base::Bind(&CookieManager::SetStoragePathOnIOThread,
-                                                 scoped_refptr<CookieManager>(this),
-                                                 path,
-                                                 persist_session_cookies,
-                                                 file_storage_type));
+                            base::Bind(DeleteCookiesOnIOThread,
+                                       cookie_monster,
+                                       url,
+                                       cookie_name));
+  }
+}
+
+static void SetStoragePathOnIOThread(
+    scoped_refptr<content::URLRequestContextGetterEfl> request_context_getter,
+    const std::string& path,
+    bool persist_session_cookies,
+    bool file_storage_type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (request_context_getter.get()) {
+    base::FilePath storage_path(path);
+    request_context_getter->SetCookieStoragePath(storage_path, persist_session_cookies);
+  }
+}
+
+void CookieManager::SetStoragePath(const std::string& path,
+                                   bool persist_session_cookies,
+                                   bool file_storage_type) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ scoped_refptr<content::URLRequestContextGetterEfl> context_getter = GetContextGetter();
+ if (context_getter.get()) {
+   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                           base::Bind(SetStoragePathOnIOThread,
+                                      context_getter,
+                                      path,
+                                      persist_session_cookies,
+                                      file_storage_type));
+ }
 }
 
 void CookieManager::GetAcceptPolicyAsync(AsyncPolicyGetCb callback, void *data) {
@@ -145,24 +160,22 @@ void CookieManager::GetAcceptPolicyAsync(AsyncPolicyGetCb callback, void *data)
 void CookieManager::GetHostNamesWithCookiesAsync(AsyncHostnamesGetCb callback, void *data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   host_callback_queue_.push(new EwkGetHostCallback(callback, data));
-  BrowserThread::PostTask(BrowserThread::IO,
-                          FROM_HERE,
-                          base::Bind(&CookieManager::FetchCookiesOnIOThread,
-                                     scoped_refptr<CookieManager>(this)));
+  scoped_refptr<net::CookieMonster> cookie_monster = GetCookieMonster();
+  if (cookie_monster.get()) {
+    BrowserThread::PostTask(BrowserThread::IO,
+                            FROM_HERE,
+                            base::Bind(&CookieManager::FetchCookiesOnIOThread,
+                                       GetSharedThis(),
+                                       cookie_monster));
+  }
 }
 
-void CookieManager::FetchCookiesOnIOThread() {
+void CookieManager::FetchCookiesOnIOThread(scoped_refptr<net::CookieMonster> cookie_monster) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!request_context_getter_.get()) {
-    OnFetchComplete(net::CookieList());
-    return;
-  }
-  scoped_refptr<net::CookieMonster> cookie_monster =
-      request_context_getter_->GetURLRequestContext()->
-      cookie_store()->GetCookieMonster();
+
   if (cookie_monster.get()) {
-    cookie_monster->GetAllCookiesAsync(
-        base::Bind(&CookieManager::OnFetchComplete, scoped_refptr<CookieManager>(this)));
+    cookie_monster->GetAllCookiesAsync(base::Bind(&CookieManager::OnFetchComplete,
+                                                  GetSharedThis()));
   } else {
     OnFetchComplete(net::CookieList());
   }
@@ -172,8 +185,8 @@ void CookieManager::OnFetchComplete(const net::CookieList& cookies) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                                     base::Bind(&CookieManager::OnFetchComplete,
-                                                   scoped_refptr<CookieManager>(this),
-                                                   cookies));
+                                               GetSharedThis(),
+                                               cookies));
     return;
   }
   if (!host_callback_queue_.empty()) {
@@ -186,15 +199,6 @@ void CookieManager::OnFetchComplete(const net::CookieList& cookies) {
   }
 }
 
-void CookieManager::SetStoragePathOnIOThread(const std::string& path,
-                                             bool persist_session_cookies,
-                                             bool file_storage_type) {  
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::FilePath storage_path(path);
-  if (request_context_getter_.get())
-    request_context_getter_->SetCookieStoragePath(storage_path, persist_session_cookies);
-}
-
 bool CookieManager::GetGlobalAllowAccess() {
   AutoLock lock(lock_);
   if (TW_COOKIE_ACCEPT_POLICY_ALWAYS == cookie_policy_)
@@ -267,27 +271,27 @@ bool CookieManager::AllowSetCookie(const GURL& url,
   return AllowCookies(url, first_party, true);
 }
 
-void CookieManager::GetCookieValueOnIOThread(const GURL& host,
-                                             std::string* result,
-                                             base::WaitableEvent* completion) {
+static void SignalGetCookieValueCompleted(base::WaitableEvent* completion,
+                                          std::string* result,
+                                          const std::string& value) {
+  DCHECK(completion);
+
+  *result = value;
+  completion->Signal();
+}
+
+static void GetCookieValueOnIOThread(scoped_refptr<net::CookieMonster> cookie_monster,
+                                     const GURL& host,
+                                     std::string* result,
+                                     base::WaitableEvent* completion) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   net::CookieOptions options;
   options.set_include_httponly();
 
-  if (!request_context_getter_.get()) {
-    DCHECK(completion);
-    completion->Signal();
-    return;
-  }
-
-  scoped_refptr<net::CookieMonster> cookie_monster =
-      request_context_getter_->GetURLRequestContext()->
-      cookie_store()->GetCookieMonster();
   if (cookie_monster.get()) {
     cookie_monster->GetCookiesWithOptionsAsync(host,
                                                options,
-                                               base::Bind(&CookieManager::GetCookieValueCompleted,
-                                                          scoped_refptr<CookieManager>(this),
+                                               base::Bind(SignalGetCookieValueCompleted,
                                                           completion,
                                                           result));
   } else {
@@ -296,24 +300,20 @@ void CookieManager::GetCookieValueOnIOThread(const GURL& host,
   }
 }
 
-void CookieManager::GetCookieValueCompleted(base::WaitableEvent* completion,
-                                            std::string* result,
-                                            const std::string& value) {
-  *result = value;
-  DCHECK(completion);
-  completion->Signal();
-}
-
 std::string CookieManager::GetCookiesForURL(const std::string& url) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  scoped_refptr<net::CookieMonster> cookie_monster = GetCookieMonster();
+  if (!cookie_monster.get())
+    return std::string();
+
   std::string cookie_value;
   base::WaitableEvent completion(false, false);
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                                  base::Bind(&CookieManager::GetCookieValueOnIOThread,
-                                                 scoped_refptr<CookieManager>(this),
-                                                 GURL(url),
-                                                 &cookie_value,
-                                                 &completion));
+                          base::Bind(GetCookieValueOnIOThread,
+                                     cookie_monster,
+                                     GURL(url),
+                                     &cookie_value,
+                                     &completion));
   //allow wait temporarily
 #if !defined(EWK_BRINGUP)
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
@@ -321,3 +321,23 @@ std::string CookieManager::GetCookiesForURL(const std::string& url) {
   completion.Wait();
   return cookie_value;
 }
+
+scoped_refptr<CookieManager> CookieManager::GetSharedThis() {
+  return scoped_refptr<CookieManager>(this);
+}
+
+scoped_refptr<content::URLRequestContextGetterEfl> CookieManager::GetContextGetter() const {
+  return scoped_refptr<content::URLRequestContextGetterEfl>(request_context_getter_.get());
+}
+
+scoped_refptr<net::CookieMonster> CookieManager::GetCookieMonster() const {
+  scoped_refptr<content::URLRequestContextGetterEfl> request_context_getter = GetContextGetter();
+  if (request_context_getter.get()) {
+    return request_context_getter->
+           GetURLRequestContext()->
+           cookie_store()->
+           GetCookieMonster();
+  } else {
+    return scoped_refptr<net::CookieMonster>();
+  }
+}

diff --git a/tizen_src/impl/cookie_manager.h b/tizen_src/impl/cookie_manager.h
index 6f5a155..ed634d9 100644 (file)
--- a/tizen_src/impl/cookie_manager.h
+++ b/tizen_src/impl/cookie_manager.h
@@ -86,35 +86,27 @@ class CookieManager : public base::RefCountedThreadSafe<CookieManager> {
   bool ShouldBlockThirdPartyCookies();
   //This is synchronous call
   std::string GetCookiesForURL(const std::string& url);
-  void SetRequestContextGetter(content::URLRequestContextGetterEfl* getter);
+
+  void SetRequestContextGetter(base::WeakPtr<content::URLRequestContextGetterEfl> getter);
  private:
   struct EwkGetHostCallback;
 
-  // Deletes cookie having host name. This must be called in IO thread.
-  void DeleteCookiesOnIOThread(const std::string& url,
-                               const std::string& cookie_name) ;
-  void SetStoragePathOnIOThread(const std::string& path,
-                                bool persist_session_cookies,
-                                bool file_storage_type);
   bool AllowCookies(const GURL& url,
                     const GURL& first_party_url,
                     bool setting_cookie);
   // Fetch the cookies. This must be called in the IO thread.
-  void FetchCookiesOnIOThread();
+  void FetchCookiesOnIOThread(scoped_refptr<net::CookieMonster> cookie_monster);
   void OnFetchComplete(const net::CookieList& cookies);
 
-  void GetCookieValueOnIOThread(const GURL& host,
-                                std::string* result,
-                                base::WaitableEvent* completion);
-  void GetCookieValueCompleted(base::WaitableEvent* completion,
-                               std::string* result,
-                               const std::string& value);
+  scoped_refptr<CookieManager> GetSharedThis();
+  scoped_refptr<content::URLRequestContextGetterEfl> GetContextGetter() const;
+  scoped_refptr<net::CookieMonster> GetCookieMonster() const;
   // Indicates whether or not we're currently clearing information:
   // it's true when ClearCookie() is called in the UI thread, and it's reset
   // after we notify the callback in the UI thread.
   // This only mutates on the UI thread.
   bool is_clearing_;
-  scoped_refptr<content::URLRequestContextGetterEfl> request_context_getter_;
+  base::WeakPtr<content::URLRequestContextGetterEfl> request_context_getter_;
   //cookie policy information
   base::Lock lock_;
   tizen_webview::Cookie_Accept_Policy cookie_policy_;

diff --git a/tizen_src/impl/url_request_context_getter_efl.cc b/tizen_src/impl/url_request_context_getter_efl.cc
index f092282..d419644 100644 (file)
--- a/tizen_src/impl/url_request_context_getter_efl.cc
+++ b/tizen_src/impl/url_request_context_getter_efl.cc
@@ -72,11 +72,12 @@ URLRequestContextGetterEfl::URLRequestContextGetterEfl(
       file_task_runner_(file_task_runner),
       net_log_(net_log),
       cookie_manager_(web_context.cookieManager()),
-      request_interceptors_(request_interceptors.Pass()) {
+      request_interceptors_(request_interceptors.Pass()),
+      weak_ptr_factory_(this) {
   // Must first be created on the UI thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  cookie_manager_->SetRequestContextGetter(this);
+  cookie_manager_->SetRequestContextGetter(weak_ptr_factory_.GetWeakPtr());
 
   if (protocol_handlers)
     std::swap(protocol_handlers_, *protocol_handlers);

diff --git a/tizen_src/impl/url_request_context_getter_efl.h b/tizen_src/impl/url_request_context_getter_efl.h
index 42ee2ec..95bbe62 100644 (file)
--- a/tizen_src/impl/url_request_context_getter_efl.h
+++ b/tizen_src/impl/url_request_context_getter_efl.h
@@ -7,6 +7,7 @@
 #define _URL_REQUEST_CONTEXT_GETTER_EFL_H_
 
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/content_browser_client.h"
 #include "net/url_request/url_request_context_getter.h"
 
@@ -78,6 +79,7 @@ class URLRequestContextGetterEfl : public net::URLRequestContextGetter {
   scoped_ptr<net::URLRequestContext> url_request_context_;
   ProtocolHandlerMap protocol_handlers_;
   URLRequestInterceptorScopedVector request_interceptors_;
+  base::WeakPtrFactory<URLRequestContextGetterEfl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextGetterEfl);
 };