629845 - Rich text editor on Lang-8 does not apply text effects, returns Javascript error Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[chiklit]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

When using the correction editor on Lang-8.com and trying to apply text styles the styles do not get applied and Javascript returns this error: 

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://lang-8.com/static/javascripts/all.js?r091120a :: doRichEditCommand :: line 8976" data: no]

This worked as of Firefox 4.0b7 and has been broken since Firefox 4.0b8. The exact regression range seems to be between 2010-11-10 and 2010-11-11 in the nightlies of 4.0b8pre. It works on the nightly from Nov 10th and doesn't work on Nov 11th. Hopefully that's specific enough to pinpoint something.

Reproducible: Always

Steps to Reproduce:
(Unfortunately I think you need to sign up for Lang-8 to try the correction features, but I'll keep an eye out if this bug appears elsewhere.)
1. Go a post on Lang-8.
2. Attempted to correct one of the sentences.
3. Select text to correct.
4. Try to use one of the style options (Red, Blue, Bold, Crossout, etc.)
Actual Results:  
Nothing happens, javascript error is generated.

Expected Results:  
Style should be applied to textfield and saved when finished editing.

Comment 1

[chiklit]

I've created a Lang-8 account for anyone who wants to test this bug you can log in with the following:

E-mail: [e-mail]
Password: bugzilla

And then go to any of the entries on the site to test.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Repro'd with Mozilla/5.0 (Windows NT 5.1; rv:2.0b11pre) Gecko/20110128 Firefox/4.0b11pre ID:20110128120226.

Range from Comment 0 would be:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df1d1ff6b489&tochange=0f17e5f1eb01 what contains e.g. Bug 602838.

Another TE Bug?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Leaving this nominated for now. I'll take this to bisect a smaller regression range.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, marking it blocking to figure out what's going on here. Though once we know that, the fix itself might not be blocking.

Comment 5

[Johnny Stenback (:jst)]

Mounir, could you look into this? If not, let me know...

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> I've created a Lang-8 account for anyone who wants to test this bug you can log
> in with the following:
> 
> E-mail: [e-mail]
> Password: bugzilla
> 
> And then go to any of the entries on the site to test.

Thank you very much for creating this account, it's really helpful! :)

(In reply to comment #5)
> Mounir, could you look into this? If not, let me know...

I've been able to reproduce this locally. I will do a regression range check tomorrow morning.

Comment 7

[Mounir Lamouri (:mounir)]

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df1d1ff6b489&tochange=0f17e5f1eb01

I'm going to bisect that.

Comment 8

[Mounir Lamouri (:mounir)]

Part 4 of bug 551704 is making this failing:
http://hg.mozilla.org/mozilla-central/rev/c922552a8dbc

It looks like nsHTMLDocument::EditingStateChanged is called twice when we try to edit the comment. The second time, root is null so it's failing. Then, the document isn't recognized as an editable document and execCommand fails.
A trivial fix consists of ignoring a failure in editor->BeginningOfDocument(). Actually, it seems a bit hard to make the document not editable if we can't select the beginning. However, I don't know if that shouldn't happen (IOW, do we consider that a failure in BeginningOfDocument should never happen in a not-messed-up situation?).

Ehsan, let me know if you have any idea? Feel free to take this bug too if you have free cycles (I've some so I can work on it).

Comment 9

[(no longer active)]

So, if the second EditingStateChanged call is coming from document.writeln, it should be disabling editing, right?  And there should be a third call following that (from nsHTMLDocuemnt::EndLoad) to re-enable it again.  And the editor object is going to be different on the third call.

My suspicion is that the editor fails to initialize correctly when we try to initialize it from EndLoad...

Comment 10

[(no longer active)]

Was comment 9 helpful?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> So, if the second EditingStateChanged call is coming from document.writeln, it
> should be disabling editing, right?  And there should be a third call following
> that (from nsHTMLDocuemnt::EndLoad) to re-enable it again.  And the editor
> object is going to be different on the third call.

There are only two calls.

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Testcase

This is actually not related to document.write at all.
Ehsan, I'm wondering how this issue didn't show up more often. Is there something not usual in this testcase? (it's more or less what Lang-8 is doing).

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Testcase

Interesting, actually the previous testcase was too minimal. The new testcase is really failing and show more clearly what is making it fail:
frame.contentWindow.document.writeln("<head><meta http-equiv='Content-Type' content='text/html; charset=UTF-8'></head><body></body>");

Without this line, the test pass.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> (In reply to comment #9)
> > So, if the second EditingStateChanged call is coming from document.writeln, it
> > should be disabling editing, right?  And there should be a third call following
> > that (from nsHTMLDocuemnt::EndLoad) to re-enable it again.  And the editor
> > object is going to be different on the third call.
> 
> There are only two calls.

My bad. There is actually a third call on nsHTMLDocument::EndLoad but it's failing (yay!) here:
3253      nsCOMPtr<nsIEditingSession> editSession = do_GetInterface(docshell, &rv);
3254      NS_ENSURE_SUCCESS(rv, rv);

My breakpoint wasn't on the call but somewhere inside the method that's why I didn't catch it.

Comment 15

[(no longer active)]

(In reply to comment #13)
> Created attachment 511071 [details]
> Testcase
> 
> Interesting, actually the previous testcase was too minimal. The new testcase
> is really failing and show more clearly what is making it fail:
> frame.contentWindow.document.writeln("<head><meta http-equiv='Content-Type'
> content='text/html; charset=UTF-8'></head><body></body>");
> 
> Without this line, the test pass.

This is definitely a regression from 3.6...

Comment 16

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > So, if the second EditingStateChanged call is coming from document.writeln, it
> > > should be disabling editing, right?  And there should be a third call following
> > > that (from nsHTMLDocuemnt::EndLoad) to re-enable it again.  And the editor
> > > object is going to be different on the third call.
> > 
> > There are only two calls.
> 
> My bad. There is actually a third call on nsHTMLDocument::EndLoad but it's
> failing (yay!) here:
> 3253      nsCOMPtr<nsIEditingSession> editSession = do_GetInterface(docshell,
> &rv);
> 3254      NS_ENSURE_SUCCESS(rv, rv);
> 
> My breakpoint wasn't on the call but somewhere inside the method that's why I
> didn't catch it.

Hmm, if you set a breakpoint on nsEditingSession::TearDownEditorOnWindow, does that get hit?

Comment 17

[(no longer active)]

Or actually, nsEditingSession::MakeWindowEditable?

Comment 18

[Mounir Lamouri (:mounir)]

Actually, if you open the test case you will see that the document doesn't step loading because .close() isn't called on the frame's document. It happens that calling .close() solves the issue. The same way, if you stop the page load (with ESC key for example), it will make the testcase working.

I will try to understand why but whatever the reason is, it seems hard to have a safe testcase [1] written to check that.

[1] without hazardous setTimeout().

Comment 19

[Mounir Lamouri (:mounir)]

When .close() is called, EndLoad() is called on the document and the editor is initialized again and this time it's working because it is correctly initialized (it has a root element).

By the way, the issue is bigger than just not being able to call execCommand. The caret goes completely crazy too (it's not updated while you type).

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

According to my discussion with Ehsan on IRC, this is the minimal fix because we shouldn't make nsHTMLDocument::EditingStateChanged fails if BeginningOfDocument fails.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: GetRoot() failing should return NS_OK

GetRoot() calls GetRootElement() which tries to get the body element if mRootElement isn't set. We could have no body element so we shouldn't fail on that.

Comment 22

[(no longer active)]

(In reply to comment #19)
> When .close() is called, EndLoad() is called on the document and the editor is
> initialized again and this time it's working because it is correctly
> initialized (it has a root element).
> 
> By the way, the issue is bigger than just not being able to call execCommand.
> The caret goes completely crazy too (it's not updated while you type).

Could the caret craziness you're seeing be bug 632215?  Can you apply and test the patch on that bug to see if it fixes things?

Comment 23

[(no longer active)]

Comment on attachment 511337 [details] [diff] [review]
Patch v1

r=me.

Comment 24

[(no longer active)]

Comment on attachment 511337 [details] [diff] [review]
Patch v1

Nit: could you please keep editor/libeditor/html/tests/Makefile.in sorted?  Thanks!

Comment 25

[(no longer active)]

Comment on attachment 511338 [details] [diff] [review]
GetRoot() failing should return NS_OK

Please add an NS_WARNING if rootElement is none, just to see something on the console.  r=me with that.

Comment 26

[Mounir Lamouri (:mounir)]

Ehsan, is a r+ from you enough for editor patches, nowadays?

Comment 27

[(no longer active)]

(In reply to comment #26)
> Ehsan, is a r+ from you enough for editor patches, nowadays?

I think so, yes!  :-)

Comment 28

[Mounir Lamouri (:mounir)]

(In reply to comment #22)
> (In reply to comment #19)
> > When .close() is called, EndLoad() is called on the document and the editor is
> > initialized again and this time it's working because it is correctly
> > initialized (it has a root element).
> > 
> > By the way, the issue is bigger than just not being able to call execCommand.
> > The caret goes completely crazy too (it's not updated while you type).
> 
> Could the caret craziness you're seeing be bug 632215?  Can you apply and test
> the patch on that bug to see if it fixes things?

The first test doesn't pass but the second does when bug 632215 is reverted and only this patch is applied.
The test from this bug fails with only patch from bug 632215 is applied.

Comment 29

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/206db5f72ebb
http://hg.mozilla.org/mozilla-central/rev/e8477acc9c3d