Closed Bug 1178242 Opened 7 years ago Closed 7 years ago

Notes app can't sign in to Evernote

Categories

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

defect

Tracking

(blocking-b2g:2.5+)

VERIFIED FIXED
blocking-b2g 2.5+

People

(Reporter: benfrancis, Assigned: julienw)

References

Details

(Keywords: foxfood, Whiteboard: [bzlite])

Attachments

(4 files)

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
[Blocking Requested - why for this release]: Broken core functionality.
blocking-b2g: --- → 2.5?
Component: Gaia::Feedback → Gaia::Notes
QA Whiteboard: [foxfood-triage]
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.
blocking-b2g: 2.5? → 2.5+
Priority: -- → P1
Attached image evernote-failure.png
Screenshot of failure.
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).
Priority: P1 → P3
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.
Flags: needinfo?(nhirata.bugzilla)
> 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?
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...
Flags: needinfo?(drs)
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.
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.
Component: Gaia::Notes → Gaia::Build
Sounds like a incomplete integration where :drs could help.
Seems the redirection for oauth is not working when we have a preloaded app.

ni fabrice.
Flags: needinfo?(fabrice)
Francisco, can you attach the content of /data/local/webapps/webapps.json ?
Flags: needinfo?(fabrice)
Attached file 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"
    }
  ]
Flags: needinfo?(fabrice)
IIRC, the redirects field should not be in /data/local/webapps.json
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 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 ?
Attachment #8678283 - Flags: feedback?(doliver)
Attachment #8678283 - Flags: feedback?(bfrancis)
At least the generated webapps.json has "appStatus: 2" for the Notes app, while it was "1" before as Francisco found out.
(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.
Flags: needinfo?(fabrice)
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 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 ;)
Attachment #8678283 - Flags: review?(dale)
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.
Attachment #8678283 - Flags: feedback?(bfrancis)
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.
Flags: needinfo?(drs)
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 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.
Attachment #8678283 - Flags: review?(dale) → review+
Thanks, I'll do that !
Assignee: nobody → felash
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 !
Attachment #8680041 - Flags: review?(rchien)
Attachment #8680041 - Flags: feedback?(francisco)
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 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.
Attachment #8680041 - Flags: review?(rchien) → review?(gasolin)
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.
Attachment #8680041 - Flags: review?(gasolin) → review+
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 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.
Attachment #8680041 - Flags: feedback?(francisco) → feedback+
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 !
Attachment #8680041 - Flags: review+ → review?(rchien)
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.
Attachment #8680041 - Flags: review?(rchien) → review+
(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 ?
master: https://github.com/mozilla-b2g/gaia/commit/0036c757d418347afc9f252b2766b88e283848bf

Thanks for the tests and reviews !
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(nhirata.bugzilla)
Comment on attachment 8678283 [details] [review]
[gaia] julienw:1178242-notes-evernote > mozilla-b2g:master

Looks good, sync is back in business.
Attachment #8678283 - Flags: feedback?(doliver) → feedback+
Duplicate of this bug: 1178824
You need to log in before you can comment on or make changes to this bug.