Allow anyone to use StreamMessage *and* fetch parts on demand

RESOLVED FIXED in Thunderbird 5.0b1

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: protz, Assigned: protz)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 5.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

This is for Thunderbird Conversations.

My goal is to allow the JS Mime Emitter to stream an IMAP message with fetchPartsOnDemand set to true. When a message arrives in the inbox, if the user wishes to display it ASAP, then we stream the message through the jsmimeemitter, because gloda hasn't indexed it yet, and we need a lot of metadata, e.g. all the headers.

However, if this is a message with attachments, this kicks a complete download of the message, and it gives a very bad user experience. The second streaming (the one to display the message, not get the metadata) goes fast, because that one runs on a message that's been downloaded already.

SetFetchPartsOnDemand(PR_TRUE) happens when we call DisplayMessage (regular display pipeline). Not when we call StreamMessage, and there's currently no option to allow that.

The patch I'm attaching does just this. Could you give me some feedback? It works fine as far as I can tell, except that the MimeMessage doesn't know anymore about the attachments but that's something I should be able to easily fix.

Is there any way I can test this in the context of an xpcshell test? Currently, I'm sending myself a message with a big image attachment, and I'm clicking on it fast to display it through the Thunderbird Conversations pipeline... not the best way to achieve that I'm afraid!
Attachment #525060 - Flags: feedback?(bienvenu)
Attachment #525060 - Flags: feedback?(bugmail)
Posted patch New version (obsolete) — Splinter Review
This is a new version of the patch. In the case of mime-parts-on-demande, mime/src/mimei.cpp but extremely strange URLs with /;section=N parts, they look like:

imap://jonathan%2Eprotzenko@imap.free.fr:143/fetch%3EUID%3E/Tests%20GCV/Dont-sync-this%3E30280?header=filter&emitter=js/;section=2?part=1.2&filename=DSC_5633.JPG

After stripping, this gives:

imap://jonathan%2Eprotzenko@imap.free.fr:143/fetch%3EUID%3E/Tests%20GCV/Dont-sync-this%3E30280?/;section=2?part=1.2&filename=DSC_5633.JPG

so I had to modify the part_RE from jsmimeemitter.js to also work for these URLs. Surprisingly, these are *correct* URLs, and the thumbnail stuff still works. I just can't believe it!
Attachment #525060 - Attachment is obsolete: true
Attachment #525060 - Flags: feedback?(bugmail)
Attachment #525060 - Flags: feedback?(bienvenu)
Attachment #525062 - Flags: review?(bugmail)
Attachment #525062 - Flags: feedback?(bienvenu)
Posted patch Newer version (obsolete) — Splinter Review
Ok, this version fixes a small bug, that is, we have to strip the magic flag inside jsmimeemitter.js, because other server types such as IMAP will just pass all magic flags along.
Attachment #525062 - Attachment is obsolete: true
Attachment #525062 - Flags: review?(bugmail)
Attachment #525062 - Flags: feedback?(bienvenu)
Attachment #525065 - Flags: review?(bugmail)
Attachment #525065 - Flags: feedback?(bienvenu)
Comment on attachment 525065 [details] [diff] [review]
Newer version

This seems like a very friendly addition and a good idea.

- No embedded ANSI codes.  I love me some colors, but I believe the windows console still does not interpret them by default without some ansi.sys hoops.

- You need a test.  It looks like the the fake imap server supports BODYSTRUCTURE, so I presume this is feasible.
Attachment #525065 - Flags: review?(bugmail) → review+
Comment on attachment 525065 [details] [diff] [review]
Newer version

(I did the wrong flag thing; meant feedback+ as I want to see the test before giving review.)
Attachment #525065 - Flags: review+ → feedback+
Comment on attachment 525065 [details] [diff] [review]
Newer version

can just be:

imapUrl->SetFetchPartsOnDemand(fetchOnDemand != kNotFound);

+      if (fetchOnDemand != kNotFound)
+        imapUrl->SetFetchPartsOnDemand(PR_TRUE);
+      else
+        imapUrl->SetFetchPartsOnDemand(PR_FALSE);

With your patch, will we just fetch the body, and not the parts, even if the parts are inline, and display attachments inline is set to true? I know the imap code likes to fallback to fetching the whole message if all the parts are displayable inline.
Attachment #525065 - Flags: feedback?(bienvenu) → feedback+
(In reply to comment #3)
> Comment on attachment 525065 [details] [diff] [review]
> Newer version
> 
> This seems like a very friendly addition and a good idea.
> 
> - No embedded ANSI codes.  I love me some colors, but I believe the windows
> console still does not interpret them by default without some ansi.sys hoops.
I tend to leave dump statements all over the place when I submit WIP patches, but rest assured that I will remove them in due time :-).
> 
> - You need a test.  It looks like the the fake imap server supports
> BODYSTRUCTURE, so I presume this is feasible.
Ouch. I've never written a test that involves the IMAP server, but I might be able to copy an existing test and adapt it. Thanks for the feedback!

(In reply to comment #5)
> Comment on attachment 525065 [details] [diff] [review]
> Newer version
> 
> can just be:
> 
> imapUrl->SetFetchPartsOnDemand(fetchOnDemand != kNotFound);
ok
> 
> +      if (fetchOnDemand != kNotFound)
> +        imapUrl->SetFetchPartsOnDemand(PR_TRUE);
> +      else
> +        imapUrl->SetFetchPartsOnDemand(PR_FALSE);
> 
> With your patch, will we just fetch the body, and not the parts, even if the
> parts are inline, and display attachments inline is set to true? I know the
> imap code likes to fallback to fetching the whole message if all the parts are
> displayable inline.
I don't know about that. I just know that DisplayMessage sets fetchPartsOnDemand to true, which is what allows the streaming code to go fast. Since this is not bound for display, but rather for the jsmimeemitter, I don't think it matters much whether we're missing an inline part or two...
(In reply to comment #6)
> I don't know about that. I just know that DisplayMessage sets
> fetchPartsOnDemand to true, which is what allows the streaming code to go fast.
> Since this is not bound for display, but rather for the jsmimeemitter, I don't
> think it matters much whether we're missing an inline part or two...
I think for your use case you *want* to not fetch the whole message, even if all the parts are inline, so I was just wondering if that was working the way you wanted...otherwise some messages will still be slow to stream.
So the reason why I can't get this test to work is the MIME message generated by the fake imap server in the case of download-parts-on-demand is not parsable by libmime. The other test that exercises download-parts-on demand is test_dod.js, and it conveniently hides this fact by grepping the raw message contents, not trying to feed it to libmime.

While I could go on debugging for a few more hours and fix the root problem (i.e. do the right thing), and lose a few more hair trying to debug libmime, here's what I suggest instead for the test:
- call MsgHdrToMimeMessage with the new option,
- obtain a bogus MimeMessage whose structure actually hasn't been parsed, and is just a body with the raw message contents,
- test this body for the string "This message part will be downloaded on demand.",
- assume no one will be upset by this.

Patch follows.

David, this indeed does not fix the "inline JPEGs are downloaded in all cases", and as you said, some messages will still be slow to download. I suggest pursuing this discussion in a followup bug, is that ok for you?
Attachment #525065 - Attachment is obsolete: true
Attachment #526605 - Flags: review?(bugmail)
Further IMAP testing allowed me to pinpoint the error:

-9877760[1d3ba60]: 1d359f0:localhost:S-INBOX:CreateNewLineFromSocket: * 1 FETCH (BODY[HEADER] {13}
-9877760[1d3ba60]: 1d359f0:localhost:S-INBOX:STREAM:OPEN Size: 0: Begin Message Download Stream
-9877760[1d3ba60]: ReadNextLine [stream=1d3d570 nb=11 needmore=0]
-9877760[1d3ba60]: 1d359f0:localhost:S-INBOX:CreateNewLineFromSocket: undefined

So the IMAP server returns "undefined". With no headers at all, no wonder libmime can't figure out how to parse the message. The error comes from imapd.js:1481, which reads like so:

    case "HEADER": // I believe this specifies mime for an RFC822 message only
      data += bodyPart.mime + "\r\n\r\n";
      break;

Naïve attempts did not fix the thing, there's probably a deeper issue in the IMAP fake server. My viewpoint still doesn't change, and I do think we should leave the test "as is", because the cost of fixing the IMAP fake server would be way too high. I'm just writing this for posterity and because, well, I like to hunt bugs down :-).

CCing jcranmer/standard8 as I think one of them wrote the IMAP fake server.
(In reply to comment #10)
>     case "HEADER": // I believe this specifies mime for an RFC822 message only
>       data += bodyPart.mime + "\r\n\r\n";
>       break;
> 
> Naïve attempts did not fix the thing, there's probably a deeper issue in the
> IMAP fake server. My viewpoint still doesn't change, and I do think we should
> leave the test "as is", because the cost of fixing the IMAP fake server would
> be way too high. I'm just writing this for posterity and because, well, I like
> to hunt bugs down :-).

IIRC, I originally left the magic MIME stuff out of the original IMAP implementation, as just getting libmime to give me the real mime structure was like pulling teeth.

Mercurial seems to agree with my recollection--this line dates to bug 540676, and was originally written by Phil Lacy.
CCing Phil Lacy, he might be able to fix this for us. Phil, thoughts? :-)
Comment on attachment 526605 [details] [diff] [review]
New version of the patch with a test that... hem... "works"

It sounds like bienvenu understands the problem domain better than I, so you should probably direct the next review to him.  But before you ask for the review, if you are going to keep using this test, please finish the process of adapting the test you copied and pasted to your new purpose.  It's confusing to read a test when it's a mish-mash of two tests.  (I understand this is not the happiest code to be working with :)

One simplification you can make to the test is to just use "asyncUrlListener" instead of defining UrlListener.
Attachment #526605 - Flags: review?(bugmail) → review-
Posted patch Test cleaned up (obsolete) — Splinter Review
Thanks for the comments. I was unaware of the existence of asyncUrlListener, which is why I didn't use it. And yes, starting from an existing test is usually the easiest way to achieve what you want. But hopefully the ongoing discussion about improving the entry cost to writing tests will alleviate this kind of burden, and I may one day start writing a test without copy/pasting an existing one :-).

Transferring the review request to :bienvenu.
Attachment #526605 - Attachment is obsolete: true
Attachment #526679 - Flags: review?(dbienvenu)
My inclination is to try to fix the imap fakeserver, but I don't know how hard that will be. I will try to look at that tomorrow.
protz, can you try this with your older test? If it fails, please let me know how, and attach the test; if it succeeds, I'd take that as an r+ :-)
Comment on attachment 526679 [details] [diff] [review]
Test cleaned up

clearing review request until we see if we can fix the fake server issue.
Attachment #526679 - Flags: review?(dbienvenu)
David, your patch seems to indeed fix the fake server. Thanks! I've attached an updated test that does a clean test for a part-to-be-downloaded attachment URL. How do you want to proceed? Do you want to commit your fix, and then mine, do you want to commit both, do you want me to give you r+ for your patch and have me commit the two?

I'm asking for review again because I didn't quite understand if your meant r+ on your patch or mine :-).
Attachment #526679 - Attachment is obsolete: true
Attachment #527760 - Flags: review?(dbienvenu)
Sent the wrong patch.
Attachment #527760 - Attachment is obsolete: true
Attachment #527760 - Flags: review?(dbienvenu)
Attachment #527761 - Flags: review?(dbienvenu)
Comment on attachment 527688 [details] [diff] [review]
attempt to fix fake server - checked in.

I meant I'd take your saying that the fake server changed worked for you as your r+ for the fake server change, since you're in the unique position of having a test for that change :-) I'll land my fix and then test your patch and give r+ when I've verified that it works here, and let you land your patch.
Attachment #527688 - Flags: review+
Attachment #527688 - Attachment description: attempt to fix fake server → attempt to fix fake server - checked in.
Comment on attachment 527761 [details] [diff] [review]
Updated patch with the test

r=me, if you fix the comments that refer to the original test:

test that you can stream w/o the attachments...
+/*
+ * Tests imap save and detach attachments.
+ */

A URL
+    // An URL containing this string indicates that the attachment will be

probably just remove this one:
+  // We need to register the dummyMsgWindow so that when we've finished running
+  // the append url, in nsImapMailFolder::OnStopRunningUrl, we'll think the
+  // Inbox is open in a folder and update it, which the detach code relies
+  // on to finish the detach.
Attachment #527761 - Flags: review?(dbienvenu) → review+
checked-in http://hg.mozilla.org/comm-central/rev/cd22a42875a9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Followup on bug 652316
You need to log in before you can comment on or make changes to this bug.