Fix for bug 430624 (Crash [@ nsDocShellEditorData::DetachFromWindow] with spellcheck attribute). Patch by cpearce, r=peterv, sr=jst.
authorpeterv@propagandism.org
Fri, 02 May 2008 04:36:29 -0700
changeset 14887 166f5bbd468aba90a046ee72384c8acab24c7856
parent 14886 045b42a1da5616bfb3aaf54c8c9af065aebc5cde
child 14888 f0632f4c86c87aa3effab9b122540a8a7e5a09ff
push idunknown
push userunknown
push dateunknown
reviewerspeterv, jst
bugs430624
milestone1.9pre
Fix for bug 430624 (Crash [@ nsDocShellEditorData::DetachFromWindow] with spellcheck attribute). Patch by cpearce, r=peterv, sr=jst.
content/html/document/src/nsHTMLDocument.cpp
docshell/base/nsDocShell.cpp
docshell/base/nsDocShell.h
docshell/base/nsDocShellEditorData.cpp
docshell/base/nsDocShellEditorData.h
docshell/base/nsIDocShell.idl
docshell/shistory/public/nsISHEntry.idl
docshell/test/navigation/Makefile.in
docshell/test/navigation/test_bug430624.html
editor/libeditor/base/crashtests/430624-1.html
editor/libeditor/base/crashtests/crashtests.list
--- a/content/html/document/src/nsHTMLDocument.cpp
+++ b/content/html/document/src/nsHTMLDocument.cpp
@@ -3966,16 +3966,20 @@ nsHTMLDocument::SetEditingState(EditingS
 {
   mEditingState = aState;
   return NS_OK;
 }
 
 nsresult
 nsHTMLDocument::EditingStateChanged()
 {
+  if (mRemovedFromDocShell) {
+    return NS_OK;
+  }
+
   if (mEditingState == eSettingUp || mEditingState == eTearingDown) {
     // XXX We shouldn't recurse.
     return NS_OK;
   }
 
   PRBool designMode = HasFlag(NODE_IS_EDITABLE);
   EditingState newState = designMode ? eDesignMode :
                           (mContentEditableCount > 0 ? eContentEditable : eOff);
--- a/docshell/base/nsDocShell.cpp
+++ b/docshell/base/nsDocShell.cpp
@@ -1006,22 +1006,19 @@ nsDocShell::FirePageHideNotification(PRB
         }
 
         n = kids.Length();
         for (i = 0; i < n; ++i) {
             if (kids[i]) {
                 kids[i]->FirePageHideNotification(aIsUnload);
             }
         }
-    }
-
-    // Now make sure our editor, if any, is detached before we go
-    // any farther.
-    if (mEditorData && aIsUnload) {
-      DetachEditorFromWindow();
+        // Now make sure our editor, if any, is detached before we go
+        // any farther.
+        DetachEditorFromWindow();
     }
 
     return NS_OK;
 }
 
 //
 // Bug 13871: Prevent frameset spoofing
 //
@@ -3372,21 +3369,16 @@ nsDocShell::Reload(PRUint32 aReloadFlags
                           loadType,       // Load type
                           nsnull,         // No SHEntry
                           PR_TRUE,
                           nsnull,         // No nsIDocShell
                           nsnull);        // No nsIRequest
     }
     
 
-    // Need to purge detached editor here, else when we reload a page,
-    // the detached editor state causes SetDesignMode() to fail.
-    if (mOSHE)
-      mOSHE->SetEditorData(nsnull);
-
     return rv;
 }
 
 NS_IMETHODIMP
 nsDocShell::Stop(PRUint32 aStopFlags)
 {
     // Revoke any pending event related to content viewer restoration
     mRestorePresentationEvent.Revoke();
@@ -4880,18 +4872,23 @@ nsDocShell::Embed(nsIContentViewer * aCo
         mLoadType == LOAD_RELOAD_CHARSET_CHANGE)){
         PRBool isWyciwyg = PR_FALSE;
         // Check if the url is wyciwyg
         rv = mCurrentURI->SchemeIs("wyciwyg", &isWyciwyg);      
         if (isWyciwyg && NS_SUCCEEDED(rv))
             SetBaseUrlForWyciwyg(aContentViewer);
     }
     // XXX What if SetupNewViewer fails?
-    if (mLSHE)
+    if (mLSHE) {
+        // Restore the editing state, if it's stored in session history.
+        if (mLSHE->HasDetachedEditor()) {
+            ReattachEditorToWindow(mLSHE);
+        }
         SetHistoryEntry(&mOSHE, mLSHE);
+    }
 
     PRBool updateHistory = PR_TRUE;
 
     // Determine if this type of load should update history
     switch (mLoadType) {
     case LOAD_NORMAL_REPLACE:
     case LOAD_STOP_CONTENT_AND_REPLACE:
     case LOAD_RELOAD_BYPASS_CACHE:
@@ -5327,75 +5324,70 @@ nsDocShell::CanSavePresentation(PRUint32
     // If the document does not want its presentation cached, then don't.
     nsCOMPtr<nsIDocument> doc = do_QueryInterface(pWin->GetExtantDocument());
     if (!doc || !doc->CanSavePresentation(aNewRequest))
         return PR_FALSE;
 
     return PR_TRUE;
 }
 
-PRBool
-nsDocShell::HasDetachedEditor()
-{
-  return (mOSHE && mOSHE->HasDetachedEditor()) ||
-         (mLSHE && mLSHE->HasDetachedEditor());
-}
-
 void
-nsDocShell::ReattachEditorToWindow(nsIDOMWindow *aWindow, nsISHEntry *aSHEntry)
+nsDocShell::ReattachEditorToWindow(nsISHEntry *aSHEntry)
 {
     NS_ASSERTION(!mEditorData,
                  "Why reattach an editor when we already have one?");
-    NS_ASSERTION(aWindow,
-                 "Need a window to reattach to.");
-    NS_ASSERTION(HasDetachedEditor(),
+    NS_ASSERTION(aSHEntry && aSHEntry->HasDetachedEditor(),
                  "Reattaching when there's not a detached editor.");
 
-    if (mEditorData || !aWindow || !aSHEntry)
+    if (mEditorData || !aSHEntry)
       return;
 
     mEditorData = aSHEntry->ForgetEditorData();
     if (mEditorData) {
-        nsresult res = mEditorData->ReattachToWindow(aWindow);
+        nsresult res = mEditorData->ReattachToWindow(this);
         NS_ASSERTION(NS_SUCCEEDED(res), "Failed to reattach editing session");
     }
 }
 
 void
 nsDocShell::DetachEditorFromWindow(nsISHEntry *aSHEntry)
 {
-    if (!aSHEntry || !mEditorData)
+    if (!mEditorData)
         return;
 
-    NS_ASSERTION(!aSHEntry->HasDetachedEditor(),
-                 "Why detach an editor twice?");
+    NS_ASSERTION(!aSHEntry || !aSHEntry->HasDetachedEditor(),
+                 "Detaching editor when it's already detached.");
 
     nsresult res = mEditorData->DetachFromWindow();
     NS_ASSERTION(NS_SUCCEEDED(res), "Failed to detach editor");
 
     if (NS_SUCCEEDED(res)) {
-      // Make aSHEntry hold the owning ref to the editor data.
-      aSHEntry->SetEditorData(mEditorData.forget());
+        // Make aSHEntry hold the owning ref to the editor data.
+        if (aSHEntry)
+            aSHEntry->SetEditorData(mEditorData.forget());
+        else
+            mEditorData = nsnull;
     }
 
 #ifdef DEBUG
     {
         PRBool isEditable;
         GetEditable(&isEditable);
         NS_ASSERTION(!isEditable,
                      "Window is still editable after detaching editor.");
     }
 #endif // DEBUG
 
 }
 
 void
 nsDocShell::DetachEditorFromWindow()
 {
-    DetachEditorFromWindow(mOSHE);
+    if (mOSHE)
+        DetachEditorFromWindow(mOSHE);
 }
 
 nsresult
 nsDocShell::CaptureState()
 {
     if (!mOSHE || mOSHE == mLSHE) {
         // No entry to save into, or we're replacing the existing entry.
         return NS_ERROR_FAILURE;
@@ -5537,16 +5529,20 @@ nsDocShell::FinishRestore()
     PRInt32 n = mChildList.Count();
     for (PRInt32 i = 0; i < n; ++i) {
         nsCOMPtr<nsIDocShell> child = do_QueryInterface(ChildAt(i));
         if (child) {
             child->FinishRestore();
         }
     }
 
+    if (mOSHE && mOSHE->HasDetachedEditor()) {
+      ReattachEditorToWindow(mOSHE);
+    }
+
     if (mContentViewer) {
         nsCOMPtr<nsIDOMDocument> domDoc;
         mContentViewer->GetDOMDocument(getter_AddRefs(domDoc));
 
         nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
         if (doc) {
             // Finally, we remove the request from the loadgroup.  This will
             // cause onStateChange(STATE_STOP) to fire, which will fire the
@@ -6005,21 +6001,16 @@ nsDocShell::RestoreFromHistory()
                    newBounds.y, newBounds.width, newBounds.height);
 #endif
 
             widget->Resize(newBounds.x, newBounds.y, newBounds.width,
                            newBounds.height, PR_FALSE);
         }
     }
 
-    if (HasDetachedEditor()) {
-      nsCOMPtr<nsIDOMWindow> domWin = do_QueryInterface(privWin);
-      ReattachEditorToWindow(domWin, mLSHE);
-    }
-
     // Simulate the completion of the load.
     nsDocShell::FinishRestore();
 
     // Restart plugins, and paint the content.
     if (shell)
         shell->Thaw();
 
     return privWin->FireDelayedDOMEvents();
@@ -7148,20 +7139,16 @@ nsDocShell::InternalLoad(nsIURI * aURI,
         }
 
         if (NS_FAILED(rv)) 
             return rv;
     }
 
     mLoadType = aLoadType;
 
-    // Detach the current editor so that it can be restored from the
-    // bfcache later.
-    DetachEditorFromWindow();
-
     // mLSHE should be assigned to aSHEntry, only after Stop() has
     // been called. But when loading an error page, do not clear the
     // mLSHE for the real page.
     if (mLoadType != LOAD_ERROR_PAGE)
         SetHistoryEntry(&mLSHE, aSHEntry);
 
     mSavingOldViewer = savePresentation;
 
@@ -7215,32 +7202,16 @@ nsDocShell::InternalLoad(nsIURI * aURI,
                    (aFlags & INTERNAL_LOAD_FLAGS_BYPASS_CLASSIFIER) != 0);
     if (req && aRequest)
         NS_ADDREF(*aRequest = req);
 
     if (NS_FAILED(rv)) {
         nsCOMPtr<nsIChannel> chan(do_QueryInterface(req));
         DisplayLoadError(rv, aURI, nsnull, chan);
     }
-    
-    if (aSHEntry) {
-        if (aLoadType & LOAD_CMD_HISTORY) {
-            // We've just loaded a page from session history. Reattach
-            // its editing session if it has one.
-            nsCOMPtr<nsIDOMWindow> domWin;
-            CallGetInterface(this, static_cast<nsIDOMWindow**>(getter_AddRefs(domWin)));
-            ReattachEditorToWindow(domWin, aSHEntry);
-        } else {
-            // This is a non-history load from a session history entry. Purge any
-            // previous editing sessions, so that the the editing session will
-            // be recreated. This can happen when we reload something that's
-            // in the bfcache.
-            aSHEntry->SetEditorData(nsnull);
-        }
-    }
 
     return rv;
 }
 
 nsIPrincipal*
 nsDocShell::GetInheritedPrincipal(PRBool aConsiderCurrentDocument)
 {
     nsCOMPtr<nsIDocument> document;
@@ -8730,19 +8701,22 @@ nsDocShell::ShouldDiscardLayoutState(nsI
 
 //*****************************************************************************
 // nsDocShell: nsIEditorDocShell
 //*****************************************************************************   
 
 NS_IMETHODIMP nsDocShell::GetEditor(nsIEditor * *aEditor)
 {
   NS_ENSURE_ARG_POINTER(aEditor);
-  nsresult rv = EnsureEditorData();
-  if (NS_FAILED(rv)) return rv;
-  
+
+  if (!mEditorData) {
+    *aEditor = nsnull;
+    return NS_OK;
+  }
+
   return mEditorData->GetEditor(aEditor);
 }
 
 NS_IMETHODIMP nsDocShell::SetEditor(nsIEditor * aEditor)
 {
   nsresult rv = EnsureEditorData();
   if (NS_FAILED(rv)) return rv;
 
@@ -9038,19 +9012,22 @@ nsDocShell::EnsureScriptEnvironment()
 
     return NS_OK;
 }
 
 
 NS_IMETHODIMP
 nsDocShell::EnsureEditorData()
 {
-    NS_ASSERTION(!HasDetachedEditor(), "EnsureEditorData() called when detached.\n");
-
-    if (!mEditorData && !mIsBeingDestroyed && !HasDetachedEditor()) {
+    PRBool openDocHasDetachedEditor = mOSHE && mOSHE->HasDetachedEditor();
+    if (!mEditorData && !mIsBeingDestroyed && !openDocHasDetachedEditor) {
+        // We shouldn't recreate the editor data if it already exists, or
+        // we're shutting down, or we already have a detached editor data
+        // stored in the session history. We should only have one editordata
+        // per docshell.
         mEditorData = new nsDocShellEditorData(this);
     }
 
     return mEditorData ? NS_OK : NS_ERROR_NOT_AVAILABLE;
 }
 
 nsresult
 nsDocShell::EnsureTransferableHookData()
--- a/docshell/base/nsDocShell.h
+++ b/docshell/base/nsDocShell.h
@@ -520,17 +520,17 @@ protected:
     // Check whether aURI is about:blank
     static PRBool IsAboutBlank(nsIURI* aURI);
 
     // Call this when a URI load is handed to us (via OnLinkClick or
     // InternalLoad).  This makes sure that we're not inside unload, or that if
     // we are it's still OK to load this URI.
     PRBool IsOKToLoadURI(nsIURI* aURI);
     
-    void ReattachEditorToWindow(nsIDOMWindow *aWindow, nsISHEntry *aSHEntry);
+    void ReattachEditorToWindow(nsISHEntry *aSHEntry);
     void DetachEditorFromWindow(nsISHEntry *aSHEntry);
 
 protected:
     // Override the parent setter from nsDocLoader
     virtual nsresult SetDocLoaderParent(nsDocLoader * aLoader);
 
     // Event type dispatched by RestorePresentation
     class RestorePresentationEvent : public nsRunnable {
@@ -672,20 +672,16 @@ protected:
     nsPIDOMEventTarget *       mChromeEventHandler; //Weak Reference
 
 #ifdef DEBUG
     PRBool mInEnsureScriptEnv;
 #endif
 
     static nsIURIFixup *sURIFixup;
 
-    // Returns true when the currently open document has a detached editor
-    // waiting to be reattached.
-    PRBool HasDetachedEditor();
-
 public:
     class InterfaceRequestorProxy : public nsIInterfaceRequestor {
     public:
         InterfaceRequestorProxy(nsIInterfaceRequestor* p);
         virtual ~InterfaceRequestorProxy();
         NS_DECL_ISUPPORTS
         NS_DECL_NSIINTERFACEREQUESTOR
  
--- a/docshell/base/nsDocShellEditorData.cpp
+++ b/docshell/base/nsDocShellEditorData.cpp
@@ -68,22 +68,22 @@ nsDocShellEditorData::nsDocShellEditorDa
 nsDocShellEditorData::~nsDocShellEditorData()
 {
   TearDownEditor();
 }
 
 void
 nsDocShellEditorData::TearDownEditor()
 {
-  NS_ASSERTION(mIsDetached, "We should be detached before tearing down");
   if (mEditor) {
     mEditor->PreDestroy();
     mEditor = nsnull;
   }
   mEditingSession = nsnull;
+  mIsDetached = PR_FALSE;
 }
 
 
 /*---------------------------------------------------------------------------
 
   MakeEditable
 
 ----------------------------------------------------------------------------*/
@@ -239,22 +239,26 @@ nsDocShellEditorData::DetachFromWindow()
   mMakeEditable = PR_FALSE;
 
   nsCOMPtr<nsIDOMDocument> domDoc;
   domWindow->GetDocument(getter_AddRefs(domDoc));
   nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(domDoc);
   if (htmlDoc)
     mDetachedEditingState = htmlDoc->GetEditingState();
 
+  mDocShell = nsnull;
+
   return NS_OK;
 }
 
 nsresult
-nsDocShellEditorData::ReattachToWindow(nsIDOMWindow *aWindow)
+nsDocShellEditorData::ReattachToWindow(nsIDocShell* aDocShell)
 {
+  mDocShell = aDocShell;
+
   nsCOMPtr<nsIDOMWindow> domWindow = do_GetInterface(mDocShell);
   nsresult rv = mEditingSession->ReattachToWindow(domWindow);
   NS_ENSURE_SUCCESS(rv, rv);
 
   mIsDetached = PR_FALSE;
   mMakeEditable = mDetachedMakeEditable;
 
   nsCOMPtr<nsIDOMDocument> domDoc;
--- a/docshell/base/nsDocShellEditorData.h
+++ b/docshell/base/nsDocShellEditorData.h
@@ -67,17 +67,17 @@ public:
   nsresult MakeEditable(PRBool inWaitForUriLoad);
   PRBool GetEditable();
   nsresult CreateEditor();
   nsresult GetEditingSession(nsIEditingSession **outEditingSession);
   nsresult GetEditor(nsIEditor **outEditor);
   nsresult SetEditor(nsIEditor *inEditor);
   void TearDownEditor();
   nsresult DetachFromWindow();
-  nsresult ReattachToWindow(nsIDOMWindow *aWindow);
+  nsresult ReattachToWindow(nsIDocShell *aDocShell);
 
 protected:
 
   nsresult EnsureEditingSession();
 
   // The doc shell that owns us. Weak ref, since it always outlives us.  
   nsIDocShell* mDocShell;
 
--- a/docshell/base/nsIDocShell.idl
+++ b/docshell/base/nsIDocShell.idl
@@ -63,17 +63,17 @@ interface nsIWebNavigation;
 interface nsISimpleEnumerator;
 interface nsIInputStream;
 interface nsIRequest;
 interface nsISHEntry;
 interface nsILayoutHistoryState;
 interface nsISecureBrowserUI;
 interface nsIDOMStorage;
 
-[scriptable, uuid(4b00222a-8d0a-46d7-a1fe-43bd89d19324)]
+[scriptable, uuid(7d1cf6b9-daa3-476d-8f9f-9eb2a971a95c)]
 interface nsIDocShell : nsISupports
 {
   /**
    * Loads a given URI.  This will give priority to loading the requested URI
    * in the object implementing	this interface.  If it can't be loaded here
    * however, the URL dispatcher will go through its normal process of content
    * loading.
    *
--- a/docshell/shistory/public/nsISHEntry.idl
+++ b/docshell/shistory/public/nsISHEntry.idl
@@ -53,17 +53,17 @@ interface nsISupportsArray;
 %{C++
 struct nsRect;
 class nsDocShellEditorData;
 %}
 [ref] native nsRect(nsRect);
 [ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData);
 
 
-[scriptable, uuid(abe54136-49e5-44ca-a749-290038c6b85d)]
+[scriptable, uuid(c16fde76-3108-450e-8c8c-ae8286f286ed)]
 interface nsISHEntry : nsIHistoryEntry
 {
     /** URI for the document */
     void setURI(in nsIURI aURI);
 
     /** Referrer URI */
     attribute nsIURI referrerURI;
 
--- a/docshell/test/navigation/Makefile.in
+++ b/docshell/test/navigation/Makefile.in
@@ -45,16 +45,17 @@ include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES = \
 		test_bug13871.html \
 		test_bug270414.html \
 		test_bug278916.html \
 		test_bug279495.html \
 		test_bug386782.html \
+ 		test_bug430624.html \
 		test_bug430723.html \
 		test_child.html \
 		test_grandchild.html \
 		test_sibling-off-domain.html \
 		test_sibling-matching-parent.html \
 		test_opener.html \
 		test_not-opener.html \
 		test_popup-navigates-children.html \
new file mode 100644
--- /dev/null
+++ b/docshell/test/navigation/test_bug430624.html
@@ -0,0 +1,55 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=430624
+-->
+<head>
+  <title>Test for Bug 430624</title>
+  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>  
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=430624">Mozilla Bug 430624</a>
+<p id="display"></p>
+
+
+
+<div id="content" style="display: none">
+  
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+/** Test for Bug 430624 **/
+
+function onLoad() {
+  window.frames[0].frameElement.onload = onReload;
+  window.frames[0].location = window.frames[0].location;
+}
+
+function onReload() {
+  var bodyElement = window.frames[0].frameElement.contentDocument.body;
+  sendChar('S', bodyElement);
+  sendChar('t', bodyElement);
+  sendChar('i', bodyElement);
+  sendChar('l', bodyElement);
+  sendChar('l', bodyElement);
+  sendChar(' ', bodyElement);
+
+  is(bodyElement.innerHTML, "Still contentEditable", "Check we're contentEditable after reload");
+
+  SimpleTest.finish();
+}
+
+SimpleTest.waitForExplicitFinish();
+
+</script>
+</pre>
+
+<iframe onload="onLoad()" src="data:text/html;charset=utf-8,<body contenteditable>contentEditable</body>"></iframe>
+
+</body>
+</html>
+
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/base/crashtests/430624-1.html
@@ -0,0 +1,14 @@
+<html>
+<head>
+<script>
+function crash() {
+  window.frames[0].onload = null;
+  window.frames[0].location = 'data:text/html;charset=utf-8,2nd%20page';
+}
+</script>
+</head>
+<body onload="crash()">
+  <!-- iframe contents: <html><body onload="document.body.setAttribute('spellcheck', true);"></body></html> -->
+  <iframe src="data:text/html;charset=utf-8;base64,PGh0bWw%2BPGJvZHkgb25sb2FkPSJkb2N1bWVudC5ib2R5LnNldEF0dHJpYnV0ZSgnc3BlbGxjaGVjaycsIHRydWUpOyI%2BPC9ib2R5PjwvaHRtbD4%3D"></iframe>
+</body>
+</html>
\ No newline at end of file
--- a/editor/libeditor/base/crashtests/crashtests.list
+++ b/editor/libeditor/base/crashtests/crashtests.list
@@ -1,3 +1,4 @@
 load 382527-1.html
 load 402172-1.html
 load 407256-1.html
+load 430624-1.html