838843 - [email] email search does not update as headers are modified or deleted, allows user to still see and fail to display deleted messages

Status: VERIFIED FIXED

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

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat

## Environment :
name="releases/mozilla-b2g18" path="gecko" revision="d0c9d7c63b36"
name="integration/gaia-v1-train" path="gaia" revision="39765fc9c0d0"
BuildID 20130206070201
Version 18.0
Unagi
  
## Repro :
1. setup email with gmail account
2. create an email Test to that account
3. do a search for Test
4. select the email and delete it
5. When back in the search select the email that you just deleted.
6. try to select it again

## Expected :
1. The email should be sent to the deleted folder and on app quit or loss of focus it should get deleted.

## Actual :
1. logcat error:
02-06 13:36:40.415: E/GeckoConsole(456): [JavaScript Error: "Error: Problem handling message type: gotBody TypeError: body is null 
02-06 13:36:40.415: E/GeckoConsole(456):  MessageReaderCard.prototype.buildBodyDom@app://email.gaiamobile.org/js/message-cards.js:1170
02-06 13:36:40.415: E/GeckoConsole(456): MessageReaderCard.prototype.postInsert@app://email.gaiamobile.org/js/message-cards.js:895
02-06 13:36:40.415: E/GeckoConsole(456): Cards.pushCard@app://email.gaiamobile.org/js/mail-common.js:423
02-06 13:36:40.415: E/GeckoConsole(456): gotBody@app://email.gaiamobile.org/js/message-cards.js:725
02-06 13:36:40.415: E/GeckoConsole(456): MailAPI.prototype._recv_gotBody@app://email.gaiamobile.org/js/ext/gaia-email-opt.js:2151
02-06 13:36:40.415: E/GeckoConsole(456): ma___bridgeReceive@app://email.gaiamobile.org/js/ext/gaia-email-opt.js:1954
02-06 13:36:40.415: E/GeckoConsole(456): createBridgePair/TMB.__sendMessage/<@app://email.gaiamobile.org/js/ext/gaia-email-opt.js:38069
02-06 13:36:40.415: E/GeckoConsole(456): handleMessage@app://email.gaiamobile.org/js/ext/gaia-email-opt.js:686
02-06 13:36:40.415: E/GeckoConsole(456): " {file: "app://email.gaiamobile.org/js/ext/gaia-email-opt.js" line: 1794}]



## Note :
1. email becomes inresponsive until you kill it and restart the app.
2. logging now as I see it in gmail, going to check hotmail

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat of not being able to get into hotmail after reboot.

logcat of not being able to get into hotmail.  Gmail is the first account which loads, hotmail is not loading properly.  It is keeping me at the Loading Messages progress screen.

I think the root cause came from the same issue.  The only other thing I can think of is that I have deleted an email with no subject...

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat: hotmail

Deleting the account and readding the hotmail account fixes it.
Ignore comment 1.  

Turns out that it's a separate issue and I will try to replicate.

Doing the steps in comment 0 gives the same results in hotmail.  logcat attached.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I should clarify: same result as in nothing happens when you tap it; no user feedback saying that the email is already deleted.  The email app continues to function when doing this to hotmail account.

Comment 4

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

There's basically 2 problems going on in here, with the breakage coming from point 2.

1) Search slices are effectively unknown to FolderStorage so they don't hear about changes to headers.  So when the deletion goes through, the slice doesn't hear about it.  This was a simplification related to the crash-landing of the search functionality.  Search slices at the minimum should be checked by the deletion events.  We also should have them checked for updated headers.  The key thing is that they should not be considered when deciding what blocks to evict from the cache because their time coverage can be very very large.  Added headers could be considered and run through the filter, but it seems very forgivable to make that a future enhancement.

2) We call eatEventsUntilNextCard before calling getBody.  getBody can return null because of situations like this, or less blatant situations simply related to the 2-generals problem.  We explode on the null return body.  Because we are now eating events, all clicks will be forever eaten.  Instead, we should detect the null body, abort eating the events and forget about popping a new card.  As a sanity-failsafe, we should probably also mark the message DOM node as collapsed/display: none so the user doesn't spend the rest of their life trying to click on the defunct message.  Note that if we do deferred body synchronization, that won't be affected because the call to getBody() will just take a while until we synchronize the body.  It might be polite to actually show a body card in that case with a spinner active, but we can address that when we get to deferred body sync.


This is definitely something we want to fix as a high priority.  Fixing #2 is sufficient to hide the problem, #1 is more of a nicety.  (Speculatively marking headers as collapsed as part of the deletion operation itself is a possibility, but will introduce edge cases when we resume supporting undo's on deletion, so we would want to be very careful about that.)

Comment 5

[Dylan Oliver [:doliver]]

triage: leo+ 

Note that bug 846970 was just fixed to address some deletion problems. Any chance that will have a positive effect on this bug?

Comment 6

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

(In reply to Dylan Oliver [:doliver] from comment #5)
> triage: leo+ 
> 
> Note that bug 846970 was just fixed to address some deletion problems. Any
> chance that will have a positive effect on this bug?

The problems are independent, although that bug could trigger more instances/variations of this bug once it caused us to deadlock and fail to release the mutex.

Comment 7

[Dominic Kuo [:dkuo]]

For this scenario, can we just also update the searched list after we deleted some messages?
like re-search again or force updating the searched list.

Comment 8

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

If you look in data/lib/mailapi/mailslice.js in the back-end, you will find a method deleteMessageHeaderAndBody.  Right now it goes through all known slices and generates onHeaderRemoved notifications for those slices.  We need to update that; see comment 4.  Probably by maintaining a list of _searchSlices and traversing them in that function too.

Comment 9

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: pull request

github: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/152

Comment 10

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

Comment on attachment 727620 [details]
pull request

Thanks for the patch!  I've made my review comments on the bug, so clearing the review flag.  I think :lightsofapollo's feedback was spot on and my multiplicity desire has been made, so let's have him do the follow-up review.

Comment 11

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

Er, my multiplicity desire has been made known.

Comment 12

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

WIP: https://github.com/yurenju/gaia-email-libs-and-more/commit/af56ef13fe51d4108fa0241044e6b89615fe199b

unit tests of gaia-email-libs isn't easy to understand, still studying. do we have any documents for unit test of gaia-email-libs? thank you.

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: WIP unit test patch

WIP patch for unit test, I want to add it to test_mutation.js but still working on it. I'll take vacation next week. feel free to take this bug!

Comment 14

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

re-assign to nobody since I need to focus on other bugs.

Comment 15

[Wayne Chang [:wchang]]

Andrew can you help here? this has stalled for a while.
I think we can focus on displaying the correct list after deletion and not worry so much about adb error if these are separate issues.

Thanks.

Comment 16

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

(In reply to Wayne Chang [:wchang] from comment #15)
> Andrew can you help here? this has stalled for a while.
> I think we can focus on displaying the correct list after deletion and not
> worry so much about adb error if these are separate issues.

This doesn't have a QE target associated, so until we dig out from under QE4, it's on the back burner.

Comment 17

[Alex Keybl [:akeybl]]

Triage: Not a regression, no partner blocker, no external feedback, no recent repros internally. Let's fix for 1.2 or later.

Will reconsider if any of that changes.

Comment 18

[Evan Tseng [:evanxd]]

Attachment: Errors of running unit test 1

Comment 19

[Evan Tseng [:evanxd]]

Attachment: Errors of running unit test 2

Comment 20

[Evan Tseng [:evanxd]]

Hi Andrew,

I have problems for running the unit test.

After I setup the environment of running unit test and run tests,
I got some errors. And you could see the errors in the below images.

1. https://bugzilla.mozilla.org/attachment.cgi?id=776999
2. https://bugzilla.mozilla.org/attachment.cgi?id=777000

Is the situation correct?
Or we could do something for running the unit test correctly?

Thanks. :)

Comment 21

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

(Evan and I addressed this on IRC.  Evan wins a prize for running into the least trouble (that I heard about :) when setting up dovecot/postfix!)

Comment 22

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

(re-titling to fold in our failure to notice flag changes so I can dupe a bug to this bug)

Comment 23

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

Evan, I assume you're not working on this anymore.  Please re-take if you are planning to.  I think :psingapati is interested in fixing these issues if you're busy, though.

Comment 25

[psingapati]

:asuth,
have verified with yurenju patch , delete is working fine,
have done similar modifications for starred/seen flags, added onHeaderModified in searchfilter for searchslices.

but  
splice.onchange is not executed in the method _processSliceUpdate
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/ext/mailapi/main-frame-setup.js#2610

does mailapi this_slices{}  also handles searchSlices?

Please provide information.

Comment 26

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

Yes.  FoldersStorage.updateMessageHeader is the method we use to update headers:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/mailslice.js#L3702

You will see that it walks self._slices to see if the header being updated is in the slice (or more specifically, should be in the slice based on our invariant).  If it is, we send an update for it via the call to slice.onHeaderModified.

Comment 27

[psingapati]

Attachment: Work in Progress PR

The work in progress PR is raised for https://bugzilla.mozilla.org/show_bug.cgi?id=838843

Unable to cherry-pick from yurenju github due to conflicts, have done manual merge.
Added code for updating search headers on modtags.
Currently facing intermittent errors in updating flags/move or delete. Possible error, not able to generate idx at 
https://github.com/psingapati/gaia-email-libs-and-more/blob/Bug_838843_SearcSlice_Flags_Delete_Move_Operations/data/lib/mailapi/searchfilter.js#L674

Please review and give your comments

Comment 28

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

I started to write unit-tests for this to assist me in the review and then it snowballed into cleaning up the search slice implementation.  Stealing.  Pull request up momentarily.

Comment 29

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

Attachment: cleaned up search slice with add/change/remove handled plus unit tests

Note that you might also just want to review https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/297 which also has the html body search fix for bug 882917 in the same pull request.

Note that running Gaia with these fixes applied will still be somewhat sad-looking because of bug 989116 where the non-search message_list is going to get confused by the search slice and will throw a ton of errors involving listPerson because it will try and use the MailMatchedHeader like it is a MailHeader.  In practice these errors should be harmless.  You will want to run with the search-HTML changes too though as otherwise you're going to see a ton of the document.implementation errors.

Comment 30

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8399512 [details] [review]
cleaned up search slice with add/change/remove handled plus unit tests

(same summary as the superpatch review in bug 882917)

Comment 31

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

Landed separately from the bug 882917 bit because the changes are sufficiently decoupled and it's hopefully slightly less confusing this way.

landed in GELAM/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/296
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/ec62aa283bbd846c0f615ec9ad42f6cbd9d252f6

landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/17927
https://github.com/mozilla-b2g/gaia/commit/b9cc06d3f503dcc03cdd6f5d2f414696dd5d6dc3

Comment 32

[psingapati]

Cherry-picked commit into v1.3 , issue is working fine.
Can you please request to uplift into v1.3

Comment 33

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

(In reply to psingapati from comment #32)
> Cherry-picked commit into v1.3 , issue is working fine.
> Can you please request to uplift into v1.3

I'm not sure this will make our blocking criteria since it was never actually a regression.  needinfo'ing :doliver for clarity.  :psingapati, if you could indicate if you're planing to ship this fix even if we don't uplift it into the official Mozilla 1.3 tree, that might impact triage decisions since it's generally preferable to minimize such deviation.

In general, this change and its HTML-search-fixing friend bug 882917 are safe and and search is broken-ish without them, so we could do much worse than uplifting the fixes.

Comment 34

[Dylan Oliver [:doliver]]

Since this bug does not meet our blocking criteria, we do not plan to uplift this to the 1.3 branch. We want as little change as possible in the 1.3 code at this point.

Comment 35

[Edward Chen[:edchen]]

[Environment]
Gaia      553da99ab09b6b894d9f95bb06b16b6e1ddbf0a1
Gecko     https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf
BuildID   20140414160205
Version   31.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

[Result]
PASS

Comment 36

[James Burke [:jrburke]]

Asking formally for 1.4+ on this bug as it will be uplifted because the fix is depended on by bug 989116, a 1.4+ blocker, and that bug will be marked fix once the changeset for bug 796474, another 1.4+ bug fix is uplifted.

Comment 37

[Dylan Oliver [:doliver]]

1.4+ per comment #36

Comment 38

[James Burke [:jrburke]]

Fixed on v1.4 branch:
https://github.com/mozilla-b2g/gaia/commit/81e97c3ca58be0487292011bc59efa4cebab30be

from pull request:
https://github.com/mozilla-b2g/gaia/pull/18808

That pull request was a roll up commit of the following bugs, since bug 796474, a 1.4+ bug, depended on changes in the other bugs, and the other bugs also block another 1.4 bug, bug 989116.

Commits that were part of the roll up:

bug 838843: fc4a74a8400838e5fd18da6b7d8851a5a4380019
bug 882917: 0a8ccfdb26a33789bec754d769b0786570ceb28c
bug 796474: 67868019817334815ebb881ef8cd1b478989aa01