We have a problem in the process on destroying WebContentsImpl because

content/public/browser/web_contents_observer.h

Index: content/public/browser/web_contents_observer.h
diff --git a/content/public/browser/web_contents_observer.h b/content/public/browser/web_contents_observer.h
index 9fae539faf6e3a97c3198e6d95a3e703202a7982..59e02b414479dc70b1a60d7e3c3d37f9c0d9c1d8 100644
--- a/content/public/browser/web_contents_observer.h
+++ b/content/public/browser/web_contents_observer.h
@@ -292,10 +292,18 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener,
   virtual void DidCloneToNewWebContents(WebContents* old_web_contents,
                                         WebContents* new_web_contents) {}
 
-  // Invoked when the WebContents is being destroyed. Gives subclasses a chance
-  // to cleanup. At the time this is invoked |web_contents()| returns NULL.
-  // It is safe to delete 'this' from here.
-  virtual void WebContentsDestroyed(WebContents* web_contents) {}
+  // When WebContents is being destroyed it notifies its observers and this
+  // process is split in two phases (untrusted and trusted).

[jam, 2014/05/02 19:34:11]

this trusted and untrusted terminology is a bit confusing and not standard, so
i'd leave this out.

i would stay with something like 

// Invoked when the WebContents is being destroyed. Gives subclasses a chance
// to cleanup. After this method returns, web_contents() returns NULL.

[zverre, 2014/05/05 16:40:25]

Done.

+  // First WebContents loops through its observers and calls
+  // WebContentsDestroyed to let them run cleanup code. At the time this is
+  // invoked |web_contents()| is guaranteed to be non NULL for all
+  // WebContentsObservers so they can directly or indirectly call each other.
+  // It is safe to delete 'this' from here. When this loop has been finished
+  // trusted phase starts, WebContents loops its WebContentsObservers again
+  // and NULLs its |web_contents_| pointers. This is done by calling
+  // WebContentsDestroyed and this behavior can't be modified since it is
+  // private.
+  virtual void WebContentsDestroyed() {}
 
   // Called when the user agent override for a WebContents has been changed.
   virtual void UserAgentOverrideSet(const std::string& user_agent) {}
@@ -359,9 +367,9 @@ class CONTENT_EXPORT WebContentsObserver : public IPC::Listener,
  private:
   friend class WebContentsImpl;
 
-  // Invoked from WebContentsImpl. Invokes WebContentsDestroyed and NULL out
-  // |web_contents_|.
-  void WebContentsImplDestroyed();
+  // Resets |web_contents_| pointer to NULL as a final step in the process
+  // of destroying the WebContents. See comments near WebContentsDestroyed.
+  void WebContentsDestroyedPrivate() { web_contents_ = NULL; }

[jam, 2014/05/02 19:34:11]

nit: since it's inline and it's a simple method, it should just be
reset_web_contents() { web_contents_ = NULL; }

or really since it's private and the calling class is friends, i don't see a
need to have a method at all

[zverre, 2014/05/05 12:40:43]

Jam, thank you for your comments. I don't really want to write loop manually
because actual list of observers is private and only ObserverListBase::Iterator
can get actual observers' pointers. Also this approach seems not standard and
looks a bit weird in Chromium code. It's much more easy to find observers'
invocation in the code if it is always done with FOR_EACH_OBSERVER. So I'd
better stay with reset_web_contents as a better alternative to
WebContentsDestroyedPrivate.

[jam, 2014/05/05 15:05:03]

ok sure. i don't feel strongly about it (btw for the record, we have some places
that iterate manually over the observer list)

[zverre, 2014/05/05 16:40:25]

Done.

 
   WebContentsImpl* web_contents_;