Reduce locking in browser and frame implementations to avoid potential deadlocks (issue #824).

git-svn-id: https://chromiumembedded.googlecode.com/svn/trunk@1265 5089003a-bbd8-11dd-ad1f-f1f9622dbc98
2013-05-31 15:34:53 +00:00 · 2013-05-31 15:34:53 +00:00 · 468eee6560
parent b3cd2ae6a8
commit 468eee6560
3 changed files with 169 additions and 118 deletions
--- a/libcef/browser/browser_host_impl.cc
+++ b/libcef/browser/browser_host_impl.cc
@ -2129,7 +2129,9 @@ CefBrowserHostImpl::CefBrowserHostImpl(
  response_manager_.reset(new CefResponseManager);

  placeholder_frame_ =
-      new CefFrameHostImpl(this, CefFrameHostImpl::kInvalidFrameId, true);
+      new CefFrameHostImpl(this, CefFrameHostImpl::kInvalidFrameId, true,
+                           CefString(), CefString(),
+                           CefFrameHostImpl::kInvalidFrameId);

  // Make sure RenderViewCreated is called at least one time.
  RenderViewCreated(web_contents->GetRenderViewHost());
@ -2150,30 +2152,30 @@ CefRefPtr<CefFrame> CefBrowserHostImpl::GetOrCreateFrame(
  if (!frame_name.empty())
    name = frame_name;

-  base::AutoLock lock_scope(state_lock_);
-
-  if (is_main_frame)
-    main_frame_id_ = frame_id;
-
  CefRefPtr<CefFrameHostImpl> frame;
+  bool frame_created = false;

-  // Check if a frame object already exists.
-  FrameMap::const_iterator it = frames_.find(frame_id);
-  if (it != frames_.end())
-    frame = it->second.get();
+  {
+    base::AutoLock lock_scope(state_lock_);

-  if (!frame.get()) {
-    // Create a new frame object.
-    frame = new CefFrameHostImpl(this, frame_id, is_main_frame);
-    frames_.insert(std::make_pair(frame_id, frame));
+    if (is_main_frame)
+      main_frame_id_ = frame_id;
+
+    // Check if a frame object already exists.
+    FrameMap::const_iterator it = frames_.find(frame_id);
+    if (it != frames_.end())
+      frame = it->second.get();
+
+    if (!frame.get()) {
+      frame = new CefFrameHostImpl(this, frame_id, is_main_frame, url, name,
+                                   parent_frame_id);
+      frame_created = true;
+      frames_.insert(std::make_pair(frame_id, frame));
+    }
  }

-  if (!url.empty())
-    frame->SetURL(url);
-  if (!name.empty())
-    frame->SetName(name);
-  if (parent_frame_id != CefFrameHostImpl::kUnspecifiedFrameId)
-    frame->SetParentId(parent_frame_id);
+  if (!frame_created)
+    frame->SetAttributes(url, name, parent_frame_id);

  return frame.get();
 }
@ -2194,42 +2196,54 @@ void CefBrowserHostImpl::DetachFrame(int64 frame_id) {
 }

 void CefBrowserHostImpl::DetachAllFrames() {
-  base::AutoLock lock_scope(state_lock_);
+  FrameMap frames;

-  FrameMap::const_iterator it = frames_.begin();
-  for (; it != frames_.end(); ++it)
+  {
+    base::AutoLock lock_scope(state_lock_);
+
+    frames = frames_;
+    frames_.clear();
+
+    if (main_frame_id_ != CefFrameHostImpl::kInvalidFrameId)
+      main_frame_id_ = CefFrameHostImpl::kInvalidFrameId;
+    if (focused_frame_id_ != CefFrameHostImpl::kInvalidFrameId)
+      focused_frame_id_ = CefFrameHostImpl::kInvalidFrameId;
+  }
+
+  FrameMap::const_iterator it = frames.begin();
+  for (; it != frames.end(); ++it)
    it->second->Detach();
-
-  frames_.clear();
-
-  if (main_frame_id_ != CefFrameHostImpl::kInvalidFrameId)
-    main_frame_id_ = CefFrameHostImpl::kInvalidFrameId;
-  if (focused_frame_id_ != CefFrameHostImpl::kInvalidFrameId)
-    focused_frame_id_ = CefFrameHostImpl::kInvalidFrameId;
 }

 void CefBrowserHostImpl::SetFocusedFrame(int64 frame_id) {
-  base::AutoLock lock_scope(state_lock_);
+  CefRefPtr<CefFrameHostImpl> unfocused_frame;
+  CefRefPtr<CefFrameHostImpl> focused_frame;
+  
+  {
+    base::AutoLock lock_scope(state_lock_);

-  if (focused_frame_id_ != CefFrameHostImpl::kInvalidFrameId) {
-    // Unfocus the previously focused frame.
-    FrameMap::const_iterator it = frames_.find(frame_id);
-    if (it != frames_.end())
-      it->second->SetFocused(false);
-  }
-
-  if (frame_id != CefFrameHostImpl::kInvalidFrameId) {
-    // Focus the newly focused frame.
-    FrameMap::iterator it = frames_.find(frame_id);
-    if (it != frames_.end()) {
-      it->second->SetFocused(true);
-      focused_frame_id_ = frame_id;
-      return;
+    if (focused_frame_id_ != CefFrameHostImpl::kInvalidFrameId) {
+      // Unfocus the previously focused frame.
+      FrameMap::const_iterator it = frames_.find(frame_id);
+      if (it != frames_.end())
+        unfocused_frame = it->second;
    }
+
+    if (frame_id != CefFrameHostImpl::kInvalidFrameId) {
+      // Focus the newly focused frame.
+      FrameMap::iterator it = frames_.find(frame_id);
+      if (it != frames_.end())
+        focused_frame = it->second;
+    }
+
+    focused_frame_id_ =
+        focused_frame ? frame_id : CefFrameHostImpl::kInvalidFrameId;
  }

-  // No valid frame found.
-  focused_frame_id_ = CefFrameHostImpl::kInvalidFrameId;
+  if (unfocused_frame)
+    unfocused_frame->SetFocused(false);
+  if (focused_frame)
+    focused_frame->SetFocused(true);
 }

 void CefBrowserHostImpl::OnAddressChange(CefRefPtr<CefFrame> frame,
--- a/libcef/browser/frame_host_impl.cc
+++ b/libcef/browser/frame_host_impl.cc
@ -49,12 +49,18 @@ class ViewTextHandler : public CefResponseManager::Handler {

 CefFrameHostImpl::CefFrameHostImpl(CefBrowserHostImpl* browser,
                                   int64 frame_id,
-                                   bool is_main_frame)
+                                   bool is_main_frame,
+                                   const CefString& url,
+                                   const CefString& name,
+                                   int64 parent_frame_id)
    : frame_id_(frame_id),
      is_main_frame_(is_main_frame),
      browser_(browser),
      is_focused_(false),
-      parent_frame_id_(kInvalidFrameId) {
+      url_(url),
+      name_(name),
+      parent_frame_id_(parent_frame_id == kUnspecifiedFrameId ?
+                       kInvalidFrameId : parent_frame_id) {
 }

 CefFrameHostImpl::~CefFrameHostImpl() {
@ -66,88 +72,86 @@ bool CefFrameHostImpl::IsValid() {
 }

 void CefFrameHostImpl::Undo() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Undo", NULL);
+  SendCommand("Undo", NULL);
 }

 void CefFrameHostImpl::Redo() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Redo", NULL);
+  SendCommand("Redo", NULL);
 }

 void CefFrameHostImpl::Cut() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Cut", NULL);
+  SendCommand("Cut", NULL);
 }

 void CefFrameHostImpl::Copy() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Copy", NULL);
+  SendCommand("Copy", NULL);
 }

 void CefFrameHostImpl::Paste() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Paste", NULL);
+  SendCommand("Paste", NULL);
 }

 void CefFrameHostImpl::Delete() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "Delete", NULL);
+  SendCommand("Delete", NULL);
 }

 void CefFrameHostImpl::SelectAll() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "SelectAll", NULL);
+  SendCommand("SelectAll", NULL);
 }

 void CefFrameHostImpl::ViewSource() {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId)
-    browser_->SendCommand(frame_id_, "GetSource", new ViewTextHandler(this));
+  SendCommand("GetSource", new ViewTextHandler(this));
 }

 void CefFrameHostImpl::GetSource(CefRefPtr<CefStringVisitor> visitor) {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId) {
-    browser_->SendCommand(frame_id_, "GetSource",
-                          new StringVisitHandler(visitor));
-  }
+  SendCommand("GetSource", new StringVisitHandler(visitor));
 }

 void CefFrameHostImpl::GetText(CefRefPtr<CefStringVisitor> visitor) {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_ && frame_id_ != kInvalidFrameId) {
-    browser_->SendCommand(frame_id_, "GetText",
-                          new StringVisitHandler(visitor));
-  }
+  SendCommand("GetText", new StringVisitHandler(visitor));
 }

 void CefFrameHostImpl::LoadRequest(CefRefPtr<CefRequest> request) {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_)
-    browser_->LoadRequest((is_main_frame_ ? kMainFrameId : frame_id_), request);
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 frame_id;
+
+  {
+    base::AutoLock lock_scope(state_lock_);
+    browser = browser_;
+    frame_id = (is_main_frame_ ? kMainFrameId : frame_id_);
+  }
+
+  if (browser)
+    browser->LoadRequest(frame_id, request);
 }

 void CefFrameHostImpl::LoadURL(const CefString& url) {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_)
-    browser_->LoadURL((is_main_frame_ ? kMainFrameId : frame_id_), url);
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 frame_id;
+
+  {
+    base::AutoLock lock_scope(state_lock_);
+    browser = browser_;
+    frame_id = (is_main_frame_ ? kMainFrameId : frame_id_);
+  }
+
+  if (browser)
+    browser->LoadURL(frame_id, url);
 }

 void CefFrameHostImpl::LoadString(const CefString& string,
                                  const CefString& url) {
-  base::AutoLock lock_scope(state_lock_);
-  if (browser_) {
-    browser_->LoadString((is_main_frame_ ? kMainFrameId : frame_id_), string,
-                         url);
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 frame_id;
+
+  {
+    base::AutoLock lock_scope(state_lock_);
+    browser = browser_;
+    frame_id = (is_main_frame_ ? kMainFrameId : frame_id_);
  }
+
+  if (browser)
+    browser->LoadString(frame_id, string, url);
 }

 void CefFrameHostImpl::ExecuteJavaScript(const CefString& jsCode,
@ -175,13 +179,19 @@ int64 CefFrameHostImpl::GetIdentifier() {
 }

 CefRefPtr<CefFrame> CefFrameHostImpl::GetParent() {
-  base::AutoLock lock_scope(state_lock_);
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 parent_frame_id;

-  if (is_main_frame_ || parent_frame_id_ == kInvalidFrameId)
-    return NULL;
+  {
+    base::AutoLock lock_scope(state_lock_);
+    if (is_main_frame_ || parent_frame_id_ == kInvalidFrameId)
+      return NULL;
+    browser = browser_;
+    parent_frame_id = parent_frame_id_;
+  }

-  if (browser_)
-    return browser_->GetFrame(parent_frame_id_);
+  if (browser)
+    return browser->GetFrame(parent_frame_id);

  return NULL;
 }
@ -201,19 +211,16 @@ void CefFrameHostImpl::SetFocused(bool focused) {
  is_focused_ = focused;
 }

-void CefFrameHostImpl::SetURL(const CefString& url) {
+void CefFrameHostImpl::SetAttributes(const CefString& url,
+                                     const CefString& name,
+                                     int64 parent_frame_id) {
  base::AutoLock lock_scope(state_lock_);
-  url_ = url;
-}
-
-void CefFrameHostImpl::SetName(const CefString& name) {
-  base::AutoLock lock_scope(state_lock_);
-  name_ = name;
-}
-
-void CefFrameHostImpl::SetParentId(int64 frame_id) {
-  base::AutoLock lock_scope(state_lock_);
-  parent_frame_id_ = frame_id;
+  if (!url.empty() && url != url_)
+    url_ = url;
+  if (!name.empty() && name != name_)
+    name_ = name;
+  if (parent_frame_id != kUnspecifiedFrameId)
+    parent_frame_id_ = parent_frame_id;
 }

 CefRefPtr<CefV8Context> CefFrameHostImpl::GetV8Context() {
@ -234,14 +241,37 @@ void CefFrameHostImpl::SendJavaScript(
  if (startLine < 0)
    startLine = 0;

-  base::AutoLock lock_scope(state_lock_);
-  if (browser_) {
-    browser_->SendCode((is_main_frame_ ? kMainFrameId : frame_id_), true,
-                       jsCode, scriptUrl, startLine, NULL);
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 frame_id;
+
+  {
+    base::AutoLock lock_scope(state_lock_);
+    browser = browser_;
+    frame_id = (is_main_frame_ ? kMainFrameId : frame_id_);
  }
+
+  if (browser)
+    browser->SendCode(frame_id, true, jsCode, scriptUrl, startLine, NULL);
 }

 void CefFrameHostImpl::Detach() {
  base::AutoLock lock_scope(state_lock_);
  browser_ = NULL;
 }
+
+void CefFrameHostImpl::SendCommand(
+    const std::string& command,
+    CefRefPtr<CefResponseManager::Handler> responseHandler) {
+  CefRefPtr<CefBrowserHostImpl> browser;
+  int64 frame_id;
+
+  {
+    base::AutoLock lock_scope(state_lock_);
+    browser = browser_;
+    // Commands can only be sent to known frame ids.
+    frame_id = frame_id_;
+  }
+
+  if (browser && frame_id != kInvalidFrameId)
+    browser->SendCommand(frame_id, command, responseHandler);
+}
--- a/libcef/browser/frame_host_impl.h
+++ b/libcef/browser/frame_host_impl.h
@ -8,6 +8,7 @@

 #include <string>
 #include "include/cef_frame.h"
+#include "libcef/common/response_manager.h"
 #include "base/synchronization/lock.h"

 class CefBrowserHostImpl;
@ -19,7 +20,10 @@ class CefFrameHostImpl : public CefFrame {
 public:
  CefFrameHostImpl(CefBrowserHostImpl* browser,
               int64 frame_id,
-               bool is_main_frame);
+               bool is_main_frame,
+               const CefString& url,
+               const CefString& name,
+               int64 parent_frame_id);
  virtual ~CefFrameHostImpl();

  // CefFrame methods
@ -52,9 +56,9 @@ class CefFrameHostImpl : public CefFrame {
  virtual void VisitDOM(CefRefPtr<CefDOMVisitor> visitor) OVERRIDE;

  void SetFocused(bool focused);
-  void SetURL(const CefString& url);
-  void SetName(const CefString& name);
-  void SetParentId(int64 frame_id);
+  void SetAttributes(const CefString& url,
+                     const CefString& name,
+                     int64 parent_frame_id);

  // Avoids unnecessary string type conversions.
  void SendJavaScript(const std::string& jsCode,
@ -71,6 +75,9 @@ class CefFrameHostImpl : public CefFrame {
  static const int64 kInvalidFrameId = -4;

 protected:
+  void SendCommand(const std::string& command,
+                   CefRefPtr<CefResponseManager::Handler> responseHandler);
+
  int64 frame_id_;
  bool is_main_frame_;