Mail reply window stops painting.

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: bwinton, Assigned: Ehsan)

Tracking

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 -, status1.9.2 wanted)

Details

(Whiteboard: [tb33needed])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
###!!! 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 )
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.)
blocking2.0: --- → ?
Whiteboard: [tb33needs]
blocking2.0: ? → final+
(Reporter)

Comment 2

7 years ago
FWIW, I'm more than happy to help Ehsan with any problems he might have building comm-central.
(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.  :-)
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.
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).
Attachment #495286 - Flags: review?(bzbarsky)
Whiteboard: [tb33needs] → [tb33needs][has patch][needs review bz]
Created attachment 495296 [details] [diff] [review]
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.
Attachment #495286 - Attachment is obsolete: true
Attachment #495296 - Flags: review?(bzbarsky)
Attachment #495286 - Flags: review?(bzbarsky)
Comment on attachment 495296 [details] [diff] [review]
Patch (v2)

r=me
Attachment #495296 - Flags: review?(bzbarsky) → review+
Whiteboard: [tb33needs][has patch][needs review bz] → [tb33needs][needs landing]
http://hg.mozilla.org/mozilla-central/rev/a7adfbb051fd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus?
Resolution: --- → FIXED
Whiteboard: [tb33needs][needs landing] → [tb33needs]
Target Milestone: --- → mozilla2.0b8
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...
(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.
Good to know, thanks. Adding bienvenu and protz, as I suspect they'll want to be conscious of this as well.
Depends on: 618041
Whiteboard: [tb33needs] → [tb33needed]
(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?
Whiteboard: [tb33needed] → [tb33needed][tb31needs](?)
(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
(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.
blocking1.9.2: --- → ?
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.
blocking1.9.2: ? → needed
status1.9.2: --- → wanted
Created attachment 519901 [details] [diff] [review]
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.
Attachment #519901 - Flags: feedback?(ehsan)
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!  :-)
Attachment #519901 - Flags: feedback?(ehsan) → feedback+
Mark: Still trying to get this into a 1.9.2 release? Time's running out for 1.9.2.18
blocking1.9.2: needed → ?
1.9.2.18 code freeze is Tuesday so I guess not.
blocking1.9.2: ? → -
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.
Whiteboard: [tb33needed][tb31needs](?) → [tb33needed]
Blocks: 735805
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.