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.

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.

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?

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+

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

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.

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...

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...

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

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 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.

(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.

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

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.

(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 :)

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.

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 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...

(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.

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

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....

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

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 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!

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

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 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

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

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?

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).

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

This caused bug 328331