Bug 912825 - [email] Message reader appears to duplicate body contents when buildBodyDom is called multiple times ( #275

Merged
merged 2 commits into from Jan 3, 2014

Conversation

Projects
None yet
2 participants
@mcav
Contributor

mcav commented Dec 12, 2013

No description provided.

Bug 912825 - [email] Message reader appears to duplicate body content…
…s when buildBodyDom is called multiple times (
+ // lines we'll need to fetch in order to roughly retrieve
+ // SNIPPET_SIZE_GOAL bytes.
+ var numLines = Math.floor(syncbase.POP3_SNIPPET_SIZE_GOAL / 80);
+ this.protocol.sendRequest('TOP', [number, numLines],

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 12, 2013

Contributor

Oh, and the reason I had to extract this constant was so that we could disable snippet fetching in the test.

@mcav

mcav Dec 12, 2013

Contributor

Oh, and the reason I had to extract this constant was so that we could disable snippet fetching in the test.

data/lib/mailapi/jobmixins.js
+ if (!body.bodyReps.every(function(rep) { return rep.isDownloaded; })) {
+ folderConn.downloadBodyReps(header, onDownloadReps);
+ } else {
+ onDownloadReps(null, body, true);

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

Please document the true, maybe like "/* claim we flushed to avoid an unneeded save */ true" since it's not immediately obvious.

@asutherland

asutherland Dec 13, 2013

Member

Please document the true, maybe like "/* claim we flushed to avoid an unneeded save */ true" since it's not immediately obvious.

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

Done.

@mcav

mcav Dec 16, 2013

Contributor

Done.

+ // Set POP3 to not retrieve any of the message when fetching
+ // headers. Otherwise it might have already finished downloading
+ // short messages, which would make the assertions below
+ // inconsistent between prototols.

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

this is an awesome comment, thanks for this.

@asutherland

asutherland Dec 13, 2013

Member

this is an awesome comment, thanks for this.

+ // Create a folder to test on
+ var eLazy = T.lazyLogger('misc');
+ var folderName = 'test_downloadbodyreps_idempotency';
+ var messageCount = 3;

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

On the numbers here: The IMAP test ends up with 4 messages in the inbox because we cram one in by default because of our timezone detection needs. I think this test would really be fine with just 1 message in the inbox, so maybe don't add any for IMAP, and just add 1 for other cases. This will make the log a little easier to look at and makes it more practical for us to place some more explicit expectations about what happens in the test. (More below on that.)

@asutherland

asutherland Dec 13, 2013

Member

On the numbers here: The IMAP test ends up with 4 messages in the inbox because we cram one in by default because of our timezone detection needs. I think this test would really be fine with just 1 message in the inbox, so maybe don't add any for IMAP, and just add 1 for other cases. This will make the log a little easier to look at and makes it more practical for us to place some more explicit expectations about what happens in the test. (More below on that.)

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

Done.

@mcav

mcav Dec 16, 2013

Contributor

Done.

+ eLazy.value('modified-' + idx);
+ }
+ } else {
+ header.getBody({ withBodyReps: true }, function() {

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

it's not really clear why you're also triggering withBodyReps here, so I think a comment would be useful. My interpretation is that you just want to make sure that withBodyReps doesn't somehow cause a different behaviour. Only a brief comment is needed.

@asutherland

asutherland Dec 13, 2013

Member

it's not really clear why you're also triggering withBodyReps here, so I think a comment would be useful. My interpretation is that you just want to make sure that withBodyReps doesn't somehow cause a different behaviour. Only a brief comment is needed.

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

There wasn't a reason, actually. I think I added it when trying to make the tests pass and forgot to remove it. I'll remove withBodyReps: true unless you think it should actually be in there for safety.

@mcav

mcav Dec 16, 2013

Contributor

There wasn't a reason, actually. I think I added it when trying to make the tests pass and forgot to remove it. I'll remove withBodyReps: true unless you think it should actually be in there for safety.

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

Oh, just kidding, I used it to ensure we receive the "done" event after the "modified" event. Otherwise getBody doesn't wait for anything. I'll add a comment to that effect.

@mcav

mcav Dec 16, 2013

Contributor

Oh, just kidding, I used it to ensure we receive the "done" event after the "modified" event. Otherwise getBody doesn't wait for anything. I'll add a comment to that effect.

+ }
+ }
+
+ // Fetch the body thrice; the first will generate onchange;

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

I think it would be good for us to do some testAccount.expect_runOp('downloadBodyReps', {}) calls here. Since there is a potential race (not terribly likely, but potential), I think it might make sense to just have 2 downloadBodyReps: true calls followed by a withBodyReps call and not have any chaining in place to rule out the potential for the job to never be issued.

// only the first job will actually download the bodies, the other jobs will still happen but will turn into no-ops
testAccount.expect_runOp('downloadBodyReps', { save: 'server' }); // this might need conn: true/etc.
testAccount.expect_runOp('downloadBodyReps', { save: false });
testAccount.expect_runOp('downloadBodyReps', { save: false });

Ideally we could also put an expectation on the sync too to make sure that we're only trying to talk to the server once. It doesn't look like POP3 currently has a good logging event to hang off of this and in general it seems like the gymnastics aren't yet justified since we haven't actually regressed on this. So, I think the save event may be a sufficient proxy for now.

This may also let you remove the set matching mode.

@asutherland

asutherland Dec 13, 2013

Member

I think it would be good for us to do some testAccount.expect_runOp('downloadBodyReps', {}) calls here. Since there is a potential race (not terribly likely, but potential), I think it might make sense to just have 2 downloadBodyReps: true calls followed by a withBodyReps call and not have any chaining in place to rule out the potential for the job to never be issued.

// only the first job will actually download the bodies, the other jobs will still happen but will turn into no-ops
testAccount.expect_runOp('downloadBodyReps', { save: 'server' }); // this might need conn: true/etc.
testAccount.expect_runOp('downloadBodyReps', { save: false });
testAccount.expect_runOp('downloadBodyReps', { save: false });

Ideally we could also put an expectation on the sync too to make sure that we're only trying to talk to the server once. It doesn't look like POP3 currently has a good logging event to hang off of this and in general it seems like the gymnastics aren't yet justified since we haven't actually regressed on this. So, I think the save event may be a sufficient proxy for now.

This may also let you remove the set matching mode.

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

I've added those expectations, and cleaned up the flow a bit. There's an unfortunate situation having to do with the fact that POP3 doesn't set USES_CONN, which means that POP3 doesn't use th_main's help_expect_connection stuff. Long story short, POP3 could use a pass to clean up that logging and allow us to have parity between the implementations at test time.

I added a comment and some exceptions for the POP3 case here; I don't think it makes sense to completely revamp that in this PR, but it's probably worth creating a tracking bug and addressing that independently.

@mcav

mcav Dec 16, 2013

Contributor

I've added those expectations, and cleaned up the flow a bit. There's an unfortunate situation having to do with the fact that POP3 doesn't set USES_CONN, which means that POP3 doesn't use th_main's help_expect_connection stuff. Long story short, POP3 could use a pass to clean up that logging and allow us to have parity between the implementations at test time.

I added a comment and some exceptions for the POP3 case here; I don't think it makes sense to completely revamp that in this PR, but it's probably worth creating a tracking bug and addressing that independently.

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 17, 2013

Member

agreed, don't need to revamp in this PR.

@asutherland

asutherland Dec 17, 2013

Member

agreed, don't need to revamp in this PR.

+ });
+ });
+
+// testAccount.do_closeFolderView(testView);

This comment has been minimized.

Show comment Hide comment
@asutherland

asutherland Dec 13, 2013

Member

remove or uncomment or something. Cleanup's not really required for this since the test then ends.

@asutherland

asutherland Dec 13, 2013

Member

remove or uncomment or something. Cleanup's not really required for this since the test then ends.

This comment has been minimized.

Show comment Hide comment
@mcav

mcav Dec 16, 2013

Contributor

Done.

@mcav

mcav Dec 16, 2013

Contributor

Done.

mcav added a commit that referenced this pull request Jan 3, 2014

Merge pull request #275 from mcav/downloadbodyreps-idempotent
Bug 912825 - [email] Message reader appears to duplicate body contents when buildBodyDom is called multiple times. r=asuth

@mcav mcav merged commit c348db8 into mozilla-b2g:master Jan 3, 2014

1 check passed

default The Travis CI build passed
Details

@mcav mcav deleted the mcav:downloadbodyreps-idempotent branch Jul 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment