1178242 - Notes app can't sign in to Evernote

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Build, defect, P3)

Description

[Ben Francis [:benfrancis]]

User-Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Notes app preloaded on Spark devices has Evernote integration but authorization shows a network error

Comment 1

[Peter Dolanjski [:pdol]]

[Blocking Requested - why for this release]: Broken core functionality.

Comment 2

[Dylan Oliver [:doliver]]

The connection to Evernote seems to happen succesfully (in that I get the notification email from Evernote to confirm that I have granted account access to a new device) but there seems to be an application error in the handling of the message that comes back which prevents it from completing the data exchange.

Comment 3

[Dylan Oliver [:doliver]]

Attachment: evernote-failure.png

Screenshot of failure.

Comment 4

[Dylan Oliver [:doliver]]

This looks to be something wrong with the Notes build that is included in the Spark distro. If I delete the one from the Aries build and then re-install directly from Marketplace, I don't run into the error. It also works fine when building from scratch.

Downgrading to P3 as this looks to be config related and there is a workaround (though admittedly not a great workaround as you'd have to re-create your existing local notes if you remove and reinstall the app).

Comment 5

[Dylan Oliver [:doliver]]

Naoki, do you know where the work was done to pull Notes into the Spark build? I assumed it was pulling it from Marketplace but then I'm not sure why we'd see different behavior when re-installing.

Comment 6

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

https://github.com/mozilla-b2g/gaia/blob/master/distros/spark/remote.list#L6

Comment 7

[Dylan Oliver [:doliver]]

> notes,https://marketplace.firefox.com/app/dcdaeefc-26f4-4af6-ad22-82eb93beadcd/manifest.webapp

It looks like the remote list is pulling in the right version, so I'm not sure what's up. Perhaps there is some difference in the installation process that happens from remoting vs. directly from Marketplace?

Comment 8

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

That's possible.  There was an issue where Vaani had a locales folder and that caused a conflict where it wasn't pushing in the folder and strings were missing.

I would have to have time to investigate further in regards to the difference.  It may be easier to have someone like DRS investigate instead; though... he's pretty busy too...

Comment 9

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

It looks like the app files are the exact same.  
I think it has something to do with the way that the apps are registered.

I did a diff between the zip files and also the update.webapp after pulling both a working version and a non working version.

I think we're missing something in terms of the app install registration for the distros.

Comment 10

[Dylan Oliver [:doliver]]

Moving this into Build component as a more likely area based on comments 4-9. I do think this is a regression as we've had a number of operators use Notes as a pre-install and I haven't seen any reports of this problem prior to the Spark build.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sounds like a incomplete integration where :drs could help.

Comment 12

[Francisco Jordano [:arcturus] [:francisco]]

Seems the redirection for oauth is not working when we have a preloaded app.

ni fabrice.

Comment 13

[[:fabrice] Fabrice Desré]

Francisco, can you attach the content of /data/local/webapps/webapps.json ?

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: webapps.json

Attaching the webapps.json

Regarding the notes app, I cannot see in the webapps.json the 'redirects' field, but checking /data/local/webapps/{88c463ad-fed7-f54c-adc5-348398f80f51}/manifest.webapp I can see the redirects field:

"redirects": [
    {
      "from": "http://ffos-notes.local/redirect.html",
      "to": "/redirect.html"
    }
  ]

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

IIRC, the redirects field should not be in /data/local/webapps.json

Comment 16

[Francisco Jordano [:arcturus] [:francisco]]

Also checking the webapps.json, the appStatus for {88c463ad-fed7-f54c-adc5-348398f80f51}, is 1, which is installed, not privileged

So then makes sense that the registry is not informed about the redirects:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#450

Actually, reviewing the webapps.json, we have more appStatus with value 1 when it should be 2 (privileged). And those apps are the ones downloaded with the distro.

So if i'm not wrong (that lately is happening quite often), this is not just  a problem for the notes app, but any app distributed that makes use of a privileged api.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

see [1]: if the metadata file exists, we need to put the type in it. Which is not the case for the preloaded files.

[1] https://github.com/mozilla-b2g/gaia/blob/master/build/utils-xpc.js#L383-L390

Moreover, we were not reading the manifest.webapp from the downloaded zip file.

So here is a first try but I'm not really sure it's working properly. Could someone test it with Notes and other apps ?

Comment 19

[Julien Wajsberg [:julienw]]

At least the generated webapps.json has "appStatus: 2" for the Notes app, while it was "1" before as Francisco found out.

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> At least the generated webapps.json has "appStatus: 2" for the Notes app,
> while it was "1" before as Francisco found out.

That's a good sign since we don't honor redirects in non privileged apps.

Comment 21

[Francisco Jordano [:arcturus] [:francisco]]

Just tested Julien patch via:

GAIA_DISTRIBUTION_DIR=distros/spark  make reset-gaia

the registry is looking good and I was able to sync with a test account from evernote.

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

Hey Dale,

is it something you can review ? That's my first python lines ever (actually I mostly copy pasted stuff), and there is no test, so what could go wrong ?

Note I wouldn't be upset if you decide to rewrite it ;)

Comment 23

[Ben Francis [:benfrancis]]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

Sorry but I don't think I can provide useful feedback on this patch.

Comment 24

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

This should be upstreamed here, where the code originally came from:
https://github.com/mozilla-b2g/preload-app-toolkit

I don't know anything about how it works, so I can't help with suggestions or reviews.

Comment 25

[Julien Wajsberg [:julienw]]

Ben, I was merely asking you to test the patch because you were the original reporter.

Doug, thanks for the pointer, I wasn't aware.

Comment 26

[Dale Harvey (:daleharvey)]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

I am not an expert at this part of code, one suggestion I would make would be to do the pull request against https://github.com/mozilla-b2g/preload-app-toolkit and copying across preload.py as a whole.

But testing it works, makes sense so happy putting a +1 on it.

Comment 27

[Julien Wajsberg [:julienw]]

Thanks, I'll do that !

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Here is a new approach which is IMO less risky.

I'm not confident enough with the preload.py script and while trying to write some tests for it I felt that I was maybe breaking things... Current cases are not well-tested :/

So here is another approach which may also makes more sense. We already have all the necessary information, we just need to properly get it in the build script.

Hey Ricky, what do you think ? Also I would appreciate if you can point me to a unit or integration test I could change for this.

Francisco, I would also appreciate that you test this new patch from a clean master (so without any leftover from downloads using the previous patch). From my test "appStatus" seems to be correct but I'd like a double-check by really testing on the device.

Thanks !

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

FTR here is my WIP for preload-app-toolkit:

https://github.com/mozilla-b2g/preload-app-toolkit/compare/master...julienw:1178242-properly-preload-privileged-apps?expand=1

We may want to get the change to unit tests. But with this patch I realized we don't output properly update.webapp in the end of the process. This made me too afraid to land this change and breaking things at this stage.

Comment 31

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Forward to Fred since he is peer of preload-app tool.

Comment 32

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Sorry I made a mistake, path seems good to me.

Comment 33

[Ricky Chien [:rickychien] (inactive)]

We don't have well protected utils tests actually, it would be great if you would like to add a simple unit test for that. (see also build/test/unit/utils_test.js)

Comment 34

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Tried to build the distribution with this patch and worked well. Could connect with evernote and import stuff.

Great job Julien.

Comment 35

[Julien Wajsberg [:julienw]]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

Hey Ricky,

I wrote one test that currently fails on master but works with the patch. Maybe you can have a quick look on it ?

Thanks !

Comment 36

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8680041 [details] [review]
[gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master

r+ again, after applying your patch it passed on my Mac locally, I didn't see any failures. 

Are you still seeing failures somewhere? Test result on gaia-try looks great for me.

Comment 37

[Julien Wajsberg [:julienw]]

(In reply to Ricky Chien [:rickychien] from comment #36)
> Comment on attachment 8680041 [details] [review]
> [gaia] julienw:1178242-fetch-type-from-zip-manifest > mozilla-b2g:master
> 
> r+ again, after applying your patch it passed on my Mac locally, I didn't
> see any failures. 
> 
> Are you still seeing failures somewhere? Test result on gaia-try looks great
> for me.

You misunderstood my comment :) I made the new unit test especially to fail on master, but pass with my patch :)

Note that it tests only utils-node.js, not utils-xpc.js which we really use while building... so I'm not sure it's that valuable currently. But better than nothing, I guess ?

Comment 38

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/0036c757d418347afc9f252b2766b88e283848bf

Thanks for the tests and reviews !

Comment 39

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

verified on a week old build.

Build ID               20151102192757
Gaia Revision          bfcf8e9bfa7ba264c5f8bc865ce8a435f9b134cb
Gaia Date              2015-11-02 06:43:44
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c534d279756291c0ba8ff5bdd4baa35cf56c9e4d
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150527.043015
Firmware Date          Wed May 27 04:30:24 EDT 2015
Bootloader             L1TC000118D0

Comment 40

[Dylan Oliver [:doliver]]

Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

Looks good, sync is back in business.