635420 - Clicking on content within Normal tab of Composer and stops switching to other tabs

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Ian Neal]

Steps to reproduce:
1/ Open SeaMonkey (builds after and including buildID 20101121005024)
2/ Go to http://www.seamonkey-project.org/start/
3/ Open Error Console
4/ Select Edit Page from File Menu
5/ Click on content within Normal tab window
6/ Look in error console

Expected result
1/ No errors

Actual result
1/ Following message:
Error: An error occurred updating the cmd_updateStructToolbar command: [Exception... "'[JavaScript Error: "element is null" {file: "chrome://editor/content/editor.js" line: 3245}]' when calling method: [nsIControllerCommand::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 80"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 86

2/ Clicking on another tab within composer gives:
Error: An error occurred executing the cmd_AllTagsMode command: [Exception... "'[JavaScript Error: "element is null" {file: "chrome://editor/content/editor.js" line: 3245}]' when calling method: [nsIControllerCommand::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

3/ If you close Composer after this has happened, seems to generate crash but might not be related.

Regression range for mozilla-central is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=baa6cc2f72e4&tochange=baa51e6d4a15

Comment 1

[Ian Neal]

Backing out patches from bug 612447 (and bug 633738 to allow the other back out), fixes the issue. Can't add bug 612447 as dependency due to it being a security bug.

Comment 2

[(no longer active)]

I'm not really convinced that this is actually a problem in the editor.  I think SeaMonkey just fails to handle the fact that the bogus element is now only in empty contenteditable documents (see bug 611182), and the js code just fails because it doesn't do proper error checking.

In this loop here: <http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#3244> we never check to make sure that we're not getting past the docuemnt root element, and I think that's why |element| ends up being null.

Someone should investigate this on the SeaMonkey side, and let me know if something is really broken in core.

Comment 3

[[e-mail]]

GetBodyElement() is returning the old body element from the about:blank page.

Comment 4

[[e-mail]]

[GetBodyElement() is a thin wrapper around GetCurrentEditor().rootElement]

Comment 5

[(no longer active)]

Neil, can you please bisect to find the exact regression changeset?  If this is really a problem in core, it's worrying...

Comment 6

[[e-mail]]

Changeset 91980a82eeae works, changeset 406748270073 doesn't.
(I haven't tried changeset 64fdcad8cb11 yet but I doubt it will work.)

Comment 7

[[e-mail]]

OK, so before changeset 64fdcad8cb11 what happens is that the editor window opens, the <editor> binding creates an editor, then editor.js loads the document to be edited, a new editor is created, the old editor is destroyed and the new one initialised, and everything works as expected.

However changeset 64fdcad8cb11 changes things so that when the document to be edited loads we get the same editor object again. It doesn't get destroyed, so it gets reinitialised unexpectedly.

Comment 8

[(no longer active)]

(In reply to comment #7)
> OK, so before changeset 64fdcad8cb11 what happens is that the editor window
> opens, the <editor> binding creates an editor, then editor.js loads the
> document to be edited, a new editor is created, the old editor is destroyed and
> the new one initialised, and everything works as expected.

How does the editor get destroyed here?  In other words, what call stack leads to nsEditor::PreDestroy getting called?

Comment 9

[[e-mail]]

(In reply to comment #8)
> what call stack leads to nsEditor::PreDestroy getting called?
nsDocShellEditorData::SetEditor
nsDocShell::SetEditor
nsEditingSession::SetupEditorOnWindow
nsEditingSession::EndDocumentLoad
nsEditingSession::OnStateChange
nsDocLoader::FireOnStateChange
nsDocLoader::doStopDocumentLoad
nsDocLoader::DocLoaderIsEmpty
nsDocLoader::OnStopRequest
[boring stuff snipped]

Before the patch, inEditor != mEditor, so the old editor gets destroyed. After the patch, inEditor == mEditor, so nothing happens.

Comment 10

[[e-mail]]

So that would be because of this change then:
>-  nsCOMPtr<nsIEditor> editor = do_CreateInstance(classString, &rv);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  // Try to reuse an existing editor
>+  nsCOMPtr<nsIEditor> editor = do_QueryReferent(mExistingEditor);
>+  if (!editor) {
>+    editor = do_CreateInstance(classString, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mExistingEditor = do_GetWeakReference(editor);
>+  }

Comment 11

[[e-mail]]

Attachment: Proposed patch

So, this patch is in two parts:
1. When reusing an editor, call PreDestroy on it, since we're about to call Init on it, and not doing so would be bad, including causing a crash if the old editor is released before the document is. (This might be bug 633123.)
2. When pre-destroying an editor, its root element is out of date, so null it.

Comment 12

[(no longer active)]

Comment on attachment 515042 [details] [diff] [review]
Proposed patch

Ah, yeah, now it all makes sense.  Thanks for debugging this, Neil.

>diff --git a/editor/composer/src/nsEditingSession.cpp b/editor/composer/src/nsEditingSession.cpp
>--- a/editor/composer/src/nsEditingSession.cpp
>+++ b/editor/composer/src/nsEditingSession.cpp
>@@ -448,17 +448,22 @@ nsEditingSession::SetupEditorOnWindow(ns
>   }
> 
>   // create and set editor
>   nsCOMPtr<nsIEditorDocShell> editorDocShell = do_QueryInterface(docShell, &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Try to reuse an existing editor
>   nsCOMPtr<nsIEditor> editor = do_QueryReferent(mExistingEditor);
>-  if (!editor) {
>+  if (editor)
>+  {
>+    editor->PreDestroy(PR_FALSE);
>+  }
>+  else
>+  {

Nit: coding styles, etc.  i.e.,

if (!editor) {
  editor->...;
} else {

r=me with that.

I didn't want to approve this patch myself, since we're so close to the release, but I really think we should take this, roc.  Do you agree?

Comment 13

[[e-mail]]

(In reply to comment #12)
> (From update of attachment 515042 [details] [diff] [review])
> Nit: coding styles, etc.
Sorry, I was following existing style, I hadn't noticed that you had started adding new code with this style.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7fbcd2458b9e