Last Comment Bug 213729 - should look in mem cache for messages when offline
: should look in mem cache for messages when offline
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.1a1
Assigned To: David :Bienvenu
:
Mentors:
: 105146 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-07-24 09:24 PDT by Galileo Vidoni
Modified: 2009-01-22 10:17 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (3.63 KB, patch)
2008-07-08 13:11 PDT, David :Bienvenu
neil: review+
neil: superreview+
Details | Diff | Splinter Review
fix including news (6.23 KB, patch)
2008-07-09 15:02 PDT, David :Bienvenu
neil: review-
neil: superreview-
Details | Diff | Splinter Review
fix for news (4.57 KB, patch)
2008-07-11 08:12 PDT, David :Bienvenu
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Galileo Vidoni 2003-07-24 09:24:25 PDT
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 2003-10-07 10:42:50 PDT
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 2003-10-07 11:01:53 PDT
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 2003-10-07 11:09:56 PDT
local cache is the offline store, not the mem cache.
Comment 4 Henrik Gemal 2003-10-07 11:22:42 PDT
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 2004-03-24 08:02:35 PST
changing summary
Comment 6 David :Bienvenu 2008-07-08 13:11:00 PDT
Created attachment 328548 [details] [diff] [review]
proposed fix

Look in memory cache as well as disk cache before displaying error message.
Comment 7 neil@parkwaycc.co.uk 2008-07-08 16:14:01 PDT
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 2008-07-08 16:21:23 PDT
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 2008-07-09 15:02:40 PDT
Created attachment 328758 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2008-07-09 16:27:59 PDT
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 neil@parkwaycc.co.uk 2008-07-11 04:32:50 PDT
Comment on attachment 328758 [details] [diff] [review]
fix including news

Sorry, I can't get this to work for news.
Comment 12 neil@parkwaycc.co.uk 2008-07-11 04:35:32 PDT
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 2008-07-11 06:40:54 PDT
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 2008-07-11 08:12:59 PDT
Created attachment 329078 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2008-07-13 13:16:08 PDT
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 2008-07-17 15:26:48 PDT
fixed for both news and imap
Comment 17 David :Bienvenu 2008-07-25 08:35:42 PDT
*** Bug 105146 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.