Closed Bug 629845 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: chiklit, Assigned: mounir)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [softblocker])

Attachments

(3 files, 1 obsolete file)

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.
OS: Mac OS X → All
I've created a Lang-8 account for anyone who wants to test this bug you can log in with the following:

E-mail: lang8bug@gmail.com
Password: bugzilla

And then go to any of the entries on the site to test.
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?
Keywords: regression
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
blocking2.0: --- → ?
Leaving this nominated for now. I'll take this to bisect a smaller regression range.
Assignee: nobody → jonas
Actually, marking it blocking to figure out what's going on here. Though once we know that, the fix itself might not be blocking.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Mounir, could you look into this? If not, let me know...
Assignee: jonas → mounir.lamouri
(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: lang8bug@gmail.com
> 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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hardware: x86 → All
Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df1d1ff6b489&tochange=0f17e5f1eb01

I'm going to bisect that.
Severity: major → normal
Summary: [REGRESSION] Rich text editor on Lang-8 does not apply text effects, returns Javascript error Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand] → Rich text editor on Lang-8 does not apply text effects, returns Javascript error Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]
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).
Blocks: 551704
Component: General → Editor
QA Contact: general → editor
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...
Was comment 9 helpful?
(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.
Attached file Testcase (obsolete) —
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).
Keywords: testcase
Attached file 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.
Attachment #511067 - Attachment is obsolete: true
(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.
(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...
(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?
Or actually, nsEditingSession::MakeWindowEditable?
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().
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).
Attached patch Patch v1Splinter Review
According to my discussion with Ehsan on IRC, this is the minimal fix because we shouldn't make nsHTMLDocument::EditingStateChanged fails if BeginningOfDocument fails.
Attachment #511337 - Flags: review?(ehsan)
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.
Attachment #511338 - Flags: review?(ehsan)
Whiteboard: [softblocker] → [softblocker][needs review]
(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 on attachment 511337 [details] [diff] [review]
Patch v1

r=me.
Attachment #511337 - Flags: review?(ehsan) → review+
Comment on attachment 511337 [details] [diff] [review]
Patch v1

Nit: could you please keep editor/libeditor/html/tests/Makefile.in sorted?  Thanks!
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.
Attachment #511338 - Flags: review?(ehsan) → review+
Ehsan, is a r+ from you enough for editor patches, nowadays?
Whiteboard: [softblocker][needs review] → [softblocker]
(In reply to comment #26)
> Ehsan, is a r+ from you enough for editor patches, nowadays?

I think so, yes!  :-)
Whiteboard: [softblocker] → [softblocker][passed try][needs landing]
(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.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/206db5f72ebb
http://hg.mozilla.org/mozilla-central/rev/e8477acc9c3d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [softblocker][passed try][needs landing] → [softblocker]
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.