499069 - Clicking on an empty IMAP folder does not display it until headers have been downloaded (flicker, account central shown)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

+++ This bug was initially created as a clone of Bug #498145 +++

Per Standard8, https://bugzilla.mozilla.org/show_bug.cgi?id=498145#c0:

> 1) When switching to an IMAP folder that is empty, account central briefly
> shows up. This is probably best seen just after startup when you're still
> opening connections etc, but it does it every time I enter an empty IMAP folder.

My response, https://bugzilla.mozilla.org/show_bug.cgi?id=498145#c4:

> This was sortof intentional, but not really.  Per the legacy code, we do not
> enter some folders[1] until a call to updateFolder completes.  The old code
> used to leave the previous view displayed through this 'limbo' time, but
> because of our normalized control flow and invariants, we end up displaying
> account central.
>
> I expect we can probably get away with entering empty IMAP folders early and
> should do so.  The account central thing is probably fine for the dubious
> mail.password_protect_local_cache pref.

bienvenu has taken on the task of enhancing nsMsgDBView so that we can actually do this; the effort is on bug 499064.

I have a patch that makes the minor change required so that we can enter immediately once this happens.  We may need to mitigate pathological screen refreshes by scrolling down by one row in the case where new messages are being inserted at the top.

Comment 1

[Mark Banner (:standard8)]

The flickering of account central on empty folders is really bad. We need to try and fix that.

Comment 2

[David :Bienvenu]

asuth, do you have a patch for this, now that bug 499064 is fixed? Or do you want me to try to whip something up?

Comment 3

[WADA:World Anti-bad-Duping Agency]

> 1) When switching to an IMAP folder that is empty, (snip)

Bug 462880 is relevant?
(if "*0 EXISTS" is returned, fetch flags doesn't seem to be issued, then already expunged mail at server is not removed if folder is empty.)

Comment 4

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

Comment 5

[Jay Moran (jaym/lazvon)]

I understand where the project's principals are coming from. I would like to just make a blunt comment though, and hopefully this is taken as the comment on the appearance of the product and NOT as any type of statement on the hard work of folks on the project, or their decisions.

The current flashing when changing folders looks like crap. If I was a new person trying out Thunderbird for the first time, or someone coming back after using other products for the past couple of years, I'd click on a couple of folders and wonder what other details have been overlooked in the release and worry for the integrity of my email and promptly go back to what I was using and likely not revisit. It looks shoddy as is. It is not ready to ship in that condition. I would feel very bad saying "Ship it!" in the current condition.

Again, I understand opinions will vary, and I certainly don't feel entitled here or anything, I just wanted to give my impression of the decision to ship with flashing folders. As always though, thanks to everyone for creating the best damn email client I've found out there!

Comment 8

[Hubert FONGARNAND]

Hmmm... I can't imagine thundebird 3... with this bug...

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Given where this bug was cloned from, I'm assuming this is a regression from Tb2.  That said, I'm on the fence about blocking 3.1 for this.  clarkbw, what's your feeling here?

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

This isn't part of the 3.1 goal of easing users into the new version of Thunderbird however it does seem like a poor regression.  

This feels like we need to make an engineering time investment call.  If there's an easy way to at least show nothing instead of the account central or it's simple enough to get this fixed I would think we could block the release for it.  If this feels long and involved I don't think it's more important than our ultimate 3.1 goal.

What do we have for ideas and estimates?

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

My reading of comment 0 is that Andrew has a patch with most of the work already done.  Giving to him for further comment...

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

I found my patch.  It's one line, which is nice.  I need to think a little about how to test / whether to test.

Notes to myself: The problem as stated on the bug implies a mozmill test but since the change is in DBViewWrapper we can do an xpcshell test; mozmill tests currently can't use IMAP (so currently untestable) and the change is so trivial the xpcshell test might end up a tautology.  (Also problematic is that the message injection code may want to force the header retrieval.)

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 change the policy to enter empty folders immediately, test the end-to-end goal

The patch basically consists of the desired policy change and what amounts to a test for the functionality we depend on from bug 499064.  Namely, that we enter a folder with zero messages and the view properly updates when the headers actually do get downloaded.  A minor testing infrastructure change is made to allow us to inject messages without triggering an updateFolder as part of the injection process.

Comment 14

[David :Bienvenu]

Comment on attachment 443884 [details] [diff] [review]
v1 change the policy to enter empty folders immediately, test the end-to-end goal

I'm seeing some very strange behavior in the message pane with this patch, especially failure to clear the message pane when switching folders.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #14)
> (From update of attachment 443884 [details] [diff] [review])
> I'm seeing some very strange behavior in the message pane with this patch,
> especially failure to clear the message pane when switching folders.

So, potentially important magical logic we could be losing out on the implementation of GetManyHeadersToDownload:

- http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#310
 returns true if isLocked.

- http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#1124
 returns true if !mDatabase
 (also returns true if total messages <= 0, but that's the explicit case we are expecting)

I don't really see either of these explaining what you're seeing.  Is it possible you ran with one of my other, more dangerous patches applied? :)

Comment 16

[David :Bienvenu]

I don't have anything else applied but this patch.

I'm seeing an exception:

Exception (error clearing message pane)
-- Exception object --
+ QueryInterface (function) 3 lines
+ message (string) 'Component returned failure code: 0x805e0006 [nsIDOMLocation.
href]'
+ result (number) 2153644038
+ name (object) null
+ filename (string) 'chrome://messenger/content/msgMail3PaneWindow.js'
+ lineNumber (number) 892
+ columnNumber (number) 0
+ location (object) JS frame :: chrome://messenger/content/msgMail3PaneWindow.js
 :: ClearMessagePane :: line 892
+ inner (object) null
+ data (object) null
+ initialize (function) 3 lines
*

I think I also saw the account central page displayed on startup, before the initial folder was loaded, which seemed new...

Comment 17

[David :Bienvenu]

ugh, sorry, I see that exception w/o your patch applied as well. Very strange.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

do you have davida's alternate account summary extension installed or any other extension that would try and co-opt one of our browser instances?

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

hm, yeah, your error code 0x805e0006 is NS_ERROR_CONTENT_BLOCKED...

Comment 20

[David :Bienvenu]

I looked at my extensions; none of the interesting ones were enabled, though maybe some of them left breadcrumbs around.

Comment 21

[David :Bienvenu]

Comment on attachment 443884 [details] [diff] [review]
v1 change the policy to enter empty folders immediately, test the end-to-end goal

the new imap test seems to leak a bit, but r=me, modulo that. I'll attach a leak log in a minute.

My message pane clearing issue seems to be moz-central trunk-only.

Comment 22

[David :Bienvenu]

Attachment: test run output with leak log

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Are you running against mozilla-central?  I see no leaks on linux or windows against mozilla-1.9.2.

Based on the signature, I would presume the fake server is not correctly/fully shutting down.

Comment 24

[David :Bienvenu]

(In reply to comment #23)
> Are you running against mozilla-central?  I see no leaks on linux or windows
> against mozilla-1.9.2.
This is with a 3.1 build, so comm central 1.9.2 and moz-central 1.9.2
> 
> Based on the signature, I would presume the fake server is not correctly/fully
> shutting down.
yes, more than likely. But we're not leaking imap connections, so that's good :-)

Comment 25

[Mark Banner (:standard8)]

Could this be an intermittent leak? There's a bug or two around here on that I think.

Comment 26

[Andrew Sutherland [:asuth] (he/him)]

pushed to comm-1.9.2:
http://hg.mozilla.org/releases/comm-1.9.2/rev/c4781e298ba3

pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/1627f0686bdc

I have punted on the leak seeing as how it sounds non-specific to the test, I can't easily reproduce, and shutdown race leaks relating to the test framework aren't tremendously concerning.