616590 - Mail reply window stops painting.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

###!!! ASSERTION: already started a batch!: '!mRootVM', file ../../dist/include/nsIViewManager.h, line 304
WARNING: NS_ENSURE_TRUE(*aCell) failed: file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/html/nsTableEditor.cpp, line 3005
###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 410
###!!! ASSERTION: bad state: 'mUpdateCount > 0', file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/base/nsEditor.cpp, line 4184
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 5653
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040154: file /Volumes/Devel/thunderbird/src-central/mailnews/base/util/nsMsgUtils.cpp, line 135
###!!! ASSERTION: bad state: 'mUpdateCount > 0', file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/base/nsEditor.cpp, line 4184
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file /Volumes/Devel/thunderbird/src-central/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 5653
--DOMWINDOW == 33 (0x1368c4598) [serial = 35] [outer = 0x0] [url = about:blank]
###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /Volumes/Devel/thunderbird/src-central/mozilla/dom/src/threads/nsDOMWorker.cpp, line 1277
--DOMWINDOW == 32 (0x12ecbcca8) [serial = 44] [outer = 0x1372e28f0] [url = about:blank]
###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /Volumes/Devel/thunderbird/src-central/mozilla/dom/src/threads/nsDOMWorker.cpp, line 1277

(To save you a couple of keystrokes, https://developer.mozilla.org/en/Simple_Thunderbird_build )

Comment 1

[(no longer active)]

Roc, this is one of the bugs in the series of update view count mismatch problems.  It should block 2.0, IMO.  If it does, I'll provide a fix (I have to build comm-central first, but I'm pretty sure that a fix won't be too complicated.)

Comment 2

[Blake Winton (:bwinton) (:☕️)]

FWIW, I'm more than happy to help Ehsan with any problems he might have building comm-central.

Comment 3

[(no longer active)]

(In reply to comment #2)
> FWIW, I'm more than happy to help Ehsan with any problems he might have
> building comm-central.

I'll let you know if I hit any problems, thanks!  This would be a good first step for me to repro Thunderbird specific bugs and debug them locally, for sure.  :-)

Comment 4

[(no longer active)]

So here is what's happening.  I also suspect that this might be the problem behind bug 525359 (and possibly many other similar bugs as well).

Composer/mail doesn't use the designMode API to make their documents editable.  This is well and all, except for the fact that nsHTMLDocument::EditingStateChanged has no knowledge of editors set up independently, therefore we may attempt to create another editor for the same document if the page loaded inside it uses the contentEditable or designMode APIs.

I have a patch in the works which should fix it.  I'm CCing Thunderbird, SeaMonkey and BlueGriffon folks because they may want to test their code with the patch that I will attach here.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

The original problem is something which can't be tested easily in our automated suites.  This test triggers the assertion, and it relies on breaking other chrome tests in the suite to be effective, but this would also need Litmus tests for the Thunderbird bug 525359 (which is also fixed by this patch BTW).

Comment 6

[(no longer active)]

Attachment: Patch (v2)

The last abort condition was in fact too strict.  For example, this test <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/388980-1.html?force=1> hits that code path.  This version of the patch removes that final NS_ABORT.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 495296 [details] [diff] [review]
Patch (v2)

r=me

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a7adfbb051fd

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Thanks again, Ehsan!  Is using switching Tb composer to use design mode something that we should be thinking about?  If so, I'd be interested to understand more...

Comment 10

[(no longer active)]

(In reply to comment #9)
> Thanks again, Ehsan!  Is using switching Tb composer to use design mode
> something that we should be thinking about?  If so, I'd be interested to
> understand more...

Yes, I believe this is the direction towards which mailnews should be headed.  You can still get the editor object if you want to manipulate it directly, but you shouldn't need to initialize the editor without the document knowing about it.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Good to know, thanks. Adding bienvenu and protz, as I suspect they'll want to be conscious of this as well.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #5)
> Created attachment 495286 [details] [diff] [review]
> Patch (v1)
> 
> The original problem is something which can't be tested easily in our automated
> suites.  This test triggers the assertion, and it relies on breaking other
> chrome tests in the suite to be effective, but this would also need Litmus
> tests for the Thunderbird bug 525359 (which is also fixed by this patch BTW).

Ehsan, does this mean the patch isn't suitable for back-porting to 1.9.2 branch?

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #5)
> > Created attachment 495286 [details] [diff] [review]
> > Patch (v1)
> > 
> > The original problem is something which can't be tested easily in our automated
> > suites.  This test triggers the assertion, and it relies on breaking other
> > chrome tests in the suite to be effective, but this would also need Litmus
> > tests for the Thunderbird bug 525359 (which is also fixed by this patch BTW).
> 
> Ehsan, does this mean the patch isn't suitable for back-porting to 1.9.2
> branch?

Not at all.  Is there a Thunderbird developer who can work on testing this and porting it back to 1.9.2

Comment 14

[Wayne Mery (:wsmwk)]

(In reply to comment #13)
> (In reply to comment #12)
> > Ehsan, does this mean the patch isn't suitable for back-porting to 1.9.2
> > branch?
> 
> Not at all.  Is there a Thunderbird developer who can work on testing this and
> porting it back to 1.9.2

that's good to hear. We probably want this for 1.9.2 (and nominating) because it is one of the highest reported issues at getsatisfaction (104 people and still rising) in http://getsatisfaction.com/mozilla_messaging/topics/when_i_receive_mail_from_incredimail_user_i_cant_reply_with_thunderbird_3_0_is_a_bug_have_you_got (via bug 525359). It has tests seems like the risk would be low.

Comment 15

[Daniel Veditz [:dveditz]]

making a semi-blocker for 1.9.2 -- if you get a branch patch it'll rise to the top of the approval queue, but we wouldn't block the next release on this if it doesn't make progress.

Comment 16

[Mark Banner (:standard8)]

Attachment: Attempted 1.9.2 back-port

Ehsan: I'm trying to back-port this to 1.9.2. The patch itself works fine and fixes the testcase in bug 525359.

The only changes currently in the patch when compared to trunk are in the Makefile.in so that the test is installed correctly.

However, when I was trying to run the test in Firefox I came across to issues:

1) The test gets marked as todo. There appears to be no actual check for pass/fail case. There is a single line "document.documentElement.clientWidth;", I'm not sure if this was meant to be the test.

2) I could not see any difference in the test case display with or without the source part of the patch applied.

Could you take a look - I'm hoping to get this in for the next releases.

Comment 17

[(no longer active)]

Comment on attachment 519901 [details] [diff] [review]
Attempted 1.9.2 back-port

Well, the test is tricky.  I mostly added it in the hope that it would corrupt the rendering of the browser if we regress this, something would break in the future tests.  The |document.documentElement.clientWidth| statement makes sure that we flush layout, so that all pending changes on the document take effect.

You can add an ok(true, "...") check before SimpleTest.finish if you don't want the test to get marked as todo, I don't care much either way.

However, please apply this fix <http://hg.mozilla.org/mozilla-central/rev/da4e84ab56c2> to the test before landing it, otherwise we'd be fighting with bug 618041 on 1.9.2 too!  :-)

Comment 18

[Daniel Veditz [:dveditz]]

Mark: Still trying to get this into a 1.9.2 release? Time's running out for 1.9.2.18

Comment 19

[Daniel Veditz [:dveditz]]

1.9.2.18 code freeze is Tuesday so I guess not.

Comment 20

[Mark Banner (:standard8)]

Yeah, sorry, we haven't had time to do all the necessary confirmation, and seeing as 5.0 will be coming out at the same time and the next 3.1.x release, for those seeing this issue, 5.0 will become the way to get the fix.