213729 - should look in mem cache for messages when offline

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Galileo Vidoni]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 (ax)

if you have an IMAP mail account, you see you messages, and then disconnect, you
can continue viewing those who had been read. but if you go to the offline mode,
and you click one of those messages, it sais that it isn't downloaded, so it is
not viewable. this happens if you don't download your messages, of course.

Reproducible: Always

Steps to Reproduce:
1.connect to an IMAP mail account and read a new mail
2.go to the offline mode
3.try to read it now

Actual Results:  
at least for the current session, let me see the same I was able to see in
online modeit says that the message hasn't been downloaded, so you can't see it

Expected Results:  
at least for the current session, let me see the same I was able to see in
online mode

Comment 1

[Henrik Gemal]

confirm with 20031003

when viewing a message we should check if the message has been download OR if
the message is in the cache.
I think we only does the first thing.

Comment 2

[Henrik Gemal]

looking at the sourcecode it seems weird, but it's 100% reproducable. The
sourcecode is:
http://lxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapService.cpp#961

---
PRBool msgIsInLocalCache = PR_FALSE;
msgurl->GetMsgIsInLocalCache(&msgIsInLocalCache);
if (!msgIsInLocalCache)
{
display the offline message in the messagepane.
---

so we should check if the message is in the cache. somehow this is not
happening. I'll try to debug some more. I "just" need to build mozilla, argh!

Comment 3

[David :Bienvenu]

local cache is the offline store, not the mem cache.

Comment 4

[Henrik Gemal]

of course. I didn't see:
if (WeAreOffline())
at
http://lxr.mozilla.org/mozilla/source/mailnews/imap/src/nsImapService.cpp#968

Comment 5

[David :Bienvenu]

changing summary

Comment 6

[David :Bienvenu]

Attachment: proposed fix

Look in memory cache as well as disk cache before displaying error message.

Comment 7

[[e-mail]]

Comment on attachment 328548 [details] [diff] [review]
proposed fix

Excellent! Any chance you can do news too? ;-)

>   rv = url->SetSpec(urlSpec);
What's the difference, if any, between this spec...

>+          url->GetAsciiSpec(escapedSpec);
... and this one?

Comment 8

[David :Bienvenu]

heh, what is this news you speak of?  :-)

I could take a stab at news. Worst case, I could tell Joshua how to do it.

The difference between the specs is that the spec you get back is escaped, or more escaped than the one that is set.

Comment 9

[David :Bienvenu]

Attachment: fix including news

this patch includes news. It also changes the check for a cache session to just use mCacheSession. If mCacheSession hasn't been set, no use looking for anything in the cache.

Comment 10

[[e-mail]]

Comment on attachment 328758 [details] [diff] [review]
fix including news

>+      if (!msgIsInCache)
This could do with an && mCacheSession rather than testing it later.
r+sr=me for the imap patch with this fixed. Still testing news...

>+          // stick the uid validity in front of the url, so that if the uid validity
>+          // changes, we won't re-use the wrong cache entries.
Side note: does this makes about:cache harder to use?

>         if (WeAreOffline())
>         {
>-          if (!hasMsgOffline)
Might want to keep this check and add an mCacheSession check to save time (possibly not all in separate if statements).

>+          url->SetPort(isSecure ? SECURE_NEWS_PORT: NEWS_PORT);
Nit: space before :

>+          // nntp urls are truncated at the query part when used as cache keys
>+          char * anchor = (char *)strrchr(cacheKey.BeginWriting(), '?');
>+          if (anchor)
>+            *anchor = '\0';
I suspect that this isn't the correct way to shorten an nsCString ;-)
PRInt32 pos = cacheKey.FindChar('?');
if (pos != -1)
  cacheKey.SetLength(pos);

Comment 11

[[e-mail]]

Comment on attachment 328758 [details] [diff] [review]
fix including news

Sorry, I can't get this to work for news.

Comment 12

[[e-mail]]

Comment on attachment 328548 [details] [diff] [review]
proposed fix

>+      if (!msgIsInCache)
r+sr=me if you check mCacheSession here and use it below.

Comment 13

[David :Bienvenu]

thx, Neil, I'll change the imap cache session check.

Re news, can you compare the cache key used when we put a message in the cache,

http://mxr.mozilla.org/mozilla/source/mailnews/news/src/nsNNTPProtocol.cpp#934

with the one my patch uses to look up the message:

+            msgIsInCache = NS_SUCCEEDED(mCacheSession->OpenCacheEntry(cacheKey,
+                                        nsICache::ACCESS_READ, PR_FALSE,
+                                        getter_AddRefs(cacheEntry)));

One possibility is that there's something about your news server configuration that makes the cache keys different - mine is extremely vanilla, news.mozilla.org, port 119, no user name or password.

Comment 14

[David :Bienvenu]

Attachment: fix for news

try this - it turned out that nsNNTPProtocol::Init was bailing out with an error if we were offline, because, unlike imap, it tries to open a socket early on in the process.  If we avoid the early bailout when offline, we will try to use the memory cache later on.

I don't know why it seemed to work for me - I might have just been fixated on getting the mem cache key just right.

I had to use same approach to truncating the cache key in both places in order for the match to work.

Comment 15

[[e-mail]]

Comment on attachment 329078 [details] [diff] [review]
fix for news

I really feel happier about the new cache key code too :-)

Comment 16

[David :Bienvenu]

fixed for both news and imap