[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)

RESOLVED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::E-Mail
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ffos_st, Assigned: asuth)

Tracking

unspecified
2.2 S4 (23jan)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:-, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: sprd388951, PTR1_BLOCK)

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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

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

Comment 2

4 years ago
Btw, I'm using Apples iCloud E-Mail Service (andre.fiedler@me.com)
(Reporter)

Updated

4 years ago
Summary: [FFOS7715 v2.1] The email app cann't delete the message → [Email][FFOS7715 v2.1] The email app cann't delete the message
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
Flags: needinfo?(ffos_st)
also ni Andrew for further comment
Flags: needinfo?(bugmail)
(Assignee)

Comment 5

4 years ago
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

4 years ago
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.
Flags: needinfo?(ffos_st)
Flags: needinfo?(bugmail)
(In reply to ffos_st from comment #7)
> (In reply to Vance Chen [:vchen][vchen@mozilla.com] 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 : yifeibanxia@163.com
>           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!
Flags: needinfo?(bugmail)
(Assignee)

Comment 9

4 years ago
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.
Assignee: nobody → bugmail
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bugmail)
Summary: [Email][FFOS7715 v2.1] The email app cann't delete the message → [email/IMAP] moves broken on servers without UIDNEXT support, such as all CoreMail servers (ex: 126.com, 163.com)
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 2.2?
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
Flags: needinfo?(bugmail)
(Assignee)

Comment 11

4 years ago
Created attachment 8551600 [details] [review]
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.
Thanks for your promptly help Andrew. 

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

Thanks
Flags: needinfo?(yong.ren)
Flags: needinfo?(ffos_st)
(Assignee)

Comment 13

4 years ago
Created attachment 8551608 [details] [review]
gaia pull request (landed)
Flags: needinfo?(bugmail)
(Assignee)

Comment 14

4 years ago
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.
Attachment #8551600 - Flags: review?(m)

Comment 15

4 years ago
Okay, I will invite the ffos_st to test this patch, and reply quickly, thanks vance and Asuth. :)
Flags: needinfo?(yong.ren)

Updated

4 years ago
Whiteboard: sprd388951
(Reporter)

Comment 16

4 years ago
Hi:Yong Ren and  Vance:

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

thanks
Flags: needinfo?(ffos_st)
(Reporter)

Comment 17

4 years ago
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

4 years ago
(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.
Flags: needinfo?(vchen)
Flags: needinfo?(bugmail)
(Assignee)

Comment 19

4 years ago
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.
(Assignee)

Comment 20

4 years ago
Created attachment 8551734 [details] [review]
(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)
Flags: needinfo?(bugmail)
Thanks Andrew!

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

Thanks!
Flags: needinfo?(yong.ren)

Comment 22

4 years ago
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 on attachment 8551600 [details] [review]
GELAM patch with tests

lgtm.
Attachment #8551600 - Flags: review?(m) → review+
triage: not blocking but will seek 2.2 approval
blocking-b2g: 2.2? → -
(Reporter)

Comment 25

4 years ago
Hi: Vance and Yong Ren:

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

thanks
(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
Flags: needinfo?(yong.ren)
(Assignee)

Comment 28

4 years ago
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.
Attachment #8551608 - Attachment description: gaia/master pull request (test with this) → gaia pull request (landed)
Attachment #8551608 - Flags: review+
Attachment #8551608 - Flags: approval-gaia-v2.2?
Whiteboard: sprd388951 → sprd388951, PTR1_BLOCK

Updated

3 years ago
Attachment #8551608 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
(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

3 years ago
(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
v2.2: https://github.com/mozilla-b2g/gaia/commit/b2a78812c3286db099c11aa380cde525a84ab9a3
status-b2g-v2.2: affected → fixed
Target Milestone: --- → 2.2 S4 (23jan)
(Assignee)

Comment 32

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

I am requesting v2.1 per the request by yong.ren@spreadtrum.com 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.
Attachment #8551608 - Flags: approval-gaia-v2.1?
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1072322
(Assignee)

Updated

3 years ago
Summary: [email/IMAP] moves broken on servers without UIDNEXT support, such as all CoreMail servers (ex: 126.com, 163.com) → [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)
(Assignee)

Comment 34

3 years ago
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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1065247
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!
Attachment #8551608 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+

Updated

3 years ago
status-b2g-v2.1S: --- → fixed
(Assignee)

Updated

3 years ago
Blocks: 1083355
You need to log in before you can comment on or make changes to this bug.