925961 - [B2G][Email] Receiver's email address displays as "null" after saving, editing, and then saving the draft again

Status: RESOLVED FIXED

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

Description

[Matthew Vaughan]

Attachment: LogCat for saving a draft, editing it, and saving it again.

Description:
The receiver's email address can end up displaying as the word "null" when a user saves a draft locally, goes back in to edit it, and then saves it as a draft again. The user will see "null" as the receiver in the Local Drafts list and when they enter the draft to edit it further.

Repro Steps:
1) Update Buri to BuildID: 20131011004001
2) Launch the Email app
3) Set up an email account on the phone
4) Tap the Create New Email icon (pencil in a box)
5) Enter at least a receiver email address
6) Tap the Back button and select "Save to Local Drafts"
7) Select the email in the Local Drafts list
8) Tap the Back button again and select "Save to Local Drafts"
9) Observe receiver's email address in the Local Drafts list
10) Enter the email again

Actual:
Receiver's email address displays as "null".

Expected:
Receiver's email address remains exactly as the user entered it.

Environmental Variables:
Device: Buri
BuildID: 20131011004001
Gaia: 79abf09f2b5b6440f43cb5ae44ef6c85c0437e8d
Gecko: c8e97fd5b94d
Version: 26.0a2

Notes:
Repro frequency: 100%
See attached: NullInDraftsList.png ; NullInCompose.png ; logcat.txt

Comment 1

[Matthew Vaughan]

Attachment: Screenshot of null receiver while composing an email.

Comment 2

[Matthew Vaughan]

Attachment: Screenshot of null in the Local Drafts list.

Comment 3

[Matthew Vaughan]

This bug is not occurring in the 10/11/13 1.1 build. The receiver's email address remains exactly as the user entered it.

Environmental Variables:
Build ID: 20131011041214
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/cb2a1a27a94c
Gaia: 680f3b86b1e4ff1411ece6ba397b8b0e56b4b31c
Platform Version: 18.1

Comment 4

[Sarah Parsons]

This is a regression from 1.1, it does not reproduce on 1.1.

This issue reproduces on all Buri 1.2 builds starting from Build ID: 20130621031231

Gaia   e2f19420fa6a26c4287588701efaec09a750dba1
SourceStamp 7ba8c86f1a56
BuildID 20130621031231
Version 24.0a1

I've tested several 1.2 builds including 10/11, 10/02, 9/18, 7/11, 7/02, and 6/21 all of which reproduce the issue.

Comment 5

[Dylan Oliver [:doliver]]

triage: koi+ for regression

Comment 6

[Evan Tseng [:evanxd]]

Attachment: PR for Email App

Hi James,

Could you give me feedback on the patch?
And I'm writing the integrations tests for this bug.

Thanks.

Comment 7

[James Burke [:jrburke]]

Comment on attachment 820925 [details] [review]
PR for Email App

Evan, I would be curious to know more on why the dataSet.name gets set to a string with value 'null' where on the first edit, this is not the case. My first thought would be to avoid that data conversion from occurring.

Comment 8

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

This is almost certainly related to the contact resolution process and something ugly happening with the round-tripping of that.  It's definitely a code smell if we ever check for the already-stringified version of a special type like "null"/"undefined", etc.

Comment 9

[Evan Tseng [:evanxd]]

Comment on attachment 820925 [details] [review]
PR for Email App

Hi James,

Because of this line https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/compose.js#L382, the dataSet.name is set by a string value as 'null' at first time.

We could check the value of the name variable in createBubbleNode function https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/compose.js#L268, and we could set a valid value to dataSet.name.

I updated the patch for the above. How do you think? Please give me feedback on it.
Thanks. :)

Comment 10

[Evan Tseng [:evanxd]]

Comment on attachment 820925 [details] [review]
PR for Email App

Hi Andrew,

We could check the value of the name variable in createBubbleNode function https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/compose.js#L268, and we could set a valid value to dataSet.name.

How do you think?
Thanks. :)

Comment 11

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

Comment on attachment 820925 [details] [review]
PR for Email App

I added some comments on the pull request.  In general, I would like for us to avoid hard-coding message/folder indexes since that can lead to brittle tests.  Much better to use folder names or types, and to identify messages based on their subjects.

In terms of the nulls, I think I'd like for us to try and address the technical debt we've got related to this.  The line you point out has a TODO for us to parse e-mail addresses in that code.  I think we should fix that now.  We can have mailapi/mailapi.js require 'addressparser', and then re-expose that functionality so that compose.js can properly parse the e-mail address.

I would suggest that we add a method call parseMailbox to the MailAPI instance that calls the parse function and returns the {name, address} object we expect.  (We could potentially create a convenience function like resolveMailboxToPeep that under the hood runs the parse function and then calls resolveEmailAddressToPeep too.)  Once we do this, we'll also want to have the integration test type in a full '"John Example" <[e-mail]>"' E-mail address too to make sure the parser is hooked up right.

And we should keep your change to not set the name if it's null/falsey, of course.

Comment 12

[James Burke [:jrburke]]

Comment on attachment 820925 [details] [review]
PR for Email App

Switching off feedback flag as :asuth outlined what I was trying to get at in previous comment: finding the root of the 'null' strings.

Comment 13

[Evan Tseng [:evanxd]]

Comment on attachment 820925 [details] [review]
PR for Email App

Hi Andrew,

I already updated the patch for your comments.
Please help me review the patch.
Thanks. :)

Comment 14

[Evan Tseng [:evanxd]]

Hi Andrew,

Because the deadline of the bug is almost reached, could we skip the TODO thing in this bug? We could file a new bug for that.

(Actually I already have patch included integration test for the TODO thing for Gaia part. But I have no time for GELAM part. https://github.com/evanxd/gaia/commit/06654513c2f2f66ae7a0bad65696121ff60c3bc9)

Comment 15

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

Comment on attachment 820925 [details] [review]
PR for Email App

Thanks for the test updates; I think they're a great improvement.  There's a small fix to be made regarding the folder type selector, but otherwise this looks good.  However, I do want us to deal with the TODO right now since, as discussed on IRC, I don't think there's any major deadline coming up.  And also, I think you're basically there.  I think if you add the addressparser dependency into mailapi/mailapi.js in GELAM and copy the other code you listed in there, I think the build step will probably just do what we need.  No tricks with _addressparser should be required.

I'm needinfo-ing James Burke in case he has any advice about the build process for GELAM; I'm not sure if it will get confused by us having a file that is loaded by both the main-page JS and the worker JS.  This should also be somewhat obvious if when you do "make install-into-gaia" it tries to remove addressparser from the file already loaded by the back-end.

Comment 16

[Evan Tseng [:evanxd]]

Hi Andrew,

Thanks for the comments in GitHub. I also updated for that.

The target milestone of the bug is set as 1.3 Sprint 3 - 10/25.
So I think the deadline is 10/25.

Comment 17

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

(In reply to Evan Tseng [:evanxd] from comment #16)
> The target milestone of the bug is set as 1.3 Sprint 3 - 10/25.
> So I think the deadline is 10/25.

Not a deadline, just an estimate of when it will be completed.  Also, I talked with Dylan we actually do our sprints from wednesday to wednesday, so 10/25 is really more like 10/30 for our team.  If you don't think we can finish it for 10/30, we can move it out to the next sprint.

Comment 18

[Evan Tseng [:evanxd]]

Attachment: PR for GELAM

Hi Andrew,

Please help me to review the patch.
Thanks.

Comment 19

[Evan Tseng [:evanxd]]

Comment on attachment 820925 [details] [review]
PR for Email App

Hi Andrew,

Yes, I think we could finish it before 10/30.
Please help me to review the patch.
Thanks.

Comment 20

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

Comment on attachment 822716 [details] [review]
PR for GELAM

Almost there; a few minor comments,the main one being about the manually inserted define() call in mailapi.js which should not be there.  I think we'll be good to go with that fixed.  Please do request review again on this patch and the gaia one so I can double-check that the install-into-gaia stuff went correctly.

Also, when requesting review, you can just request review with the "review?" flag.  I appreciate your polite request, but unless you have some specific comments to make about the patch, there's no need for any comment in the review request as I will still get 2 emails about it either way.

Comment 21

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

Comment on attachment 820925 [details] [review]
PR for Email App

(this looks good but I want to see the revised GELAM changes in the pull request at the same time.  And it looks like jrburke's input is not needed.)

Comment 22

[Evan Tseng [:evanxd]]

Comment on attachment 820925 [details] [review]
PR for Email App

Hi Andrew,

I updated the patch for your comments.
Please help me to review the patch.
Thanks.

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 822716 [details] [review]
PR for GELAM

Hi Andrew,

I updated the patch for your comments.
Please help me to review the patch.
Thanks.

Comment 24

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

Comment on attachment 820925 [details] [review]
PR for Email App

Looks good, thanks!

Comment 25

[Evan Tseng [:evanxd]]

Hi Andrew,

I don't why some unit tests always failed for my GELAM patch.
Please see https://travis-ci.org/mozilla-b2g/gaia-email-libs-and-more/builds/13245560.

The patch seems not be related with these failed tests.
Do you know why are they failed?

Thanks.

Comment 26

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

The tests pass locally for me when using an older b2g-desktop build, so I think you're fine to land.  We will need to investigate why the tests fail/how the tests are failing, but that does not need to block this bug right now.  In some other bugs I'm looking into b2g-desktop crashers which are also complicating the whole testing story.

The primary way to investigate will be to implement bug 910066 or something along those lines.

Comment 27

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

Also, I did have travis re-run the build several times, and the set of tests changed.  In general, I think we may be looking at new failures due to timing changes now that Mozilla has our own bunch of Travis VMs.

Comment 28

[Evan Tseng [:evanxd]]

Email App master: https://github.com/evanxd/gaia/commit/e363d38cde26a6416cb9926c04896866bba4b88a

GELAM masater: https://github.com/evanxd/gaia-email-libs-and-more/commit/965770e330c18b0012dc5b1bf17ebacf5bd34d07

Comment 29

[Evan Tseng [:evanxd]]

Hi Andrew,

Got it. Thanks.

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted e363d38cde26a6416cb9926c04896866bba4b88a to:
v1.2: 60a69a3508a6127c61ed8701667424011b08d972

Comment 31

[Anthony Ricaud (:rik)]

I had to revert this because of a very frequent intermittent.

v1.2: cb981e2f47bc644b4d178d54378c3676c946facf
master: 00cd7534afb8d727531c7a939e54e382b3be713a

1) go to local drafts page should show the correct email address in a item of mail list:
NoSuchElement: (7) Unable to locate element: .card-compose .cmp-back-btn
Remote Stack:
<none>
at Error.MarionetteError (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)
at Object.Client._handleCallback (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:472:19)
at /home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:506:21
at TcpSync.send (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:94:10)
at Object.send (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:453:36)
at Object.Client._sendCommand (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:499:19)
at Object._findElement (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:1281:19)
at Object.findElement (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-client/lib/marionette/client.js:1330:32)
at Object.MarionetteHelper.waitForElement (/home/travis/build/mozilla-b2g/gaia/node_modules/marionette-helper/index.js:126:19)
at Object.Email._waitForElementNoTransition (/home/travis/build/mozilla-b2g/gaia/apps/email/test/marionette/lib/email.js:421:31)
at Object.Email.saveLocalDrafts (/home/travis/build/mozilla-b2g/gaia/apps/email/test/marionette/lib/email.js:233:10)
at Context.<anonymous> (/home/travis/build/mozilla-b2g/gaia/apps/email/test/marionette/local_drafts_test.js:40:9)
at Test.Runnable.run (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:211:32)
at Runner.runTest (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:355:10)
at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:401:12
at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:281:14)
at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:290:7
at next (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:234:23)
at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runner.js:253:7
at done (/home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:185:5)
at /home/travis/build/mozilla-b2g/gaia/node_modules/mocha/lib/runnable.js:197:9
at /home/travis/build/mozilla-b2g/gaia/apps/email/test/marionette/lib/servers/fakeimap.js:73:7
at /home/travis/build/mozilla-b2g/gaia/node_modules/mail-fakeservers/lib/control_server.js:58:7
at IncomingMessage.<anonymous> (/home/travis/build/mozilla-b2g/gaia/node_modules/mail-fakeservers/lib/control_request.js:30:7)
at IncomingMessage.g (events.js:175:14)
at IncomingMessage.EventEmitter.emit (events.js:117:20)
at _stream_readable.js:910:16
at process._tickCallback (node.js:415:13)

Comment 33

[Evan Tseng [:evanxd]]

Hi Anthony,

First, I ran the test more than 10 times in local,
and we could not reproduce the error.

Was the error happened in TPBL or Travis intermittently?
And how often did the error happen roughly?

Thanks.

Comment 34

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

I think :jrburke was working with :gaye at the end of last week to try and get helper functions a chance to run in failure cases so in e-mail we can better understand these intermittent failures.

Comment 35

[Evan Tseng [:evanxd]]

Hi Andrew,

Yes, jrburke is working on a stuff let us get more log info for the failures.
He might done that tomorrow.

At the same time, I'll try to update the test code for fixing the issue. I had see the same kind of the error message before.

Hopefully we could fix the issue at this time. Otherwise we might disable the test first and land the patch. Then we could file a new bug for the issue.

How do you think?

Comment 36

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

(In reply to Evan Tseng [:evanxd] from comment #35)
> At the same time, I'll try to update the test code for fixing the issue. I
> had see the same kind of the error message before.

This sounds good.  Remember that you can get Travis to re-run test runs (if you are logged in to Travis, at least), and in your pull requests you can change things so only your test is run, etc., if that helps you narrow down the problem.
 
> Hopefully we could fix the issue at this time. Otherwise we might disable
> the test first and land the patch. Then we could file a new bug for the
> issue.
> 
> How do you think?

I agree that it makes sense to land with the test disabled if we can't figure out the test problem soon, but since we're pretty bad about dealing with our backlog, let's investigate first and work on our failure logging because I expect all of our integration tests are going to experience problems like these.

Comment 37

[Evan Tseng [:evanxd]]

Hi all,

We disable the test first, and we follow the issue in http://bugzil.la/936328.

Comment 38

[Evan Tseng [:evanxd]]

master: https://github.com/mozilla-b2g/gaia/commit/37ad831cfde9e5b88f3e330e3a4e47b7df85b41c

Comment 39

[James Burke [:jrburke]]

Late on this, but to clear the neeedinfo: agreed on disabling the test -- the additional logging added today for timeout errors does not seem to be working anyway, so we likely will not have it figured out soon.

Comment 40

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 37ad831cfde9e5b88f3e330e3a4e47b7df85b41c to:
v1.2: 5eff7b00f3acaef6144d050a357b6d33e46807a9

Comment 41

[Jason Smith [:jsmith]]

The uplift here broke sending of emails.

Andrew - Can you back this out of 1.2 & investigate why the uplift caused a smoketest regression?

Comment 42

[Jason Smith [:jsmith]]

Actually - this is only patch that landed in relation to email in the regression range, which definitely makes this the root cause of the regression.

John - Can you back this out of 1.2 specifically?

Comment 43

[John Ford [:jhford] CET/CEST Berlin Time]

[v1.2 20e8d40] Revert "Merge pull request #13338 from evanxd/bug-925961"

Comment 44

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

Evan, can you prepare a pull request for v1.2?  Manually applying the revised main-frame-setup.js hunk from your initial landing in addition to your revised patch (or mcav's bleach.js fix landing that accidentally brought it into Gaia) should get us where we need to go.  If you could test on device and then ask review from myself and :jrburke and one of us will sanity check, that would be ideal.  Thanks!

Comment 45

[Evan Tseng [:evanxd]]

Attachment: PR for Email App for v1.2

Hi Andrew,

Please help me to review the patch.

Comment 46

[Evan Tseng [:evanxd]]

Comment on attachment 832064 [details] [review]
PR for Email App for v1.2

Hi James,

Please help me review the patch.
Thanks.

Comment 47

[James Burke [:jrburke]]

Comment on attachment 832064 [details] [review]
PR for Email App for v1.2

Compared the 1.2 diff with the two pushes that were done before, and it all is the same, this one for 1.2 also includes the gelam change in main-frame-setup.js. Which is what we want for 1.2 to work. If the marionette tests for email also pass locally on the branch, this should be good to merge. Travis for this pull request is also green.

If you can also be sure test it on a phone to make sure the bug is fixed, that is appreciated.

Comment 48

[Evan Tseng [:evanxd]]

Hi James,

Sure, the patch is passed the integration test locally, and it also works on a phone.
Thanks for your review.

Evan

Comment 49

[Evan Tseng [:evanxd]]

v1.2 branch: https://github.com/mozilla-b2g/gaia/commit/a1a66c8e08c8cb55697f0cb24b80fdbcae0e491e