Clean up the proxy code for handling frame navigation.

When a frame navigates to a new page, the old context handle is disposed, and the global proxy object is detached from
the old context. A new context is created using the global proxy object. However, the security token is not created
because, at this point, the security origin is not updated to new URL.
When the new document is ready, V8Proxy updates the security token using the security origin of the document.

Our code relies on the fact that ScriptController::clearWindowShell be called only when the frame loads a page, but not at closing time.


Review URL: http://codereview.chromium.org/10687

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5396 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 62 insertions, 109 deletions

diff --git a/webkit/port/bindings/v8/ScriptController.cpp b/webkit/port/bindings/v8/ScriptController.cpp
index 60af17f..6521a4d 100644
--- a/webkit/port/bindings/v8/ScriptController.cpp
+++ b/webkit/port/bindings/v8/ScriptController.cpp
@@ -323,11 +323,6 @@ void ScriptController::finishedWithEvent(Event* evt)
     m_proxy->finishedWithEvent(evt);
 }
 
-void ScriptController::clearDocumentWrapper()
-{
-    m_proxy->clearDocumentWrapper();
-}
-
 // Create a V8 object with an interceptor of NPObjectPropertyGetter
 void ScriptController::BindToWindowObject(Frame* frame, const String& key, NPObject* object)
 {
@@ -512,6 +507,9 @@ NPObject* ScriptController::createScriptObjectForPluginElement(HTMLPlugInElement
 
 void ScriptController::clearWindowShell()
 {
+    // V8 binding expects ScriptController::clearWindowShell only be called
+    // when a frame is loading a new page. V8Proxy::clearForNavigation
+    // creates a new context for the new page.
     m_proxy->clearForNavigation();
 }
 
@@ -522,9 +520,7 @@ void ScriptController::attachDebugger(void*)
 
 void ScriptController::updateDocument()
 {
-    // TODO(eseidel): Should update document property on current window object
-    // and all previous window objects which may still be alive.
-    notImplemented();
+    m_proxy->updateDocument();
 }
 
 
diff --git a/webkit/port/bindings/v8/ScriptController.h b/webkit/port/bindings/v8/ScriptController.h
index 76ba5a1..9c29a56 100644
--- a/webkit/port/bindings/v8/ScriptController.h
+++ b/webkit/port/bindings/v8/ScriptController.h
@@ -170,7 +170,6 @@ public:
     JSInstanceHandle createScriptInstanceForWidget(Widget*);
 
     void clearPluginObjects();
-    void clearDocumentWrapper();
     void disconnectFrame();
 
     // Check if the javascript engine has been initialized.
@@ -232,7 +231,6 @@ public:
 
     void clearWindowShell();
     void updateDocument();
-    void domWindowReady() { m_proxy->domWindowReady(); }
 
     void pauseTimeouts(OwnPtr<PausedTimeouts>&);
     void resumeTimeouts(OwnPtr<PausedTimeouts>&);
diff --git a/webkit/port/bindings/v8/v8_proxy.cpp b/webkit/port/bindings/v8/v8_proxy.cpp
index 6fa8e8d..1176692 100644
--- a/webkit/port/bindings/v8/v8_proxy.cpp
+++ b/webkit/port/bindings/v8/v8_proxy.cpp
@@ -1019,7 +1019,7 @@ bool V8Proxy::HandleOutOfMemory()
     Frame* frame = V8Proxy::retrieveFrame(context);
 
     V8Proxy* proxy = V8Proxy::retrieve(frame);
-    // Clean m_context, m_document, and event handlers.
+    // Clean m_context, and event handlers.
     proxy->clearForClose();
     // Destroy the global object.
     proxy->DestroyGlobal();
@@ -1556,20 +1556,6 @@ bool V8Proxy::isEnabled()
     return false;  // Other protocols fall through to here
 }
 
-void V8Proxy::clearDocumentWrapper()
-{
-    v8::Local<v8::Context> context = GetContext();
-    if (context.IsEmpty())
-        return;  // not initialize yet
-
-    if (!m_document.IsEmpty()) {
-#ifndef NDEBUG
-        UnregisterGlobalHandle(this, m_document);
-#endif
-        m_document.Dispose();
-        m_document.Clear();
-    }
-}
 
 // static
 void V8Proxy::DomainChanged(Frame* frame)
@@ -1585,10 +1571,6 @@ void V8Proxy::clearForClose()
     if (m_context.IsEmpty())
         return;
 
-    {   v8::HandleScope handle_scope;
-        clearDocumentWrapper();
-    }
-
     m_context.Dispose();
     m_context.Clear();
 }
@@ -1600,7 +1582,8 @@ void V8Proxy::clearForNavigation()
         return;
 
     {   v8::HandleScope handle;
-        clearDocumentWrapper();
+
+        v8::Context::Scope context_scope(m_context);
 
         // Turn on access check on the old DOMWindow wrapper.
         v8::Handle<v8::Object> wrapper =
@@ -1618,21 +1601,63 @@ void V8Proxy::clearForNavigation()
 
         // Separate the context from its global object.
         m_context->DetachGlobal();
+
+        m_context.Dispose();
+        m_context.Clear();
+
+        // Reinitialize the context so the global object points to
+        // the new DOM window.
+        initContextIfNeeded();
     }
+}
 
-    // Corresponds to the context creation in initContextIfNeeded().
-    m_context.Dispose();
-    m_context.Clear();
+
+void V8Proxy::SetSecurityToken() {
+    Document* document = m_frame->document();
+    // Setup security origin and security token
+    if (!document) {
+        m_context->UseDefaultSecurityToken();
+        return;
+    }
+
+    // Ask the document's SecurityOrigin to generate a security token.
+    // If two tokens are equal, then the SecurityOrigins canAccess each other.
+    // If two tokens are not equal, then we have to call canAccess.
+    // Note: we can't use the HTTPOrigin if it was set from the DOM.
+    SecurityOrigin* origin = document->securityOrigin();
+    String token;
+    if (!origin->domainWasSetInDOM())
+        token = document->securityOrigin()->toString();
+
+    // An empty token means we always have to call canAccess.  In this case, we
+    // use the global object as the security token to avoid calling canAccess
+    // when a script accesses its own objects.
+    if (token.isEmpty()) {
+        m_context->UseDefaultSecurityToken();
+        return;
+    }
+
+    CString utf8_token = token.utf8();
+    // NOTE: V8 does identity comparison in fast path, must use a symbol
+    // as the security token.
+    m_context->SetSecurityToken(
+        v8::String::NewSymbol(utf8_token.data(), utf8_token.length()));
 }
 
 
-void V8Proxy::domWindowReady()
+void V8Proxy::updateDocument()
 {
-    // Reinitialize context so the global object is reused by
-    // the new context.
-    if (!m_global.IsEmpty() && m_context.IsEmpty()) {
+    if (!m_frame->document())
+        return;
+
+    if (m_global.IsEmpty()) {
+        ASSERT(m_context.IsEmpty());
+        return;
+    }
+
+    {
         v8::HandleScope scope;
-        initContextIfNeeded();
+        SetSecurityToken();
     }
 }
 
@@ -1720,39 +1745,6 @@ bool V8Proxy::CheckNodeSecurity(Node* node)
 }
 
 
-static void GenerateSecurityToken(v8::Local<v8::Context> context)
-{
-  Document* document = V8Proxy::retrieveFrame(context)->document();
-  if (!document) {
-    context->UseDefaultSecurityToken();
-    return;
-  }
-
-  // Ask the document's SecurityOrigin to generate a security token.
-  // If two tokens are equal, then the SecurityOrigins canAccess each other.
-  // If two tokens are not equal, then we have to call canAccess.
-  // Note: we can't use the HTTPOrigin if it was set from the DOM.
-  SecurityOrigin* origin = document->securityOrigin();
-  String token;
-  if (!origin->domainWasSetInDOM())
-    token = document->securityOrigin()->toString();
-
-  // An empty token means we always have to call canAccess.  In this case, we
-  // use the global object as the security token to avoid calling canAccess
-  // when a script accesses its own objects.
-  if (token.isEmpty()) {
-    context->UseDefaultSecurityToken();
-    return;
-  }
-
-  CString utf8_token = token.utf8();
-  // NOTE: V8 does identity comparison in fast path, must use a symbol
-  // as the security token.
-  context->SetSecurityToken(
-    v8::String::NewSymbol(utf8_token.data(), utf8_token.length()));
-}
-
-
 // Create a new environment and setup the global object.
 //
 // The global object corresponds to a DOMWindow instance.  However, to
@@ -1872,8 +1864,7 @@ void V8Proxy::initContextIfNeeded()
   v8::Handle<v8::Object> v8_global = context->Global();
   v8_global->Set(v8::String::New("__proto__"), window_peer);
 
-  // Setup security origin and security token
-  GenerateSecurityToken(context);
+  SetSecurityToken();
 
   m_frame->loader()->dispatchWindowObjectAvailable();
 
@@ -2626,11 +2617,6 @@ v8::Handle<v8::Value> V8Proxy::NodeToV8Object(Node* node)
   dom_node_map().set(node, v8::Persistent<v8::Object>::New(result));
 
   if (is_document) {
-    Document* doc = static_cast<Document*>(node);
-    V8Proxy* proxy = V8Proxy::retrieve(doc->frame());
-    if (proxy)
-      proxy->UpdateDocumentHandle(result);
-
     if (type == V8ClassIndex::HTMLDOCUMENT) {
       // Create marker object and insert it in two internal fields.
       // This is used to implement temporary shadowing of
@@ -2647,24 +2633,6 @@ v8::Handle<v8::Value> V8Proxy::NodeToV8Object(Node* node)
 }
 
 
-void V8Proxy::UpdateDocumentHandle(v8::Local<v8::Object> handle)
-{
-  // If the old handle is not empty, release it.
-  if (!m_document.IsEmpty()) {
-#ifndef NDEBUG
-    UnregisterGlobalHandle(this, m_document);
-#endif
-    m_document.Dispose();
-    m_document.Clear();
-  }
-
-  m_document = v8::Persistent<v8::Object>::New(handle);
-#ifndef NDEBUG
-  RegisterGlobalHandle(PROXY, this, m_document);
-#endif
-}
-
-
 // A JS object of type EventTarget can only be five possible types:
 // 1) EventTargetNode; 2) XMLHttpRequest; 3) MessagePort; 4) SVGElementInstance;
 // 5) XMLHttpRequestUpload
diff --git a/webkit/port/bindings/v8/v8_proxy.h b/webkit/port/bindings/v8/v8_proxy.h
index 3f69153..4a1aa9f 100644
--- a/webkit/port/bindings/v8/v8_proxy.h
+++ b/webkit/port/bindings/v8/v8_proxy.h
@@ -164,11 +164,11 @@ class V8Proxy {
   // Clear page-specific data, but keep the global object identify.
   void clearForNavigation();
 
-  // Clear data before closing the frame.
+  // Clear page-specific data before shutting down the proxy object.
   void clearForClose();
 
-  // Notify that a new DOMWindow object is ready.
-  void domWindowReady();
+  // Update document object of the frame.
+  void updateDocument();
 
   // Destroy the global object.
   void DestroyGlobal();
@@ -189,9 +189,6 @@ class V8Proxy {
 
   bool isEnabled();
 
-  // Remove 'document' property from the global object.
-  void clearDocumentWrapper();
-
   // Find/Create/Remove event listener wrappers.
   PassRefPtr<V8EventListener> FindV8EventListener(v8::Local<v8::Value> listener,
                                        bool html);
@@ -421,6 +418,7 @@ class V8Proxy {
  private:
   void initContextIfNeeded();
   void DisconnectEventListeners();
+  void SetSecurityToken();
 
   static bool CanAccessPrivate(DOMWindow* target);
 
@@ -484,10 +482,6 @@ class V8Proxy {
   static const char* GetSVGExceptionName(int exception_code);
 #endif
 
-  // Update m_document field, dispose old one and create a string reference
-  // to the new one.
-  void UpdateDocumentHandle(v8::Local<v8::Object> handle);
-
   // Returns a local handle of the context.
   v8::Local<v8::Context> GetContext() {
     return v8::Local<v8::Context>::New(m_context);
@@ -497,9 +491,6 @@ class V8Proxy {
   v8::Persistent<v8::Context> m_context;
   v8::Persistent<v8::Object> m_global;
 
-  // Special handling of document wrapper;
-  v8::Persistent<v8::Object> m_document;
-
   int m_handlerLineno;
 
   // A list of event listeners created for this frame,