283897 - vBulletin 3.0.1 WYSIWYG Post Reply Function is Broken

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

I noticed that in one of the recent nightlies (within the last week), that the
WYSIWYG editor for vBulletin 3.0.1 no longer works. It loads up, but you can't
actually click inside the text editor to type a message. When I open up the
Javascript console, I see this error message:
-------------------------------
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.queryCommandState]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
http://www.hardforum.com/clientscript/vbulletin_wysiwyg.js :: set_context ::
line 439"  data: no]
Source File: http://www.hardforum.com/clientscript/vbulletin_wysiwyg.js
Line: 439
-------------------------------
I'll see if I can track down which nightly this broke with and reply to the bug.
This is obviously a big issue as there are a lot of vBulletin message boards out
there.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

The WYSIWYG editor broke with the February 24 nightly.

As wgianopoulos pointed out in #developers, this is probably a leftover bug from
the checkins for bug 209020.

Comment 2

[Johnny Stenback (:jst)]

Some changes to the editor went in on the 25th (Friday), could you please test
with a fresh nightly. FYI, I registerd on the page (moz-tester, moz-tester), and
I'm able to compose a private message at least, which appears to be all I can do
with that account (as far as writing messages goes).

Did I miss something?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Still happens here using a build I made off the trunk this morning. And for me,
the Private Message editor throws the same exception.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050228
Firefox/1.0+

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ryan, is there a specific page that I could reproduce the bug on?  Comment 0
doesn't say...

Comment 5

[Ryan VanderMeulen [:RyanVM]]

John - you weren't having problems because the forum defaults to the standard
non-WYSIWYG editor by default. I logged in on that account and changed it in the
board preferences to the WYSIWYG editor, so try now.

Boris - Use the login that jst created and open up any thread. Click the post
reply button and try to type in the main message field.

Comment 6

[Johnny Stenback (:jst)]

Ok, now I see the problem. I'm not in a position where I can debug this right
now, but my guess would be that the site enables midas on a document in an empty
iframe before the document (about:blank) in the empty iframe is done loading, so
we create a new document with midas disabled in it...

Comment 7

[Johnny Stenback (:jst)]

Turns out that this wasn't a timing issue related to about:blank at all. The
problem here is that the site turns on designMode, and after that does a
document.open(), write(), and close(). That tears down a lot of the document,
including all event listeners, which the editor relies on heavily.

I've spent a good part of today figuring out a way to fix this in our current
codebase w/o rewriting a lot of code, and I've got a patch. Attaching...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Re-initialize midas in document.open(), and make the editor deal with no body node.

This does what's described above, cleans up some code, and eliminates some
duplicated code.

Comment 9

[Johnny Stenback (:jst)]

Attachment: diff -w of the above for review.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175985 [details] [diff] [review]
diff -w of the above for review.

>Index: editor/libeditor/base/nsEditor.cpp
>+already_AddRefed<nsIDOMEventReceiver>
>+nsEditor::GetDOMEventReceiver()
>+  // Now hack to make sure we are not anonymous content.
>+  // If we are grab the parent of root element for our observer.

Hmm... I guess you just moved this hack, eh?

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>@@ -270,23 +272,18 @@ NS_IMETHODIMP nsHTMLEditor::Init(nsIDOMD
> 
>-    nsCOMPtr<nsIDOMElement> bodyElement;
>-    result = nsEditor::GetRootElement(getter_AddRefs(bodyElement));
>-    if (NS_FAILED(result)) { return result; }
>-    if (!bodyElement) { return NS_ERROR_NULL_POINTER; }

This change scares me.	There are various places in editor that loop up the
content tree until they hit a body node (as checked via QI, iirc)...  I'm not
sure they function correctly (or even avoid crashing) when there is no body
node.

>-void nsHTMLEditor::HandleEventListenerError()
>+void
>+nsHTMLEditor::RemoveEventListeners()

There are other callers of HandleEventListenerError() that you haven't changed
(even in this patch; see nsHTMLAbsPosition.cpp, nsHTMLObjectResizer.cpp, etc). 
Why don't they need to call RemoveEventListeners?  What method _are_ they
ending up calling?


>+  // register the event listeners with the DOM event reveiver

"unregister", right?

Comment 11

[Kathleen :Brade]

Comment on attachment 175985 [details] [diff] [review]
diff -w of the above for review.

r-

Like bz pointed out, the editor has other places in the code which rely on
their being a body element.  Removing that check in Init() would be very bad. 
If we want to go that route for the long run, let's do it as part of a new bug
and not part of this fix.

Comment 12

[Johnny Stenback (:jst)]

(In reply to comment #11)
> (From update of attachment 175985 [details] [diff] [review] [edit])
> r-
> 
> Like bz pointed out, the editor has other places in the code which rely on
> their being a body element.  Removing that check in Init() would be very bad. 
> If we want to go that route for the long run, let's do it as part of a new bug
> and not part of this fix.
> 

We don't have a choise here, really. If the editor is to survive the
document.open/write/close() case, it *must* be able to deal with there not being
a body node around, or we'd need to make one up in the content code and deal
with all the problems that'll introduce in the sinks etc.

I'll try to go through the code and find the places where the body is needed. I
don't care so much if the editor doesn't work if there's no body, but it can't
crash, of course.

Comment 13

[[e-mail]]

Is there any mileage in reinitializing the editor in document.close()?

Comment 14

[Boris Zbarsky [:bzbarsky]]

jst, if you do go through this "looking for <body> and <html>" stuff in editor
(and we need to deal with lack of the latter too, no?), see bug 279332.  That
would probably need to be fixed in the process.

Comment 15

[Johnny Stenback (:jst)]

(In reply to comment #14)
> jst, if you do go through this "looking for <body> and <html>" stuff in editor
> (and we need to deal with lack of the latter too, no?), see bug 279332.  That
> would probably need to be fixed in the process.

There's really no case that I can think of when there won't be a html tag in a
document, not even in the case where you document.open() and immediately
document.close() it, or even if you don't. We don't throw away the document
element, and that's due to bug 55334 (or 78070, really). So until that's fiexed,
we'll never be in a situation where there's no document element.

Having said that, even if we were, I don't see any code any more in the editor
after my cleanup patch (pending) that makes the editor *crash* due to the lack
of a document element, and that's really all I care about in this case. If we
really care at some later point we could add code to the editor to deal with
that too. Note that the editor's notion of "body" element doesn't always mean
the <body> element, what it really is is the root element for which the editor
is used (i.e. the form control or whatever in a case where we're dealing with an
editor for a text input). So I've renamed that to "root" instead, not to be
confused with the document element, often refered to as the root element...

Joy. This is a mess, but I think it's getting better. I'll attach the cleanup
part of this patch as a separate patch, once that's in, the actual fix for this
bug should be much easier :)

Comment 16

[Johnny Stenback (:jst)]

Attachment: Editor "body" cleanup diff

Comment 17

[Johnny Stenback (:jst)]

Attachment: Editor "body" cleanup diff -w for reviews

This patch cleans up a bunch of "body" related code in the editor. No real big
changes, really, but it changes the notion of "body" to "root", as the editor's
"body" really wasn't always the body tag, it's simply the root of the tree that
is being edited. This also removes some duplicated code related to event
listener creation and registration, and makes the editor properly clean up its
even listeners.

I'd like to get this in, let it sit for a day or so, and then do what's needed
to actually fix this bug.

Oh, and this change also fixes bug 279332, per bz's suggestion.

Comment 18

[Simon Fraser [no longer active]]

The patch seems OK to me. I would like to see a comment in the headers above
methods that return raw pointers:
+  nsIDOMNode *GetBody();
to say that these are not addreffed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176161 [details] [diff] [review]
Editor "body" cleanup diff -w for reviews

>Index: editor/libeditor/base/nsEditor.cpp
>+nsEditor::InstallEventListeners()

This function assumes the various m*Listener pointers are non-null, right? 
Toss in some NS_PRECONDITIONS to that effect?

>+  if (NS_FAILED(rv)) {
>+    NS_ERROR("failed to register mouse listener");

Or some other listener...  How about s/mouse/some event/ ?

>+nsEditor::FindRootElement()
>+{
>+  if (!mRootElement) {

Don't we already check this in GetRoot()?  And only call GetRoot() from this
method?

It just seems like this has one level of indirection too many; why not just
call GetRootElement() from GetRoot() and skip FindRootElement() altogether?

> nsEditor::GetDocumentIsEmpty(PRBool *aDocumentIsEmpty)
> {
>+  // XXX: Shouldn't that be !firstChild???
>   *aDocumentIsEmpty = NS_SUCCEEDED(res) && firstChild;

I would think it should.... We have code in the UI calling this, but all of it
manages not to depend on the actual return value.  Moreover, this method has
never returned anything useful (used to always return true, before it was
changed to return the exact opposite of reality).  Just fix this?  Also,
wouldn't it make more sense to call hasChildNodes() on rootElement here?

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>     do {
>+      tmp.swap(blockParent);
>       res = tmp->GetParentNode(getter_AddRefs(blockParent));
>+    } while ( aOutColor.EqualsLiteral("transparent") && blockParent);
>+    if (!tmp && aOutColor.EqualsLiteral("transparent")) {

I think this is still wrong.  Since we broke out of the loop, either aOutColor
is not transparent, or blockParent is null.  In the former case, we can't get
into the if since aOutColor is not transparent.  In the latter case, we can't
have tmp null, unless blockParent was null up front (otherwise tmp will be the
rootmost ancestor of the original blockParent).

I think the right check here is just the check for aOutColor being transparent;
if it is, and we left the loop, that indicates that blockParent was null, so we
hit the root of the tree.

>Index: editor/libeditor/html/nsHTMLEditor.h

>-  NS_IMETHOD InstallEventListeners();

I'm a little confused by this.	Did all the listener members move up to
nsEditor then, or something?  I see listener members left on nsHTMLEditor.... 
If the only impl of InstallEventListeners is on nsEditor, how are listeners
specific to subclasses being handled?

Same for plaintext editor.

The rest looks ok...

Comment 20

[Johnny Stenback (:jst)]

(In reply to comment #19)
[...]
> >-  NS_IMETHOD InstallEventListeners();
> 
> I'm a little confused by this.	Did all the listener members move up to
> nsEditor then, or something?  I see listener members left on nsHTMLEditor.... 
> If the only impl of InstallEventListeners is on nsEditor, how are listeners
> specific to subclasses being handled?

The event members have always been in nsEditor (well, I didn't move them, at
least), but the code to create them and add them as listeners have been
duplicated (well, mostly) in the html and plaintext editors, so I simply moved
what was duplicated to nsEditor (the editors create slightly different
listeners, but the registration code is identical). The listener members that
are left in the html editor are created, registerd, unregisterd, and destroyed
on demand, so that code is spread throughout the code that uses them.

I'll attach a cleaned up diff that diff that fixes the rest.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Fix bz's and sfraser's issues (diff -w).

This fixes all of what's been pointed out so far.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176731 [details] [diff] [review]
Fix bz's and sfraser's issues (diff -w).

This diff is identical (except for dates) to the first "body cleanup -w"
diff....

Comment 23

[Johnny Stenback (:jst)]

Oh, duh. Must have diffed the wrong tree or something.

Comment 24

[Johnny Stenback (:jst)]

Attachment: Fix bz's and sfraser's issues (diff -w, right tree this time).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176885 [details] [diff] [review]
Fix bz's and sfraser's issues (diff -w, right tree this time).

>Index: editor/libeditor/base/nsEditor.cpp
> nsEditor::GetDocumentIsEmpty(PRBool *aDocumentIsEmpty)

>+  PRBool hasChildNodes;
>+  nsresult res = rootElement->HasChildNodes(aDocumentIsEmpty);
>+
>+  *aDocumentIsEmpty = !hasChildNodes;

You want to pass &hasChildNodes to HasChildNodes()...  What you wrote returns a
random value from this method.

>+  // Use the documents body element as the editor root if we didn't

"document's"

sr=bzbarsky with those fixed.

Comment 26

[Kathleen :Brade]

Comment on attachment 176885 [details] [diff] [review]
Fix bz's and sfraser's issues (diff -w, right tree this time).

r=brade

Please try to follow the style for braces in each file.  Originally the editor
source files used curly braces on its own line but now the style has been
contaminated and its getting harder to read.  Please don't make it worse; stick
with the original style when possible.

if (a)
{
  doB();
  doC();
}


Here is one nit you could clean up since you are changing this line even though
you didn't introduce it:

* in nsTextEditUtils::InBody, add a space before the !	in "p!="


Thanks!

Comment 27

[Johnny Stenback (:jst)]

*** Bug 286834 has been marked as a duplicate of this bug. ***

Comment 28

[Johnny Stenback (:jst)]

Attachment: Actual fix for this bug (same as the early patches in this bug, but w/o the cleanup that already landed)

Comment 29

[Kathleen :Brade]

Comment on attachment 179714 [details] [diff] [review]
Actual fix for this bug (same as the early patches in this bug, but w/o the cleanup that already landed)

r=brade

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179714 [details] [diff] [review]
Actual fix for this bug (same as the early patches in this bug, but w/o the cleanup that already landed)

sr=bzbarsky

Comment 31

[Christian Stadler]

mmh ... shouldn't this be a blocker for Bug #209020? Or perheps 'depends on'?

Comment 32

[Voyager]

Seems like the actual fix for this bug has been in for four days at least. 
What's keeping the fix from landing into the nightlies?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Nothing.  In fact, had you bothered to check, you would have noticed that it was
checked into the nightlies on 2005-04-05 at 19:22 Pacific time.

If your question is what's keeping this bug from being resolved fixed, that's a
separate issue.  I'm betting it's the fact that jst has spent all his time the
last few days working on the branch security updates.  But maybe he has more
followup work in mind for this bug, so he should be the one to resolve it
(probably once the security releases are done).

Comment 34

[Johnny Stenback (:jst)]

Marking FIXED. Sorry for not doing so earlier (yeah, been busy with security
work etc).

Comment 35

[Boris Zbarsky [:bzbarsky]]

This caused bug 328331