386782 - After going back to a page, contenteditable elements aren't editable.

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Albert Brand]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/2007070304 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/2007070304 Minefield/3.0a7pre

See the testcase URL (from bug 198155). Go to that URL, see that it works by typing in some text, visit another URL (for instance, go home) and go back. Now, try to edit the text, it's not possible anymore. Notice that you also don't see the caret when clicking on text.

Reproducible: Always



Expected Results:  
When returning to a page with editable elements, they should still be editable.

Comment 1

[Martijn Wargers (dead)]

Still seems to happen in current trunk build.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Isn't this a dup?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #2)
> Isn't this a dup?

It's not a dupe of bug 412920, the patch for that landed, and I can still reproduce this. I had a feeling there was a bfcache bug related to this, but I can't find it.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Setting the pref browser.sessionhistory.max_total_viewers=0 disables the bfcache. It also stops this bug happening, so I guess this is a probblem with the bfcache.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

I note that if you load a page with a <textarea> and a contenteditable element, when you load another page, the nsEditor for contenteditable is destroyed, but the nsEditor for the <textarea> isn't. Here's the callstack for the call to nsEditor::~nsEditor():

gklayout.dll!nsEditor::~nsEditor()  Line 188	C++
gklayout.dll!nsPlaintextEditor::~nsPlaintextEditor()  Line 118 + 0x1e bytes	C++
gklayout.dll!nsHTMLEditor::~nsHTMLEditor()  Line 238 + 0x204 bytes	C++
gklayout.dll!nsHTMLEditor::`scalar deleting destructor'()  + 0xf bytes	C++
gklayout.dll!nsEditor::Release()  Line 231 + 0xdc bytes	C++
gklayout.dll!nsHTMLEditor::Release()  Line 248 + 0xd bytes	C++
composer.dll!nsCOMPtr<nsIEditor>::~nsCOMPtr<nsIEditor>()  Line 584	C++
composer.dll!nsEditingSession::TearDownEditorOnWindow(nsIDOMWindow * aWindow=0x06206a40)  Line 686 + 0x11 bytes	C++
composer.dll!nsEditingSession::StartDocumentLoad(nsIWebProgress * aWebProgress=0x062035ec, int aIsToBeMadeEditable=1)  Line 1025	C++
composer.dll!nsEditingSession::OnStateChange(nsIWebProgress * aWebProgress=0x062035ec, nsIRequest * aRequest=0x078437c8, unsigned int aStateFlags=983041, unsigned int aStatus=0)  Line 805	C++
docshell.dll!nsDocLoader::FireOnStateChange(nsIWebProgress * aProgress=0x062035ec, nsIRequest * aRequest=0x078437c8, int aStateFlags=983041, unsigned int aStatus=0)  Line 1236	C++
docshell.dll!nsDocLoader::doStartDocumentLoad()  Line 797	C++
docshell.dll!nsDocLoader::OnStartRequest(nsIRequest * request=0x078437c8, nsISupports * aCtxt=0x00000000)  Line 529	C++
necko.dll!nsLoadGroup::AddRequest(nsIRequest * request=0x078437c8, nsISupports * ctxt=0x00000000)  Line 603 + 0x21 bytes	C++
necko.dll!nsHttpChannel::AsyncOpen(nsIStreamListener * listener=0x076822d8, nsISupports * context=0x00000000)  Line 3683	C++
docshell.dll!nsURILoader::OpenURI(nsIChannel * channel=0x078437c8, int aIsContentPreferred=0, nsIInterfaceRequestor * aWindowContext=0x062035f0)  Line 839 + 0x19 bytes	C++
docshell.dll!nsDocShell::DoChannelLoad(nsIChannel * aChannel=0x078437c8, nsIURILoader * aURILoader=0x037842a0, int aBypassClassifier=0)  Line 7479 + 0x41 bytes	C++
docshell.dll!nsDocShell::DoURILoad(nsIURI * aURI=0x06557958, nsIURI * aReferrerURI=0x00000000, int aSendReferrer=1, nsISupports * aOwner=0x00000000, const char * aTypeHint=0x00000000, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, int aFirstParty=1, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x0012d4c8, int aIsNewWindowTarget=0, int aBypassClassifier=0)  Line 7325 + 0x29 bytes	C++
docshell.dll!nsDocShell::InternalLoad(nsIURI * aURI=0x06557958, nsIURI * aReferrer=0x00000000, nsISupports * aOwner=0x00000000, unsigned int aFlags=1, const wchar_t * aWindowTarget=0x07ed42f8, const char * aTypeHint=0x00000000, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, unsigned int aLoadType=1, nsISHEntry * aSHEntry=0x00000000, int aFirstParty=1, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x00000000)  Line 7064 + 0x7f bytes	C++
docshell.dll!nsDocShell::LoadURI(nsIURI * aURI=0x06557958, nsIDocShellLoadInfo * aLoadInfo=0x076418b0, unsigned int aLoadFlags=0, int aFirstParty=1)  Line 902 + 0x56 bytes	C++
docshell.dll!nsDocShell::LoadURI(const wchar_t * aURI=0x0782f2f8, unsigned int aLoadFlags=0, nsIURI * aReferringURI=0x00000000, nsIInputStream * aPostStream=0x00000000, nsIInputStream * aHeaderStream=0x00000000)  Line 2891 + 0x2a bytes	C++
xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x0012da64, unsigned int methodIndex=26589230, unsigned int paramCount=1235600, nsXPTCVariant * params=0x0012da64)  Line 102	C++
xpcom_core.dll!xptiInterfaceInfo::GetIIDForParamNoAlloc(unsigned short methodIndex=8, const nsXPTParamInfo * param=0x00000005, nsID * iid=0x0012d8d4)  Line 720 + 0x2e bytes	C++
xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2339 + 0x21 bytes	C++
xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x038764d0, JSObject * obj=0x07351120, unsigned int argc=5, long * argv=0x05e89b9c, long * vp=0x0012db9c)  Line 1470 + 0xe bytes	C++
js3250.dll!js_Invoke(JSContext * cx=0x038764d0, unsigned int argc=5, long * vp=0x05e89b94, unsigned int flags=0)  Line 1414 + 0x20 bytes	C
js3250.dll!js_Interpret(JSContext * cx=0x038764d0, unsigned char * pc=0x0643d1e6, long * result=0x0012e378)  Line 4649 + 0x16 bytes	C
js3250.dll!js_Invoke(JSContext * cx=0x038764d0, unsigned int argc=1, long * vp=0x05e8977c, unsigned int flags=2)  Line 1430 + 0x13 bytes	C
js3250.dll!js_InternalInvoke(JSContext * cx=0x038764d0, JSObject * obj=0x073635c0, long fval=85602624, unsigned int flags=0, unsigned int argc=1, long * argv=0x05e89778, long * rval=0x0012e490)  Line 1486 + 0x18 bytes	C
js3250.dll!JS_CallFunctionValue(JSContext * cx=0x038764d0, JSObject * obj=0x073635c0, long fval=85602624, unsigned int argc=1, long * argv=0x05e89778, long * rval=0x0012e490)  Line 4982 + 0x1f bytes	C
gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x060cd768, void * aScope=0x051a3760, void * aHandler=0x051a3140, nsIArray * aargv=0x08037848, nsIVariant * * arv=0x0012e65c)  Line 1947 + 0x24 bytes	C++
gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x0782f418)  Line 248 + 0x67 bytes	C++
gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x062e0200, nsIDOMEventListener * aListener=0x064a1498, nsIDOMEvent * aDOMEvent=0x0782f418, nsISupports * aCurrentTarget=0x060cd768, unsigned int aPhaseFlags=6)  Line 1082 + 0x12 bytes	C++
gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x0602c958, nsEvent * aEvent=0x0012eacc, nsIDOMEvent * * aDOMEvent=0x0012e934, nsISupports * aCurrentTarget=0x060cd768, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012e938)  Line 1190	C++
gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 207	C++
gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x0012e9e8)  Line 266	C++
gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x060cd768, nsPresContext * aPresContext=0x0602c958, nsEvent * aEvent=0x0012eacc, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012ef00, nsDispatchingCallback * aCallback=0x0012e9e8)  Line 479 + 0x12 bytes	C++
gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012eacc, nsIView * aView=0x00000000, nsEventStatus * aStatus=0x0012ef00)  Line 5867 + 0x29 bytes	C++
gklayout.dll!PresShell::HandleEventWithTarget(nsEvent * aEvent=0x0012eacc, nsIFrame * aFrame=0x0609f328, nsIContent * aContent=0x060cd768, nsEventStatus * aStatus=0x0012ef00)  Line 5772 + 0x12 bytes	C++
gklayout.dll!nsEventStateManager::CheckForAndDispatchClick(nsPresContext * aPresContext=0x0602c958, nsMouseEvent * aEvent=0x0012f164, nsEventStatus * aStatus=0x0012ef00)  Line 3345 + 0x45 bytes	C++
gklayout.dll!nsEventStateManager::PostHandleEvent(nsPresContext * aPresContext=0x0602c958, nsEvent * aEvent=0x0012f164, nsIFrame * aTargetFrame=0x0609f328, nsEventStatus * aStatus=0x0012ef00, nsIView * aView=0x060002a0)  Line 2409 + 0x1c bytes	C++
gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f164, nsIView * aView=0x060002a0, nsEventStatus * aStatus=0x0012ef00)  Line 5888 + 0x3a bytes	C++
gklayout.dll!PresShell::HandlePositionedEvent(nsIView * aView=0x060002a0, nsIFrame * aTargetFrame=0x0609f328, nsGUIEvent * aEvent=0x0012f164, nsEventStatus * aEventStatus=0x0012ef00)  Line 5755 + 0x14 bytes	C++
gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x060002a0, nsGUIEvent * aEvent=0x0012f164, nsEventStatus * aEventStatus=0x0012ef00)  Line 5615 + 0x1e bytes	C++
gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x060002a0, nsPoint aPoint={...}, nsGUIEvent * aEvent=0x0012f164, int aCaptured=0)  Line 1383	C++
gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f164, nsEventStatus * aStatus=0x0012f048)  Line 1335 + 0x22 bytes	C++
gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f164)  Line 171	C++
gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f164, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 972 + 0xc bytes	C++
gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f164)  Line 993	C++
gkwidget.dll!nsWindow::DispatchMouseEvent(unsigned int aEventType=301, unsigned int wParam=0, long lParam=4784161, int aIsContextMenuKey=0, short aButton=0)  Line 5865 + 0x1a bytes	C++
gkwidget.dll!ChildWindow::DispatchMouseEvent(unsigned int aEventType=301, unsigned int wParam=0, long lParam=4784161, int aIsContextMenuKey=0, short aButton=0)  Line 6038	C++
gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=514, unsigned int wParam=0, long lParam=4784161, long * aRetValue=0x0012f61c)  Line 4341 + 0x24 bytes	C++
gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x00060470, unsigned int msg=514, unsigned int wParam=0, long lParam=4784161)  Line 1187 + 0x1d bytes	C++
user32.dll!76851a10() 	
[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]	
user32.dll!76851ae8() 	
user32.dll!76851a91() 	
user32.dll!76852a47() 	
user32.dll!7684c5cd() 	
user32.dll!76852a98() 	
gkwidget.dll!nsAppShell::ProcessNextNativeEvent(int mayWait=1)  Line 149	C++
gkwidget.dll!nsBaseAppShell::DoProcessNextNativeEvent(int mayWait=1)  Line 137 + 0x11 bytes	C++
gkwidget.dll!nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal * thr=0x012ef298, int mayWait=1, unsigned int recursionDepth=0)  Line 247 + 0xf bytes	C++
xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012f828)  Line 500	C++
xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x012ef298, int mayWait=1)  Line 227 + 0x16 bytes	C++
gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
tkitcmps.dll!nsAppStartup::Run()  Line 181 + 0x1c bytes	C++
xul.dll!XRE_main(int argc=4, char * * argv=0x012e0b80, const nsXREAppData * aAppData=0x012e7c68)  Line 3149 + 0x25 bytes	C++
firefox.exe!NS_internal_main(int argc=4, char * * argv=0x012e0b80)  Line 158 + 0x12 bytes	C++
firefox.exe!wmain(int argc=4, wchar_t * * argv=0x012cd440)  Line 87 + 0xd bytes	C++
firefox.exe!__tmainCRTStartup()  Line 594 + 0x19 bytes	C
firefox.exe!wmainCRTStartup()  Line 414	C
kernel32.dll!76ab3833() 	
ntdll.dll!77cba9bd() 	


Taking bug.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Adding PeterV to cc list. Peter, care to comment?

So the problem is that nsDocLoader::doStartDocumentLoad() calls down a chain to nsEditingSession::TearDownEditorOnWindow(), which deletes the old editor on its window. The editor is not being recreated when we restore from history. I think that this behavior was added to fix bug 400556. It would also be nice if the history restore also restored the editor's undo/redo stack, so we could still undo-redo like we do for <textarea> etc in the same situation.

If we get desperate, a quick fix would be to change nsDocument::CanSavePresentation() to check if we're in an html document with contenteditable nodes, and if so, not allow the document's presentation to be saved/restored. That won't restore the editor's undo stack of course, so I'll keep looking into it...

Comment 7

[Peter Van der Beken [:peterv]]

Does this also fail for designMode? I think I fixed that in bug 403501, I wonder what the difference is between designMode and contentEditable.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #7)
> Does this also fail for designMode? I think I fixed that in bug 403501, I
> wonder what the difference is between designMode and contentEditable.
> 

It fails if the main document is in designMode (e.g. <body onload="document.designMode='on'">), but not designMode iframes which are in a non-designMode document. contentEditable always fails.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Ugly WIP patch

This is one way to do it. Backup the editing state when we tear down the editor, and recreate the editor state when we session-restore it. Peterv, what do you think of this approach?

The problem with this patch, is that it doesn't remember the undo/redo stack, and when you navigate fwd/back to a designMode document. Also the caret doesn't appear in the designMode documents on restore, even though they're still editable. I'll see if I can fix the caret, and backup the TxnMgr somehow.

The problem with the current approach of having the editor on the docShell is that the main docShell stomps its nsDocShellEditorData when it loads a new page. That's why editable iframes work correctly WRT shistory restore; only the root docshell stomps its state, subframes remember theirs. 

I think perhaps a better approach would be to move the editor and its data off the docShell and onto the nsIHTMLDocument. We'd still need to detach/reattach the editor from its window when we load a new page or restore an old page into the docShell. Moving the editor off into nsIHTMLDocument matches the approach with <input>s, whose nsTextControlFrames own their nsIEditors. Messing with the lifecycle of the editor is a scary change though... Peterv, what do you think?

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1.1 backup/restore editable modes, but not undo/redo

This adds SaveEditingState()/RestoreEditingState() to nsIHTMLDocument, which is called from { nsHTMLDocument::TearingDownEditor(), nsGlobalWindow::SaveWindowState() }/nsGlobalWindow::RestoreWindowState() respectively.

I also add a fix to ensure the caret is visible when we restore a designMode document. Unfortunately this does not backup/restore the TxnMgr. To do that I'd have to start messing with and make accessible the mEditor weak references that are scattered around the place in transactions lists etc, or move the editor data out into some other class...

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> Unfortunately this does not backup/restore the TxnMgr. To do that I'd
> have to start messing with and make accessible the mEditor weak references that
> are scattered around the place in transactions lists etc, or move the editor
> data out into some other class...
> 

What I mean by this, is that to restore the transaction manager, I need to update the old txnmgr's transactions' mEditor field in the undo/redo stack. To do that from nsIHTMLDocument I need to create some public function on the nsITransactionList to allow you to reset the nsEditor of all its transactions. Does that sound acceptable?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It'd probably be best to fix this bug first without restoring the transaction manager, and then do a separate patch to restore the transaction manager, if that doesn't create much extra work. That issue is relatively minor and there are other important bugs to work on... Plus, it seems to me the long-term fix is to attach all the editor state to the document instead of the window, then this wouldn't be an issue.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V2 WIP - detach/reattach nsDocShellEditorData

I've had a go at storing/restoring the DocShell's nsDocShellEditorData on nsSHEntry. This patch is my work in progress so far. It works, complete with undo/redo, though I didn't expect it to as it doesn't reattach the listeners/controllers yet. It is also crashing on app-exit, so I haven't worked the kinks out, but I should be able to get this working tomorrow.

Any comments on my approach would be welcome.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - backup docshellEditorData and txnmgr

This detaches/reattaches the docshellEditorData, restores editing session along with transaction manager, and its undo/redo stack.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: v3.1 - v3 with leak fixed

This is the same as patch V3 but it no longer causes a leak. The only difference between this and v3 is I had to change nsDocShellEditorData::TearDownEditor() to always call nsEditor::PreDestroy().

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Mochitest

Mochitest. I'll merge this with the 3.1 patch when I make any changes requested by reviewer.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 311895 [details] [diff] [review]
v3.1 - v3 with leak fixed

Looks good, but I'm still figuring out some of the details of the new architecture.

>Index: content/html/document/src/nsHTMLDocument.h
>===================================================================

>+  virtual nsresult SetEditingState(EditingState aState);

Add a blank line.

> protected:

>Index: docshell/base/nsDocShell.cpp
>===================================================================

>@@ -5260,16 +5265,117 @@ nsDocShell::CanSavePresentation(PRUint32

>+HasDetachedEditor(nsISHEntry *aEntry)
>+{
>+    if (!aEntry)
>+        return PR_FALSE;
>+    PRBool hasEditor = PR_FALSE;
>+    if (NS_FAILED(aEntry->HasDetachedEditor(&hasEditor)))
>+        return PR_FALSE;
>+    return hasEditor;

I'd do:

    PRBool hasEditor = PR_FALSE;
    return aEntry && NS_SUCCEEDED(aEntry->HasDetachedEditor(&hasEditor)) &&
           hasEditor;

>+nsDocShell::HasDetachedEditor()
>+{
>+  return ::HasDetachedEditor(mOSHE) || ::HasDetachedEditor(mLSHE);

Rewrap.

>+}
>+
>+nsresult
>+nsDocShell::ReattachEditorToWindow(nsIDOMWindow *aWindow, nsISHEntry *aSHEntry)
>+{
>+    #ifdef DEBUG

No spaces before preprocessor directives.

>+    mIsReattachingEditor = PR_TRUE;
>+    #endif
>+
>+    NS_ASSERTION(!mEditorData, "Why are we reattaching an editor when we already have one?");
>+    NS_ASSERTION(aWindow, "Need a window to reattach to");
>+    NS_ASSERTION(HasDetachedEditor(), "Reattaching when there's not a detached editor.");

Rewrap long lines (here and in other places).

>+
>+    if (mEditorData || !aWindow) {
>+      #ifdef DEBUG
>+      mIsReattachingEditor = PR_FALSE;
>+      #endif
>+      return NS_OK;
>+    }

Can this really happen? I'd just rely on the assertions.

>+    nsresult rv = NS_OK;

Declare rv where you need it.

>+    return rv;

return NS_OK

>+nsresult

void, you only return NS_OK and no callers care.

>+nsDocShell::ResetDetachedState(nsISHEntry *aEntry)
>+{
>+    NS_ASSERTION(HasDetachedEditor() && !mEditorData,
>+        "Editor should be detached or not exist when reseting detached state...");
>+    PRBool hasDetachedEditor = PR_FALSE;

hasDetachedEditor looks unused.

>+    if (aEntry)
>+        aEntry->SetEditorData(nsnull);
>+    return NS_OK;
>+}

I'd make this a static non-member function.

>+
>+nsresult

No caller seems to care about the return value, so maybe make this void?

>+nsDocShell::DetachEditorFromWindow(nsISHEntry *aSHEntry)

>+nsresult

No caller seems to care about the return value, so maybe make this void?

>+nsDocShell::DetachEditorFromWindow()

>@@ -6753,20 +6866,19 @@ nsDocShell::InternalLoad(nsIURI * aURI,

>-
>+        

No need for these whitespace changes.

>@@ -7067,16 +7183,35 @@ nsDocShell::InternalLoad(nsIURI * aURI,

>+        // We've just loaded a page from session history. Reattach
>+        // its editing session if it has one.
>+        nsCOMPtr<nsPIDOMWindow> privWin =
>+            do_GetInterface(static_cast<nsIInterfaceRequestor*>(this));
>+        NS_ASSERTION(privWin, "could not get nsPIDOMWindow interface");
>+
>+        nsCOMPtr<nsIDOMWindow> domWin = do_QueryInterface(privWin);

Just do:

      nsCOMPtr<nsIDOMWindow> domWin;
      CallGetInterface(this, getter_AddRefs(domWin));

>+        rv = ReattachEditorToWindow(domWin, aSHEntry);
>+        NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to reattach editing session");
>+        NS_ENSURE_SUCCESS(rv,rv);

Return rv here, drop the else and return NS_OK at the end.

>+    } else if (aSHEntry) {


> nsDocShell::EnsureEditorData()

>+#if DEBUG

#ifdef

>Index: docshell/base/nsDocShell.h
>===================================================================

>+#include "nsIDocShellEditorData.h"

>+#include "nsIDocShellEditorData.h"

Don't include this twice.

>Index: docshell/base/nsDocShellEditorData.cpp
>===================================================================

>-nsDocShellEditorData::nsDocShellEditorData(nsIDocShell* inOwningDocShell)
>-: mDocShell(inOwningDocShell)
>-, mMakeEditable(PR_FALSE)
>+nsresult nsDocShellEditorData::Init(nsIDocShell* inOwningDocShell)
> {
>+  mDocShell = inOwningDocShell;
>+  mMakeEditable = PR_FALSE;
>+  mIsDetached = PR_FALSE;
>   NS_ASSERTION(mDocShell, "Where is my docShell?");
>+  return NS_OK;
> }

I think we should only allow the docshell code to create a nsDocShellEditorData, so drop the contract ID/CID and the Init method and do everything in the constructor.

>+nsresult
>+nsDocShellEditorData::DetachFromWindow()
>+{
>+  NS_ASSERTION(mEditingSession,
>+    "Can't detach when we don't have a session to detach!");

Line up " and (.

>+  nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(domDoc, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  mDetachedEditingState = (htmlDoc) ? htmlDoc->GetEditingState()
>+                                    : nsIHTMLDocument::eOff;

htmlDoc can't be null here.

>+  
>+  return rv;

return NS_OK.

>+nsDocShellEditorData::ReattachToWindow(nsIDOMWindow *aWindow)

>+  nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(domDoc, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (htmlDoc) {
>+    htmlDoc->SetEditingState(mDetachedEditingState);

htmlDoc can't be null here.

>+  }
>+  return rv;

return NS_OK.

>Index: docshell/base/nsDocShellEditorData.h
>===================================================================

>+  // while its stored in the session history bfcache.

it's


>Index: docshell/base/nsIDocShellEditorData.h
>===================================================================

>+class nsIDocShellEditorData : public nsISupports

Sucks that we have to add a new interface for this :-/.

>+  virtual nsresult MakeEditable(PRBool inWaitForUriLoad)=0;

Spaces around ='s.

>Index: docshell/shistory/public/nsISHEntry.idl
>===================================================================

>+    [noscript] void hasDetachedEditor(out boolean aHasDetachedEditor);

Make it notxpcom and returning boolean.

>Index: docshell/shistory/src/Makefile.in
>===================================================================

> 		  nkcache \
>+      editor \
>+      composer \

You want some tabs here.

>Index: docshell/shistory/src/nsSHEntry.cpp
>===================================================================

>@@ -105,16 +107,17 @@ nsSHEntry::nsSHEntry() 

>+  , mEditorData(nsnull)

No need for this.

>@@ -129,16 +132,17 @@ nsSHEntry::nsSHEntry(const nsSHEntry &ot

>+  , mEditorData(nsnull)

Ditto.

>@@ -156,16 +160,20 @@ nsSHEntry::~nsSHEntry()

>+  if (mEditorData) {
>+    mEditorData = nsnull;
>+  }

Ditto.

>@@ -828,8 +836,31 @@ nsSHEntry::DocumentMutated()

>+nsSHEntry::GetEditorData(nsIDocShellEditorData** aData)
>+{
>+  NS_ENSURE_ARG_POINTER(aData);

I don't think we should care.

>+  *aData = mEditorData;
>+  NS_IF_ADDREF(*aData);

Combine these two lines:

  NS_IF_ADDREF(*aData = mEditorData);

>+nsSHEntry::HasDetachedEditor(PRBool* aHasDetachedEditor) {

Brace on new line.

>Index: editor/composer/src/nsEditingSession.cpp
>===================================================================

>+nsEditingSession::DetachFromWindow(nsIDOMWindow* aWindow)

>+  if (mMakeWholeDocumentEditable) {
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    aWindow->GetDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

What is this trying to achieve?

Could we refactor to share some of this code with nsEditingSession::TearDownEditorOnWindow?

>Index: editor/composer/src/nsEditingSession.h
>===================================================================

>+  nsComposerCommandsUpdater* mComposerCommandsUpdater;

Can't we change mStateMaintainer to be nsRefPtr<nsComposerCommandsUpdater> instead?

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yikes, if you load an email in from your gmail inbox and then press the navigate back button, it doesn't navigate back to your inbox. This happens for me on nightlies at least as far back as 11-14-04, maybe further. My patch fixes this, so I'm marking this bug as blocking two bug which I think are dupes of the "can't nav back in gmail" bug.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

Ok, next revision, which includes a mochitest.

(In reply to comment #17)

> >+
> >+    if (mEditorData || !aWindow) {
> >+      #ifdef DEBUG
> >+      mIsReattachingEditor = PR_FALSE;
> >+      #endif
> >+      return NS_OK;
> >+    }
> 
> Can this really happen? I'd just rely on the assertions.

It was acutally a logic error, caused by detaching failing, and not releasing the ref to mEditorData when it failed. Fixed now...


> >+nsDocShell::ResetDetachedState(nsISHEntry *aEntry)
> I'd make this a static non-member function.

I removed it, which made more sense once I'd refactored a bit.


> >+nsDocShell::DetachEditorFromWindow(nsISHEntry *aSHEntry)
> No caller seems to care about the return value, so maybe make this void?

I think we should know if this fails, so it still returns nsresult, but now I assert that it succeeds. I don't want to NS_ENSURE_SUCCESS, as if the detach/reattach fails and we bail out, we can stop the loading process, and the new page is not loaded and displayed to the user. At least this way the assertion will trigger, but the page will at least partially load.


> >@@ -7067,16 +7183,35 @@ nsDocShell::InternalLoad(nsIURI * aURI,
> >+        rv = ReattachEditorToWindow(domWin, aSHEntry);
> >+        NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to reattach editing session");
> >+        NS_ENSURE_SUCCESS(rv,rv);
> 
> Return rv here, drop the else and return NS_OK at the end.

I'm still returning rv, because I want to clear the ref to the editor data on the nsISHEntry. It leaks if the reattach fails otherwise.

 
> >Index: editor/composer/src/nsEditingSession.h
> >===================================================================
> 
> >+  nsComposerCommandsUpdater* mComposerCommandsUpdater;
> 
> Can't we change mStateMaintainer to be nsRefPtr<nsComposerCommandsUpdater>
> instead?
> 

That makes things a lot simpler!

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 315021 [details] [diff] [review]
Patch v4

I'd still make DetachEditorFromWindow and ReattachEditorToWindow return void and instead move your assertion into the functions themselves (they both can only fail in one place).

>Index: docshell/base/nsDocShell.cpp
>===================================================================

>+nsDocShell::ReattachEditorToWindow(nsIDOMWindow *aWindow, nsISHEntry *aSHEntry)

>+#ifdef DEBUG
>+        mIsReattachingEditor = PR_FALSE;
>+#endif
>+        NS_ENSURE_SUCCESS(rv,rv);

Space after comma.

>+nsDocShell::DetachEditorFromWindow(nsISHEntry *aSHEntry)
>+{
>+    if (!aSHEntry || !mEditorData)
>+        return NS_OK;
>+
>+    if (aSHEntry->HasDetachedEditor())
>+        return NS_OK;

Can this happen (having both mEditorData and aSHEntry->HasDetachedEditor())? If not we should just assert that.

>@@ -7103,16 +7203,33 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>     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)));
>+            rv = ReattachEditorToWindow(domWin, aSHEntry);
>+            NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to reattach editing session");

Shouldn't you return here?

>+        }
>+
>+        // 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);
>+    }

> nsDocShell::EnsureEditorData()
> {
>-    if (!mEditorData && !mIsBeingDestroyed)
>+#ifdef DEBUG
>     {
>+        PRBool hasDetachedEditor = HasDetachedEditor();
>+        NS_ASSERTION((!mIsReattachingEditor || hasDetachedEditor),
>+                     "EnsureEditorData() called when detached.\n");
>+    }
>+#endif

Make the assertion's condition be |!mIsReattachingEditor || HasDetachedEditor()| and you don't need the ifdefs and the braces.

>Index: docshell/base/nsDocShellEditorData.cpp
>===================================================================

>@@ -69,38 +77,31 @@ nsDocShellEditorData::nsDocShellEditorDa

> nsDocShellEditorData::TearDownEditor()
> {

Can we assert mIsDetached here?

>@@ -213,20 +214,64 @@ nsDocShellEditorData::SetEditor(nsIEdito

>+  NS_ASSERTION(!mIsDetached, "Shouldn't do this when detached! Will stomp the session!");

Long line.

>+nsDocShellEditorData::DetachFromWindow()

>+  nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(domDoc);
>+  if (htmlDoc)
>+    mDetachedEditingState = htmlDoc->GetEditingState();

Should this reset mDetachedEditingState if !htmlDoc?

>Index: docshell/base/nsDocShellEditorData.h
>===================================================================

>+class nsDocShellEditorData : public nsIDocShellEditorData

I wonder if we could avoid this by using

[ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData);

in nsISHEntry, it sucks to add XPCOM to this class.

>Index: docshell/test/navigation/test_bug386782a.html
>===================================================================

>+    var testInterval = 2000;

Why do we need this and does it need to be so high? If we're waiting for loads to finish it might be better to use onload handlers.

>+      ok(bodyContentsBeforeEdit == expectedBodyBeforeEdit, "Is doc setup yet");

I'd use is(...) for comparisons.

Could we refactor the tests so that we have only one file? They look 99% identical.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Just some quick comments on comments.

(In reply to comment #22)
> >@@ -7103,16 +7203,33 @@ nsDocShell::InternalLoad(nsIURI * aURI,
> >     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)));
> >+            rv = ReattachEditorToWindow(domWin, aSHEntry);
> >+            NS_ASSERTION(NS_SUCCEEDED(rv), "Failed to reattach editing session");
> 
> Shouldn't you return here?
> 

If we return here, we won't 'aSHEntry->SetEditorData(nsnull);' before we leave the function, and we'll leak the nsIDocshellEditorData (if ReattachEditorToWindow() fails), so I think this is ok. Not sure why the cycle collector couldn't deal with this, though there's lots of refs in the editor and editingSession and stateMaintainer which depend on the nsIDocshellEditorData.

> >+        }
> >+
> >+        // 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);
> >+    }
> 


> >Index: docshell/base/nsDocShellEditorData.h
> >===================================================================
> 
> >+class nsDocShellEditorData : public nsIDocShellEditorData
> 
> I wonder if we could avoid this by using
> 
> [ptr] native nsDocShellEditorDataPtr(nsDocShellEditorData);
> 
> in nsISHEntry, it sucks to add XPCOM to this class.
> 

I added the XPCOM to this because I wanted it refcounted. Roc tells me I can do something like this to achieve that:

http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxContext.h#69
http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxTypes.h#64

Let me know if either of that's not ok.

Comment 24

[Peter Van der Beken [:peterv]]

> > Shouldn't you return here?
> > 
> 
> If we return here, we won't 'aSHEntry->SetEditorData(nsnull);' before we leave
> the function, and we'll leak the nsIDocshellEditorData (if
> ReattachEditorToWindow() fails), so I think this is ok.

Maybe ReattachEditorToWindow should do it in the one failure case where it doesn't already instead? It's a bit confusing because you fall through to the comment below it, which doesn't apply:

> > >+        // This is a non-history load from a session history entry. Purge any

> I added the XPCOM to this because I wanted it refcounted. Roc tells me I can do
> something like this to achieve that:
> 
> http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxContext.h#69
> http://mxr.mozilla.org/seamonkey/source/gfx/thebes/public/gfxTypes.h#64

That's fine (no need for a macro though). Just add inline AddRef/Release implementations and use nsRefPtr to store the nsDocShellEditorData.

Comment 25

[Peter Van der Beken [:peterv]]

>Index: editor/composer/src/nsEditingSession.cpp
>===================================================================

>+nsEditingSession::ReattachToWindow(nsIDOMWindow* aWindow)

>+  // old editor ot the window.

to

Don't think I have anything else, except maybe why do we need to reset/restore animation and JS/plugins? Would it work to just leave things as-is?

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: v5

(In reply to comment #24)
> > > Shouldn't you return here?
>
> > If we return here, we won't 'aSHEntry->SetEditorData(nsnull);' before we leave
> > the function
> 
> Maybe ReattachEditorToWindow should do it in the one failure case where it
> doesn't already instead?

Ok, that makes sense.


> > I added the XPCOM to this because I wanted it refcounted.
> That's fine (no need for a macro though). Just add inline AddRef/Release
> implementations and use nsRefPtr to store the nsDocShellEditorData.
> 

What I've done is made the nsDocShellEditorData not an XPCOM class, and made its pointer held by an nsAutoPtr. The editor data's pointer should only ever be held by either its docshell, or its nsISHEntry, but never both at the same time, so nsDocShell::ReattachEditorToWindow() takes ownership of the nsAutoPointer, telling nsISHEntry to forget its pointer. Hopefully that's inline with your comment #24.

(In reply to comment #25)
> >+nsEditingSession::ReattachToWindow(nsIDOMWindow* aWindow)
> 
> Don't think I have anything else, except maybe why do we need to reset/restore
> animation and JS/plugins? Would it work to just leave things as-is?

Strangely, reloading an existing designMode page fails if you don't have the animation/JS/plugins, but back/forward loads work ok, so I've left it as is.

Other changes:

* Patch no longer adds nsDocShell::mIsReattachingEditor.
* Refactored mochitest into 1 file. I couldn't get it to work with event listeners though, it still uses timeouts. I couldn't get the listeners to work across windows... I tried... :\
* Made nsDocShell::ReattachEditorToWindow and DetachEditorFromWindow not return failure, just assert instead.

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 316768 [details] [diff] [review]
v5

Looks great!

>Index: docshell/base/nsDocShell.cpp
>===================================================================

> nsDocShell::EnsureEditorData()
> {
>-    if (!mEditorData && !mIsBeingDestroyed)
>-    {
>+    NS_ASSERTION(!HasDetachedEditor(),
>+                 "EnsureEditorData() called when detached.\n");

No need for the \r

>Index: docshell/base/nsDocShellEditorData.h
>===================================================================

>+  virtual nsresult MakeEditable(PRBool inWaitForUriLoad);
>+  virtual PRBool GetEditable();
>+  virtual nsresult CreateEditor();
>+  virtual nsresult GetEditingSession(nsIEditingSession **outEditingSession);
>+  virtual nsresult GetEditor(nsIEditor **outEditor);
>+  virtual nsresult SetEditor(nsIEditor *inEditor);
>+  virtual void TearDownEditor();
>+  virtual nsresult DetachFromWindow();
>+  virtual nsresult ReattachToWindow(nsIDOMWindow *aWindow);

These don't need to be virtual anymore.

>Index: docshell/shistory/public/nsISHEntry.idl
>===================================================================

>+     * Gets the owning pointer to the editor data assosicated with
>+     * this shistory entry. This shentry will forget its pointer
>+     * when you call this, so you must free this when your done with

...free it when you're done...

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V6

V5 plus changes requested in comment 27. Includes mochitest. r+/sr+ peterv.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V6.1 - merge conflicts resolved

The same as V6, but resolved merge conflicts/bitrot. r+/sr+ peterv.

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Can I get Approval1.9 on v6.1 patch please? The mochitests pass (at least on Mac & Linux, last time I ran them on Windows they passed there too). This patch has a small chance of causing regressions; it makes additions to the loading code, but doesn't change the loading code execution path. This patch also fixes a major bug in gmail, and it includes a mochitest.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317172 [details] [diff] [review]
V6.1 - merge conflicts resolved

a1.9=beltzner

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 33

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008042705 Minefield/3.0pre ID:2008042705 and the steps to reproduce from comment #0

--> Verified fixed

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

There's a TryServer build with the patch backed out here if anyone needs it to check for regressions: https://build.mozilla.org/tryserver-builds/2008-04-27_15:[e-mail]-cpearce-backout-386782/

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

So far the checkin has caused 7 regressions. I'll post my fix here, as it changes the approach slightly. Reopening.

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Regression fix patch v1

This patch builds on the patch PeterV posted in bug 430628 - thanks Peter!

Part of the problem is that we need to detach (or tear down) the editor in nsDocShell::FirePageHideNotification() for security reasons. The editordata may also need to be used there. nsDocShell::InternalLoad() is called before the onpagehide event is fired, so if we detach/reattach in InternalLoad(), we'll then detach again in nsDocShell::FirePageHideNotification() - detaching the editor that we'd just reattached. Also, in order to handle reloads we need mess around resetting nsSHEntry's editor data reference.

Rather than hack around this, this patch moves our detaching solely into FirePageHideNotification(), and puts our reattaching in Embed() and RestoreFromHistory() (which emulates Embed()), we don't need to do nearly as much messing around. Embed() is called around the time the new contentviewer is hooked up during loading. This means the "old" editordata is valid right up until the new content viewer is installed, and when the new content viewer is added we have the correct editor data restored - fixing those EnsureEditorData() assertion failures.

In this patch I also am more careful in how I use mOSHE and mLSHE. mLSHE is the nsSHEntry for the page being loaded, and mOSHE is for the currently open/loaded one. I got rid of nsDocShell::HasDetachedEditor(), and now call directly into mOSHE/mLSHE's HasDetachedEditor(). The "HasDetachedEditor()" assertions are now more logically correct.

This patch seems to fix all the regressions for me, though I've not been able to test the seamonkey ones, somethings wrong with my chrome dir.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #35)
> So far the checkin has caused 7 regressions. I'll post my fix here, as it
> changes the approach slightly. Reopening.

Probably too late now, but it's best to avoid reopening a bug unless its patch is backed out. Keeping track of multiple landings in one bug just leads to trouble. Adding a reference to this bug in a new bug's comment would have been sufficient to indicate the relationship, I think.

Comment 38

[Boris Zbarsky [:bzbarsky]]

nsISHEntry needs an iid rev, right?

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

BZ: thanks, any other comments you have would be welcome.

QA: It would be cool if we could we get this latest patch ("Regression fix patch v1") thrashed, TryServer builds with the patch will appear here in the next few minutes:

https://build.mozilla.org/tryserver-builds/2008-04-29_11:[e-mail]-386782-regression_fix.v1.wip3.patch/

Thanks guys!

Comment 40

[Martijn Wargers (dead)]

Chris'link doesn't have windows builds (yet?). I accidentally also uploaded the patch to the tryserver, builds here (also with windows):
https://build.mozilla.org/tryserver-builds/2008-04-29_12:[e-mail]-1209495543/

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

After being advised by Roc and Gavin, I'll split the regression fix patch up into sub-patches that fix the individual bugs, and make tests for the relevant crash/bug. That way the only checkin in this bug will be for patch "V6.1 - merge conflicts resolved".

I think I need to change the IID for nsIDocShell as well as nsISHEntry, as v6.1 patch added a function to that interface as well. I will address that in a sub patch.

Comment 42

[Johnny Stenback (:jst)]

Comment on attachment 318093 [details] [diff] [review]
Regression fix patch v1

- In nsDocShell::DetachEditorFromWindow():

 {
-    DetachEditorFromWindow(mOSHE);
+    if (mEditorData && mOSHE) {
+        DetachEditorFromWindow(mOSHE);

The DetachEditorFromWindow() you're calling here also does the if (mEditorData) check, so no need to do that here too.

sr=jst with that, and what bz said. But I'd still love to have peterv look at this too.

Comment 43

[Bob Clary [:bc] (inactive)]

(In reply to comment #36)
> Created an attachment (id=318093) [details]
> Regression fix patch v1
> 

I tried this patch on centos5 x86_64 on the mochikit and chrome mochikit tests.

The regular mochikit tests did not complete and it appears that the test case in bug 430615 fired 

ASSERTION: mEditorFlags should not be 0: 'mEditorFlags != 0', file 
mozilla/editor/composer/src/nsEditingSession.cpp, line 1316

I also saw a lot of leaks however I don't have a baseline run of the tests to compare against. I'll work on that next.

Comment 44

[Carsten Book [:Tomcat]]

(In reply to comment #39)
> BZ: thanks, any other comments you have would be welcome.
> 
> QA: It would be cool if we could we get this latest patch ("Regression fix
> patch v1") thrashed, TryServer builds with the patch will appear here in the
> next few minutes:

Hi Chris,
tested the Tryserver builds with XP, Vista and Linux CentOS so far (Mac will also follow now) and tested the navigation buttons and also the testcase and also the regression bugs like Bug 431157 and Bug 430997 and works fine and as expected. 

Comment 45

[John Mellor (Jomel)]

This appears to have caused bug 431116 - a reproducible crash when using the TinyMCE editor.

Comment 46

[Carsten Book [:Tomcat]]

(In reply to comment #45)
> This appears to have caused bug 431116 - a reproducible crash when using the
> TinyMCE editor.
> 

The Tryserver Build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042912 Minefield/3.0pre) also fix this crash for me.

Comment 47

[Peter Van der Beken [:peterv]]

There's still an issue with this patch:

1) go to http://www.mozilla.org/editor/midasdemo/
2) enter some text
3) go to a different page (eg. www.google.com)
4) go back
5) select the text you entered in 2
6) click the 'bold' button

Notice that the text doesn't become bold.

Moving the call to ReattachEditorToWindow from nsDocshell::RestoreFromHistory to nsDocShell::FinishRestore seems to fix it. Chris: why did you change that part of my patch for bug 430628?

Comment 48

[Peter Van der Beken [:peterv]]

Comment on attachment 318093 [details] [diff] [review]
Regression fix patch v1

>Index: docshell/base/nsDocShell.cpp
>===================================================================

> nsDocShell::DetachEditorFromWindow(nsISHEntry *aSHEntry)

>+    NS_ASSERTION(aSHEntry || !aSHEntry->HasDetachedEditor(),
>+                 "Detaching editor when it's already detached.");

This seems wrong, you'll call HasDetachedEditor on a null pointer. I think you want !aSHEntry

>@@ -9002,19 +8977,25 @@ nsDocShell::EnsureScriptEnvironment()

>+    NS_ASSERTION(!openDocHasDetachedEditor,
>+      "EnsureEditorData() called when open doc has detached editor.\n");

Line this up correctly and remove the \n

>Index: editor/composer/src/nsEditingSession.cpp
>===================================================================

Let's undo the changes to the progress listener implementation here if they're unneeded (have to be sure though).

Comment 49

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 318093 [details] [diff] [review]
Regression fix patch v1

This patch is continued in bug 430624.

Comment 50

[Peter Van der Beken [:peterv]]

Marking this fixed again, this actually was fixed (comment 32). Regressions should and will be dealt with in the regression bugs.

Comment 51

[Carsten Book [:Tomcat]]

verified fixed using the steps to reproduce/testcases and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre

Comment 52

[Henri Sivonen (:hsivonen)]

(This test case fails with html5.enable=true; bug 540574.)

What's the significance of the contentEditable test having a "<br>" in the expected values but the designMode test not having it?

Does the old parser generate an extra <br> in the contentEditable case?