Closed Bug 321783 Opened 15 years ago Closed 11 years ago

Unneeded save message question for empty compose window (when the window isn't a cached one)

Categories

(Thunderbird :: Message Compose Window, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: wolfiR, Assigned: philor)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
OS: Linux → All
(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?
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.
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
Summary: unneeded save message question for empty compose window → unneeded save message question for empty compose window when mail.compose.max_recycled_win = 0
Version: 1.5 → 2.0
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
(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 

[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)
Blocks: 250965, 262772
Severity: normal → minor
Flags: blocking-thunderbird2?
Version: 2.0 → Trunk
we wouldn't respin the release for this.
Flags: blocking-thunderbird2? → blocking-thunderbird2-
Target Milestone: --- → Thunderbird 3
QA Contact: message-compose
Assignee: mscott → nobody
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.
Summary: unneeded save message question for empty compose window when mail.compose.max_recycled_win = 0 → Unneeded save message question for empty compose window (when the window isn't a cached one)
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.
Duplicate of this bug: 498594
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.
Flags: blocking-thunderbird3?
Attached patch Fix v.1 (obsolete) — Splinter Review
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).
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Attachment #383742 - Flags: review?(bugzilla)
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
Hardware: x86 → All
Tremendous catch.

Given that the effects of the change in the patch are arguably non-obvious, perhaps it should add a comment somewhere?
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.
Blocks: 245196
Keywords: regression
Attachment #383742 - Flags: review?(bugzilla) → review-
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.
Yes, very nice. Does philor seek out these really complicated bugs, or do they find him? :-)
> 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."
Attached patch Fix v.2Splinter Review
With comments.
Attachment #383742 - Attachment is obsolete: true
Attachment #383861 - Flags: review?(bugzilla)
Depends on: 499071
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)
Attachment #383861 - Flags: review?(bugzilla) → review+
http://hg.mozilla.org/comm-central/rev/6bcbb9a88f88
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3 → Thunderbird 3.0b3
No longer blocks: 250965
Duplicate of this bug: 262772
You need to log in before you can comment on or make changes to this bug.