Closed
Bug 838843
Opened 12 years ago
Closed 11 years ago
[email] email search does not update as headers are modified or deleted, allows user to still see and fail to display deleted messages
Categories
(Firefox OS Graveyard :: Gaia::E-Mail, defect)
Tracking
(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)
People
(Reporter: nhirata, Assigned: asuth)
References
Details
(Whiteboard: [c= p=3])
Attachments
(4 files, 5 obsolete files)
## 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
Reporter | ||
Comment 1•12 years ago
|
||
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...
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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.)
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Comment 5•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Updated•12 years ago
|
tracking-b2g18:
? → ---
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee: nobody → yurenju.mozilla
Comment 9•12 years ago
|
||
Attachment #727620 -
Flags: review?(bugmail)
Assignee | ||
Comment 10•12 years ago
|
||
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.
Attachment #727620 -
Flags: review?(bugmail)
Assignee | ||
Comment 11•12 years ago
|
||
Er, my multiplicity desire has been made known.
Comment 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
re-assign to nobody since I need to focus on other bugs.
Assignee: yurenju.mozilla → nobody
Updated•12 years ago
|
Whiteboard: c=
Updated•12 years ago
|
Whiteboard: c= → c=
Updated•11 years ago
|
Assignee: nobody → doliver
Updated•11 years ago
|
Whiteboard: c= → c= LOE:2d
Comment 15•11 years ago
|
||
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.
Flags: needinfo?(bugmail)
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Flags: needinfo?(bugmail)
Updated•11 years ago
|
Assignee: doliver → evanxd
Comment 17•11 years ago
|
||
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.
blocking-b2g: leo+ → -
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
Comment 20•11 years ago
|
||
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. :)
Flags: needinfo?(bugmail)
Assignee | ||
Comment 21•11 years ago
|
||
(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!)
Flags: needinfo?(bugmail)
Assignee | ||
Comment 22•11 years ago
|
||
(re-titling to fold in our failure to notice flag changes so I can dupe a bug to this bug)
Summary: [email] email search still shows deleted email and causes an error in logcat when trying to display the deleted email → [email] email search does not update as headers are modified or deleted, allows user to still see and fail to display deleted messages
Assignee | ||
Comment 23•11 years ago
|
||
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.
Assignee: evanxd → nobody
Comment 25•11 years ago
|
||
: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.
Flags: needinfo?(bugmail)
Assignee | ||
Comment 26•11 years ago
|
||
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.
Flags: needinfo?(bugmail)
Comment 27•11 years ago
|
||
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
Attachment #8396243 -
Flags: review?(bugmail)
Updated•11 years ago
|
Assignee: nobody → psingapati
Updated•11 years ago
|
Attachment #8396243 -
Attachment description: index.html → Work in Progress PR
Assignee | ||
Comment 28•11 years ago
|
||
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.
Assignee: psingapati → bugmail
Status: NEW → ASSIGNED
Whiteboard: c= LOE:2d → [c= p=3]
Target Milestone: --- → 1.4 S5 (11apr)
Assignee | ||
Comment 29•11 years ago
|
||
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.
Attachment #727620 -
Attachment is obsolete: true
Attachment #731132 -
Attachment is obsolete: true
Attachment #776999 -
Attachment is obsolete: true
Attachment #777000 -
Attachment is obsolete: true
Attachment #8396243 -
Attachment is obsolete: true
Attachment #8396243 -
Flags: review?(bugmail)
Attachment #8399512 -
Flags: review?(m)
Comment 30•11 years ago
|
||
Comment on attachment 8399512 [details]
cleaned up search slice with add/change/remove handled plus unit tests
(same summary as the superpatch review in bug 882917)
Attachment #8399512 -
Flags: review?(m) → review+
Assignee | ||
Comment 31•11 years ago
|
||
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•11 years ago
|
||
Cherry-picked commit into v1.3 , issue is working fine.
Can you please request to uplift into v1.3
Flags: needinfo?(bugmail)
Assignee | ||
Comment 33•11 years ago
|
||
(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.
Flags: needinfo?(bugmail) → needinfo?(doliver)
Comment 34•11 years ago
|
||
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.
Flags: needinfo?(doliver)
Comment 35•11 years ago
|
||
[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
Status: RESOLVED → VERIFIED
Comment 36•11 years ago
|
||
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.
blocking-b2g: - → 1.4?
Comment 37•11 years ago
|
||
1.4+ per comment #36
blocking-b2g: 1.4? → 1.4+
status-b2g-v1.4:
--- → affected
Comment 38•11 years ago
|
||
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
Updated•11 years ago
|
status-b2g-v2.0:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•