1116694 - [email/IMAP] moves/deletions broken on servers without UIDNEXT support and batch moves/deletions on UIDPLUS servers, such as all CoreMail servers (ex: 126.com, 163.com)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[ffos_st]

Description:
The email app cann't delete the message

Device: 
SPRD 7715ea

Build Identifier: 
20141230114532

Steps to Reproduce:
1) Setting the email account.
2) After waiting to accept mail.
3) Select a message to Delete

Actual Result: 
 The message can't be deleted 

Expected Result:
 The message can be deleted 

Repro frequency:
10/10,100%

Comment 1

[André Fiedler]

Same for me on Firefox OS 2.2 / Nexus 4 (build from master on 2015-01-02)

Comment 2

[André Fiedler]

Btw, I'm using Apples iCloud E-Mail Service ([e-mail])

Comment 3

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi ffos_st -

I just try my gmail account with the newest 2.1 build on Flame, I can successfully delete the email. Could you provide me the information of the email account you are using to reproduce this issue? 

Thanks for your help

Comment 4

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

also ni Andrew for further comment

Comment 5

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

I still need to review some of the various logcats and try and duplicate on dreamhost where I've also heard of a repro, but this is very likely to be a variant on bug 839273 which could explain why it's hard to reliably reproduce the problem.

Comment 6

[yong.ren]

Hi Vance and  Andrew Sutherland:

   I‘m very sorry to reply this problem too late, this bug is reproduced by our RDs on the 163 email account. I will tell them to reply quickly to the needinfo request in the future;

   on the 163 email account, the bug's Repro frequency is 100%, you can easy get the problem log. 

   thanks a lot.

Comment 8

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to ffos_st from comment #7)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #3)
> > Hi ffos_st -
> > 
> > I just try my gmail account with the newest 2.1 build on Flame, I can
> > successfully delete the email. Could you provide me the information of the
> > email account you are using to reproduce this issue? 
> > 
> > Thanks for your help
> 
> 
> Hi Vance and  Andrew Sutherland:
>   I'd like to apologize sincerely  for failing to reply to your email in
> time.
>   The email account we use to test this bug is 163 email account.
>   The specified account information is as follows:
>           account : [e-mail]
>           passward : 890406liuyifei 
>   When we use the IMAP protocol, it can appear this bug.But if the POP3
> protocol is used, this bug can not be occur.
> 
> thanks
> best wishes.

Thanks, I can reproduce this issue with the account you provide.

Hi Andrew, could you help to check this one? I use Flame 2.1 and can reproduce this issue with the account mentioned in Comment#7(at first it looks like you successfully delete the email, but once you press the sync button, the email you just delete is back). Not sure if this is a server or client side issue

Thanks

Thanks!

Comment 9

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

If at all possible, please do provide a logcat whenever you reproduce in the future.  This failure is relatively straightforward, but especially for deletion it's possible it could have been other permutations that are time-dependent on when you hit the refresh button, etc.

The problem is because of the lack of UIDNEXT.  In addition to the timezone probing we did, our efficient fetch to detect the target message's UID also depends on the (required by spec) UIDNEXT.  Besides using fallback logic in this case, I'll:
- Correct the logic to correctly propagate the error rather than silently eating it; it's not clear why we were taking that shortcut.  (Noting that this wouldn't have actually done anything useful in this case; in fact, it would have been worse, but it would have made the failure more obvious in the logcat and is helpful to the unit tests.)
- Add an additional test runner permutation so all of the core IMAP tests run with the fake-server in the mode where it doesn't provide UIDNEXT to help provide confidence about this.

The logcat entry for this ends up looking like:
WLOG: [slog] imap:protocol-error {"humanReadable":"Parse command error","commandData":"W7 UID FETCH undefined:* (UID BODY.PEEK[HEADER.FIELDS (MESSAGE-ID)])"}


I've made comment 7 that included the account credentials private.

Comment 10

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Andrew -

Our partner consider this one as a device shipping blocker and hope we can treat this issue as 1st priority. Could you help to prioritize it and let us know the expected fixed date?

Appreciate your help

Comment 11

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

Attachment: GELAM patch with tests

Here's the GELAM fix and testing improvements PR.  I'll put this up for review once I test against 163.com.

We can probably get this reviewed and landed on trunk on Tuesday.  Any uplift approvals are out of our control, of course.

Comment 12

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Thanks for your promptly help Andrew. 

Yong Ren, could you help to test the patch and let us know the result as well?

Thanks

Comment 13

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

Attachment: gaia pull request (landed)

Comment 14

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

Comment on attachment 8551600 [details] [review]
GELAM patch with tests

b2g-desktop testing showed this addressed the error.  Comments on the patch are in the commit/pull request.

Comment 15

[yong.ren]

Okay, I will invite the ffos_st to test this patch, and reply quickly, thanks vance and Asuth. :)

Comment 16

[ffos_st]

Hi:Yong Ren and  Vance:

I have tested this patch on the device 7715ea,The bug have no longer reproduce.

thanks

Comment 17

[ffos_st]

Hi:Yong Ren and  Vance:

I am so sorry to say that this bug can reproduce sometimes even though I have push this patch on my devices 7715ea.

when I delete only one message,this message is deleted.But when I delete multiple messages at the same time,this bug can be reproduced.

thanks.

Comment 18

[yong.ren]

(In reply to ffos_st from comment #17)
> Hi:Yong Ren and  Vance:
> 
> I am so sorry to say that this bug can reproduce sometimes even though I
> have push this patch on my devices 7715ea.
> 
> when I delete only one message,this message is deleted.But when I delete
> multiple messages at the same time,this bug can be reproduced.
> 
> thanks.

Hi Vance and Asuth:

    attachment 8551600 [details] [review]'s patch can not cover the whole problem, we can also reproduce the bug when delete multiple message at the same time.  please help to investigate it, thanks.

Comment 19

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

I can confirm the multiple response.  The problem is that browserbox is choking on the response to our UID COPY command, specifically, we get back:
  W14 OK [COPYUID 4 1417051618:1417051620 1421730687:1421730689] COPY completed
and browserbox reports an unexpected char at position 61, which is the "]" character.

Looking into this now.

Comment 20

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

Attachment: (upstream) imap-handler pull request to address the COPYUID response code parse failure

I've fixed the issue in imap-handler and provided an upstream pull request.  That pull request may get changed in review or partially rewritten, so it's not clear this is 100% exactly what will become permanent in upstream, but it's probably okay to land this tentatively anyways.

I've included the imap-handler change in the GELAM patch as a second commit.  I've updated the gaia commit to include all fixes in a single commit.  So testing with the revised gaia commit should now work.  (It worked for me on my 163.com test account)

Comment 21

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Thanks Andrew!

Hi Yon Ren, could you help to verify again with attachment#8551734 [details] [review] ?

Thanks!

Comment 22

[yong.ren]

Hi Vance:


Andrew's patch has some problem with push's to the wrong branch, I am communicating with him.

when these things are ready, I will reply quickly, thanks again.

Comment 23

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8551600 [details] [review]
GELAM patch with tests

lgtm.

Comment 24

[Dylan Oliver [:doliver]]

triage: not blocking but will seek 2.2 approval

Comment 25

[ffos_st]

Hi: Vance and Yong Ren:

I have tested the new patch on the device 7715ea,this can delete one or more message completely

thanks

Comment 26

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to ffos_st from comment #25)
> Hi: Vance and Yong Ren:
> 
> I have tested the new patch on the device 7715ea,this can delete one or more
> message completely
> 
> thanks

Lovely, thanks for testing on this patch!

Vance

Comment 27

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

landed on master which is 3.0:
https://github.com/whiteout-io/imap-handler/pull/9
https://github.com/whiteout-io/imap-handler/commit/3b54101d43925a17c793d7f02d555d6e0c1144c2

https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/358
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/2b4a7748ba71aa95b0785c183d0557a099065cf9

https://github.com/mozilla-b2g/gaia/pull/27513
https://github.com/mozilla-b2g/gaia/commit/e45c5dbdcfc2d598c889dfbea72fa11157422afe

Comment 28

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

Comment on attachment 8551608 [details] [review]
gaia pull request (landed)

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Complicated, but not really a regression.  We moved to use IMAP for 163.com/126.com with v2.1; prior to that autoconfig would result in ActiveSync being used, which was broken for different reasons.  IMAP would always have suffered from the lack of UIDNEXT support by the server even prior to v2.1, but the COPYUID status code parse failure was also new in v2.1.  The COPYUID parse failure will affect any batch modifications on any UIDPLUS server.
[User impact] if declined: Batch message deletion/moves won't work on IMAP servers supporting UIDPLUS.  All message deletion/moves won't work on 163.com and 126.com and other CoreMail servers.
[Testing completed]: Unit tests, testing by me against 163.com, testing by ffos_st against 163.com.
[Risk to taking this patch] (and alternatives if risky): Very low risk, especially with all our fancy testing!
[String changes made]: No string changes.

Comment 29

[bhavana bajaj [:bajaj]]

(In reply to ffos_st from comment #0)
> Description:
> The email app cann't delete the message
> 
> Device: 
> SPRD 7715ea
> 
> Build Identifier: 
> 20141230114532
> 
> Steps to Reproduce:
> 1) Setting the email account.
> 2) After waiting to accept mail.
> 3) Select a message to Delete
> 
> Actual Result: 
>  The message can't be deleted 
> 
> Expected Result:
>  The message can be deleted 
> 
> Repro frequency:
> 10/10,100%

can you verify the issue is fixed for you?

Comment 30

[yong.ren]

(In reply to bhavana bajaj [:bajaj] from comment #29)
> (In reply to ffos_st from comment #0)
> > Description:
> > The email app cann't delete the message
> > 
> > Device: 
> > SPRD 7715ea
> > 
> > Build Identifier: 
> > 20141230114532
> > 
> > Steps to Reproduce:
> > 1) Setting the email account.
> > 2) After waiting to accept mail.
> > 3) Select a message to Delete
> > 
> > Actual Result: 
> >  The message can't be deleted 
> > 
> > Expected Result:
> >  The message can be deleted 
> > 
> > Repro frequency:
> > 10/10,100%
> 
> can you verify the issue is fixed for you?

the issue is fine after using the  attachment 8551608 [details] [review], please land it on the v2.1, thanks

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/b2a78812c3286db099c11aa380cde525a84ab9a3

Comment 32

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

Comment on attachment 8551608 [details] [review]
gaia pull request (landed)

I am requesting v2.1 per the request by [e-mail] in comment 30 wherein its workingness was indicated.  Pasting the v2.2 request from comment 28 below:

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Complicated, but not really a regression.  We moved to use IMAP for 163.com/126.com with v2.1; prior to that autoconfig would result in ActiveSync being used, which was broken for different reasons.  IMAP would always have suffered from the lack of UIDNEXT support by the server even prior to v2.1, but the COPYUID status code parse failure was also new in v2.1.  The COPYUID parse failure will affect any batch modifications on any UIDPLUS server.
[User impact] if declined: Batch message deletion/moves won't work on IMAP servers supporting UIDPLUS.  All message deletion/moves won't work on 163.com and 126.com and other CoreMail servers.
[Testing completed]: Unit tests, testing by me against 163.com, testing by ffos_st against 163.com.
[Risk to taking this patch] (and alternatives if risky): Very low risk, especially with all our fancy testing!
[String changes made]: No string changes.

Comment 34

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

I also validated that the patch cherry-picks to v2.1 cleanly and have high confidence that it is also semantically safe to cherry-pick because it's orthogonal to everything else.

Comment 36

[bhavana bajaj [:bajaj]]

Comment on attachment 8551608 [details] [review]
gaia pull request (landed)

Approving given the partner verification and since it blocks their release.
:asuth mention this should apply cleanly! thanks for the dry run!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/999f21b8f546a7c20acdef7459511d8cf44c80b7