Wolfgang Rosenauer [:wolfiR] Reporter
	Description • 19 years ago

The first compose window after starting up thunderbird cannot be closed without
confirming a save message question. A second try for an empty message will close
the window immediately without confirmation.

	Mike Cowperthwaite
	Comment 1 • 19 years ago

Also on Win2K.  True for plain-text or HTML compose.  Apparently unrelated to compose-window caching, as after the first one is dismissed, you can have any number of compose windows open and they'll all dismiss immediately (so long as no changes have been made).

xref bug 250965, bug 262772.

	Wayne Mery (:wsmwk)
	Comment 2 • 18 years ago

(In reply to comment #1)
> Also on Win2K.  True for plain-text or HTML compose.  Apparently unrelated to
> compose-window caching, as after the first one is dismissed, you can have any
> number of compose windows open and they'll all dismiss immediately (so long as
> no changes have been made).
> 
> xref bug 250965, bug 262772.

Mike, why aren't this bug and bug 262772 duplicates of bug 250965?

	Mike Cowperthwaite
	Comment 3 • 18 years ago

I can reproduce this bug; 2b1 still behaves as I describe in comment 1.
However, the behavior is different on the trunk: I'm seeing it for any uncached compose window; that is, the first time I open a plain text window and close it, I'll get prompted; subsequent one-at-a-time plain text windows won't prompt, but if I open several, the uncached ones will all prompt.  Or if I then open an HTML window, it will prompt, then subsequent HTML ones won't.  I don't know if my testing before was flawed or if the behavior has in fact changed.

Bug 250965's reporter says:
> I can open and close compose windows a hundred times and it asks 100 times.

This implies that it occurs for all cached or uncached windows, and so is not this symptom.

When I confirmed this bug, I asked at bug 262772 if that behavior was also seen under Linux, but that reporter did not respond.

	Magnus Melin [:mkmelin]
	Comment 4 • 17 years ago

I see this, but only when mail.compose.max_recycled_win is set to 0.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3pre) Gecko/20070313 Thunderbird/2.0pre ID:2007031303

	Magnus Melin [:mkmelin]
	Comment 5 • 17 years ago

Well, this is strange. Now when i changed mail.compose.max_recycled_win back to 1, and restart, i still get the prompt. I'm pretty sure I did not get it before.

I also notice when I cancel the prompt and mail.compose.max_recycled_win is 0, I get this JS error.

Error: sMsgComposeService is not defined
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1064

	Wayne Mery (:wsmwk)
	Comment 6 • 17 years ago

(In reply to comment #5)
>
> I also notice when I cancel the prompt and mail.compose.max_recycled_win is 0,
> I get this JS error.
> 
> Error: sMsgComposeService is not defined
> Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
> Line: 1064

perhaps bug 160942 comment 28 and 29 

	Serge Gautherie (:sgautherie)
	Comment 7 • 17 years ago

[Mozilla Thunderbird, version 2.0.0.0pre (20070406)] (nightly) (W2Ksp4)
[Mozilla Thunderbird, version 3.0a1 (20070406)] (nightly) (W2Ksp4)

Seeing the "unneeded save question on first (only) window closing"
with new profile and account.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070406 SeaMonkey/1.5a] (nightly) (W2Ksp4)

No bug. (TB specific)

	Scott MacGregor
	Comment 8 • 17 years ago

we wouldn't respin the release for this.

	Mark Banner (:standard8)
	Comment 9 • 16 years ago

I'm seeing the same as Mike's comments in comment 3 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081003 Shredder/3.0b1pre

This is with mail.compose.max_recycled_win set to 1 (the default). Therefore updating title.

	Mark Banner (:standard8)
	Comment 10 • 16 years ago

I've just done some debugging on this, the editor keeps a record of modifications done. We try to reset that when the compose window finishes loading, however just after that we seem to be finishing loading one (or two) style sheets, the stack being:

#0  nsEditor::IncrementModificationCount (this=0xf5d600, inNumMods=1) at /Users/moztest/comm/main/src/mozilla/editor/libeditor/base/nsEditor.cpp:3830
#1  0x139e84e6 in nsEditor::DoAfterDoTransaction (this=0xf5d600, aTxn=0x17276c70) at /Users/moztest/comm/main/src/mozilla/editor/libeditor/base/nsEditor.cpp:4596
#2  0x139f1933 in nsEditor::DoTransaction (this=0xf5d600, aTxn=0x17276c70) at /Users/moztest/comm/main/src/mozilla/editor/libeditor/base/nsEditor.cpp:689
#3  0x139f1654 in nsEditor::DoTransaction (this=0xf5d600, aTxn=0x193b6630) at /Users/moztest/comm/main/src/mozilla/editor/libeditor/base/nsEditor.cpp:630
#4  0x13bd6600 in nsHTMLEditor::StyleSheetLoaded (this=0xf5d600, aSheet=0x174112f0, aWasAlternate=0, aStatus=2152857618) at /Users/moztest/comm/main/src/mozilla/editor/libeditor/html/nsHTMLEditor.cpp:4105
#5  0x1351c638 in CSSLoaderImpl::SheetComplete (this=0x1d5593a0, aLoadData=0x174114a0, aStatus=2152857618) at /Users/moztest/comm/main/src/mozilla/layout/style/nsCSSLoader.cpp:1564
#6  0x1351f0bb in SheetLoadData::OnStreamComplete (this=0x174114a0, aLoader=0x17411500, aContext=0x0, aStatus=2152857618, aDataStream=0x0) at /Users/moztest/comm/main/src/mozilla/layout/style/nsCSSLoader.cpp:756
#7  0x11cada7f in nsUnicharStreamLoader::OnStopRequest (this=0x17411500, request=0x17411740, ctxt=0x0, aStatus=2152857618) at /Users/moztest/comm/main/src/mozilla/netwerk/base/src/nsUnicharStreamLoader.cpp:167
#8  0x1203ad9a in nsJARChannel::OnStopRequest (this=0x17411740, req=0x174118e0, ctx=0x0, status=2152857618) at /Users/moztest/comm/main/src/mozilla/modules/libjar/nsJARChannel.cpp:841
#9  0x11c786fc in nsInputStreamPump::OnStateStop (this=0x174118e0) at /Users/moztest/comm/main/src/mozilla/netwerk/base/src/nsInputStreamPump.cpp:576
#10 0x11c7881c in nsInputStreamPump::OnInputStreamReady (this=0x174118e0, stream=0x1741197c) at /Users/moztest/comm/main/src/mozilla/netwerk/base/src/nsInputStreamPump.cpp:401
#11 0x004a832a in nsInputStreamReadyEvent::Run (this=0x17411bc0) at /Users/moztest/comm/main/src/mozilla/xpcom/io/nsStreamUtils.cpp:111
#12 0x004da908 in nsThread::ProcessNextEvent (this=0x713e50, mayWait=0, result=0xbfffe234) at /Users/moztest/comm/main/src/mozilla/xpcom/threads/nsThread.cpp:510

SM hasn't got this problem, so either it doesn't load the style sheets or its doing something different which means they are loaded before we call the reset.

	Morten Norby Larsen
	Comment 12 • 15 years ago

It seems to me that the problem is reproducible every time (not only the first one) if the account selected is a newsgroup.

And once it has happened once in the newsgroup account, it happens once again in the mail account, as described in this bug.

	Phil Ringnalda (:philor) Assignee
	Comment 13 • 15 years ago

CSS, indeed. The problem is that nsIEditorStyleSheets.idl has a tenuous grasp on the concept of truth. It claims that replaceStyleSheet can be async, which is close to true, since it *will* be async, that addStyleSheet is always sync, which is a total lie since addStyleSheet is actually replaceStyleSheet(nothing), that replaceOverrideStyleSheet is always sync, which is true, and that addOverrideStyleSheet "is assumed to be synchronous" where the meaning of "assumed to be" is "" since again add is replace(nothing).

	Andrew Sutherland [:asuth] (he/him)
	Comment 14 • 15 years ago

Tremendous catch.

Given that the effects of the change in the patch are arguably non-obvious, perhaps it should add a comment somewhere?

	Phil Ringnalda (:philor) Assignee
	Comment 15 • 15 years ago

Among other places, probably in the CSS files, mentioning that they're being loaded sync and you should keep your dirty @imports out of them.

	Mark Banner (:standard8)
	Comment 16 • 15 years ago

Comment on attachment 383742 [details] [diff] [review]
Fix v.1

Nice, thanks for finding this.

I think like you say we should add comments to the css files. Perhaps also to the now two instances in MsgComposeCommands where we call addOverrideStyleSheet stating that we're expecting synchronous loads. I feel I should check the comments once you've added them so r- only on that basis, I'd love this patch to go in asap though.

I think it would also be worth filing a core bug on tidying up the nsIEditorStyleSheets documentation.

	David :Bienvenu
	Comment 17 • 15 years ago

Yes, very nice. Does philor seek out these really complicated bugs, or do they find him? :-)

	Phil Ringnalda (:philor) Assignee
	Comment 18 • 15 years ago

> I think it would also be worth filing a core bug on tidying up the
> nsIEditorStyleSheets documentation.

Yeah, I have the bug typed and the patch mostly done, but I have to find bz in an insomniac mood again, to find out what I want to say about the removeStyleSheet things, since I keep getting lost on the way toward figuring out whether they're sync, async, or "it's entirely up to someone other than either the caller or the callee."

	Phil Ringnalda (:philor) Assignee
	Comment 19 • 15 years ago

With comments.

	Magnus Melin [:mkmelin]
	Comment 20 • 15 years ago

\o/

	Mark Banner (:standard8)
	Comment 21 • 15 years ago

Comment on attachment 383861 [details] [diff] [review]
Fix v.2

>-        editorStyle.addStyleSheet("chrome://messenger/skin/messageQuotes.css");
>+        /* We use addOverrideStyleSheet rather than addStyleSheet so that we get
>+           a synchronous load, rather than having a late-finishing async load
>+           mark our editor as modified when the user hasn't typed anything yet,
>+           but that means the sheet must not @import slow things, especially
>+           not over the network.
>+         */
>+        editorStyle.addOverrideStyleSheet("chrome://messenger/skin/messageQuotes.css");
...
>+  /* We use addOverrideStyleSheet rather than addStyleSheet so that we get
>+     a synchronous load, rather than having a late-finishing async load
>+     mark our editor as modified when the user hasn't typed anything yet,
>+     but that means the sheet must not @import slow things, especially
>+     not over the network.
>+   */
>   editor.addOverrideStyleSheet("chrome://messenger/content/composerOverlay.css");

nit: The prevailing style in MsgComposeCommands.js is to use // at the start of each line for multi-line comments, please use that here as well.

r=Standard8 with that fixed.

(and excellent catch btw)

	Phil Ringnalda (:philor) Assignee
	Comment 22 • 15 years ago

http://hg.mozilla.org/comm-central/rev/6bcbb9a88f88