488094 - nsIHTMLEditor refuses to set paragraph mode, freezes

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Eyal Rozenberg]

With Thunderbird 3b1 (possibly alpha versions as well), I'm unable to do my
editor.setParagraphFormat("p");
(which my extension, BiDi Mail UI does). I get:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHTMLEditor.setParagraphFormat]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://bidimailpack/content/bidimailpack-composer.js :: InsertParagraph :: line 1084"  data: no]

why is that?

To reproduce: Install BiDi Mail UI 0.9.3 (pre-release) from here:
https://www.mozdev.org/bugs/attachment.cgi?id=5791
compose a new HTML message. Switch to paragraph mode (why can't I make the default be paragraph mode anyway?). Type some text. Press Enter. That should do it; otherwise type some more text and press Enter twice.

Comment 1

[Mark Banner (:standard8)]

nsIHTMLEditor is actually a core component. Therefore moving to the relevant bugzilla component.

Comment 2

[Mark Banner (:standard8)]

Eyal, presumably your extension works on TB 2.x? Which platform(s) do you see this on?

I'm not sure if this should block or not, but seems to warrant some investigation as to a possible regression or not.

Comment 3

[[e-mail]]

The Paragraph menuitem (Format - Paragraph - Paragraph) calls doStatefulCommand('cmd_paragraphState', 'p') which calls nsParagraphStateCommand::SetState('p') which calls SetParagraphFormat('p'), so any breakage should be reproducible without your extension... always assuming your extension doesn't do anything weird with the editor.

Comment 4

[Eyal Rozenberg]

This does indeed only happen with my extension installed. Here's what it does (when I pref this code off, bug does not manifest):

http://pastebin.mozilla.org/643507

This is a transition to a new paragraph when the user presses enter. Still, I shouldn't be getting a failure setting the paragraph state.

Comment 5

[[e-mail]]

It's definitely an editor bug; I can now reproduce the exception quite easily, and it seems to be caused by trying to delete the same <br> node twice. As WillMakeBasicBlock hasn't changed in years I don't know why its behaviour might have changed, so I'm hoping Graeme might have an idea ;-)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do you have a regression range?

Are there steps to reproduce that can work in a Gecko-based browser (and which could, e.g., be put in a mochitest)?

Comment 7

[[e-mail]]

(In reply to comment #6)
> Are there steps to reproduce that can work in a Gecko-based browser (and which
> could, e.g., be put in a mochitest)?
These steps work for me in SeaMonkey Composer; presumably there's a way to duplicate them in Midas but I don't know how to program Midas.

1. Open a new Composer window
2. Hit Enter
3. Format - Paragraph - Paragraph

Although the Composer code eats the exception, Venkman can log it and you can also view the source to demonstrate that the paragraph was not created.

> Do you have a regression range?
A fairly old SeaMonkey (1.9a5pre/20070619) does not have the bug.

Comment 8

[Magnus Melin [:mkmelin]]

FWIW, for those STR in tb I get
Error: [Exception... "'JavaScript component does not have a method named: "handleEvent"' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "<unknown>"  data: no]

Comment 9

[Eyal Rozenberg]

First regressing nightly is 2007-06-29. Weird.

Comment 10

[Eyal Rozenberg]

Um, I was checking Seamonkey nightlies, of course.

Comment 11

[Magnus Melin [:mkmelin]]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-06-28&maxdate=2007-06-29+04%3A00%3A00&cvsroot=%2Fcvsroot

Would have been easy to blame bug 386009 but the times doesn't seem to match for when it was in, out, and in again.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that:
 (1) this seems to have regressed prior to 1.9
 (2) we don't yet have an accurate regression range
it's pretty hard to call this a 1.9.1 blocker, especially at this late stage, so marking wanted.

Comment 13

[Eyal Rozenberg]

dbaron: How about 1.9.2 then?

Comment 14

[Graeme McCutcheon [:graememcc]]

>It's definitely an editor bug; I can now reproduce the exception quite easily,
>and it seems to be caused by trying to delete the same <br> node twice. As
>WillMakeBasicBlock hasn't changed in years I don't know why its behaviour might
>have changed, so I'm hoping Graeme might have an idea ;-)

You're grossly overestimating my knowledge of editor code!

But did some digging anyway. Yeah, the problem does seem to be that the placeholder <br> at the start of the line where the block is being gets inserted has two attempts to delete it.

The real action seems to be in nsHTMLEditRules::WillMakeBasicBlock. First, we seem to be hitting the code after

   // consume a br, if needed

which deletes the placeholder br at the start of the line. All well and good. 

Next the code wants tries to delete the nodes in arrayOfNodes. In this case, the only node in that array is the placeholder br we've already deleted, so we fail.

We fail because bug 237964 (landed 26th June 07) introduced a check into nsHTMLEditor::DeleteNode that returns NS_ERROR_FAILURE if the node we're trying to delete isn't modifiable. (The isModifiable check ultimately leads to a call of nsINode::IsEditable() - which returns true the first time we try to delete the br - it's contained in an editable doc - and false the second, because it's not contained in any doc). Before bug 237964, no such check took place, and the delete transaction would proceed (...and work out that the node has no parent, and thus do nothing).

So, technically a regression from bug 237964, but only in as much as the sensible code landed in that bug exposed the silliness already in the codebase.

The hack fix would be to compare the node we're trying to delete to the br node we've already deleted. Not sure what the _correct_ fix is...

Comment 15

[[e-mail]]

Attachment: Safe fix

Comment 16

[[e-mail]]

Comment on attachment 377879 [details] [diff] [review]
Safe fix

We need to fix this regression on the 1.9.1 branch at some point. Requesting approval now since we can't even bake it on trunk first otherwise.

Comment 17

[Eyal Rozenberg]

Where's the bug about not using dummy br nodes in the first place? :-(

Comment 18

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

(no longer need approval to land on trunk)

Comment 19

[[e-mail]]

Pushed changeset 7fc7e27ed155 to mozilla-central.

Comment 20

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

Comment on attachment 377879 [details] [diff] [review]
Safe fix

We're closed for 1.9.1 patches, but can re-evaluate this when it comes time to look at 1.9.1.1

Comment 21

[Samuel Sidler (old account; do not CC)]

Not blocking 1.9.1.1, but I'll approve the patch.

Comment 22

[Samuel Sidler (old account; do not CC)]

Comment on attachment 377879 [details] [diff] [review]
Safe fix

Approved for 1.9.1.1. a=ss

Please land as soon as you can if you intend to get this non-blocker into 1.9.1.1.

Comment 23

[[e-mail]]

Pushed changeset a5b258c15577 to releases/mozilla-1.9.1

Comment 24

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

Eyal/Peter: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better yet the 3.5.1 release candidate: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/

Comment 25

[juan becerra [:juanb]]

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1pre) Gecko/20090716 Shredder/3.0b4pre

First I checked in Tb3(beta2) and tried the steps to reproduce the problem. The exception in the console was the same. Then I tried in the nightly, and there was no error in the console.