1095617 - [Browser] Persona not working inside browser

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Fred Muyumba [:FredM]]

Attachment: logcat_20141107_1012.txt

Description:
The "Get started" button in "Firefox Affiliates" does NOT respond when the User taps on it
   
Repro Steps:
1) Update a Flame device to BuildID: 20141107001205
2) Launch "Settings" > Tap on "Device information"
3) Tap on "Your Privacy"
4) Tap on "Firefox OS"
5) Scroll down to "mozilla" and tap on "Firefox Affiliates" link
6) Tap on "Get stated" button
  
Actual:
The "Get stated" button is NOT responsive
  
Expected: 
The "Get stated" button IS responsive
  
Environmental Variables:
Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141107001205
Gaia: 6295f6acfe91c6ae659712747dd2b9c8f51d0339
Gecko: 8c23b4f2ba29
Version: 34.0
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
  
  
Repro frequency: 100%
See attached: video clip & logcat:http://youtu.be/IA4sZ2Vxi14

Comment 1

[Fred Muyumba [:FredM]]

This issue is reproducible on Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)

Result: 
The "Get started" button in "Firefox Affiliates" does NOT respond when the User taps on it

Environmental Variables:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141107040206
Gaia: 779f05fead3d009f6e7fe713ad0fea16b6f2fb31
Gecko: 64f4392d0bdc
Version: 36.0a1 
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

-----------------------------------------------------------------------------------------------

This issue is NOT reproducible on Flame 2.0 (319mb)(Kitkat Base)(Shallow Flash

Result:
The "Get started" button in "Firefox Affiliates" IS responsive when the User taps on it

Environmental Variables:
Device: Flame 2.0 (319mb)(Kitkat Base)(Shallow Flash
BuildID: 20141107000206
Gaia: d3e4da377ee448f9c25f908159480e867dfb13f3
Gecko: 9836e9d81357 
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Joshua Mitchell (Inactive)]

[Blocking Requested - why for this release]: Broken button functionality / bug encountered during FTU

Comment 3

[Chris Kreinbring [:CKreinbring]]

Regression window
Last working
BuildID: 20140825172052
Gaia: 4d1d0ea5a82cddeeab497774cfa1703639e3c7d9
Gecko: dc352a7bf234
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First broken
BuildID: 20140826105131
Gaia: ea93363a8c424d65a9ad91438ce6961377a20f98
Gecko: e30f862fe19e
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Working Gaia / Broken Gecko = No repro
Gaia: 4d1d0ea5a82cddeeab497774cfa1703639e3c7d9
Gecko: e30f862fe19e
Broken Gaia / Working Gecko = Repro
Gaia: ea93363a8c424d65a9ad91438ce6961377a20f98
Gecko: dc352a7bf234
Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/4d1d0ea5a82cddeeab497774cfa1703639e3c7d9...ea93363a8c424d65a9ad91438ce6961377a20f98


B2G Inbound
Last working
BuildID: 20140826074425
Gaia: 38a34b7b4958785bd36f3658bc537048c2735343
Gecko: bd1bc3d755b2
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

First broken
BuildID: 20140826080629
Gaia: 31043c27f7a5156ff4abdcb0c99207f34c5e57d0
Gecko: df1112296179
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Working Gaia / Broken Gecko = No repro
Gaia: 38a34b7b4958785bd36f3658bc537048c2735343
Gecko: df1112296179
Broken Gaia / Working Gecko = Repro
Gaia: 31043c27f7a5156ff4abdcb0c99207f34c5e57d0
Gecko: bd1bc3d755b2
Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/38a34b7b4958785bd36f3658bc537048c2735343...31043c27f7a5156ff4abdcb0c99207f34c5e57d0

Comment 4

[Joshua Mitchell (Inactive)]

Possibly broken by Bug 1055761 - can you take a look Kevin?

Comment 5

[Kevin Grandon :kgrandon]

I tried reverting the suggested patch, but unfortunately the bug still reproduced. Perhaps it's some combination of gecko & gaia changes?

Also the URL for ease of testing is: https://affiliates.mozilla.org/


I noticed the following in logcat which could also be related:

W/Browser ( 1882): [JavaScript Warning: "Use of getPreventDefault() is deprecated.  Use defaultPrevented instead." {file: "https://cdn.optimizely.com/js/246059135.js" line: 92}]
W/Browser ( 1882): [JavaScript Warning: "Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://246059135.log.optimizely.com/event?a=246059135&d=25134714&y=false&src=js&s245875585=direct&s245617832=none&s246048108=false&s245677587=ff&s869421433=true&n=engagement&u=oeu1415596338367r0.880437748443253&wxhr=true&t=1415596357110&f=2000380330&g=245569630. This can be fixed by moving the resource to the same domain or enabling CORS." {file: "https://246059135.log.optimizely.com/event?a=246059135&d=25134714&y=false&src=js&s245875585=direct&s245617832=none&s246048108=false&s245677587=ff&s869421433=true&n=engagement&u=oeu1415596338367r0.880437748443253&wxhr=true&t=1415596357110&f=2000380330&g=245569630" line: 0}]
W/Browser ( 1882): [JavaScript Error: "Error: loggedInUser is not valid" {file: "jar:file:///system/b2g/omni.ja!/components/nsDOMIdentity.js" line: 116}]
W/Browser ( 1882): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "https://affiliates.mozilla.org/static/js/base-min.js?build=697dba0" line: 2}]

Comment 6

[Kevin Grandon :kgrandon]

I'll continue to look when I have time. This is possibly persona related? Has anyone filed any other persona bugs recently?

Comment 7

[Kevin Grandon :kgrandon]

I tried backing out all of the patches in the suggested regression range, (back to 8d051e3b074fd37674d0152f27761c39b54f4114), and the issue still reproduced for me. I'm thinking this might be a gecko problem?

Comment 8

[Kevin Grandon :kgrandon]

I have a feeling this might be some persona issue. When using this on a test site, myfavoritebeer.org, it also works in the desktop browser, but not in mobile. It could very well be that we are doing something wrong in gaia, but I'm looking for some help tracking this down.

The following is shown in logcat when testing the myfavoritebeer.org site:

W/Browser ( 1180): [JavaScript Warning: "Use of getPreventDefault() is deprecated.  Use defaultPrevented instead." {file: "http://myfavoritebeer.org/js/jquery.js" line: 17}]
W/Browser ( 1180): [JavaScript Error: "TypeError: navigator.id.get is not a function" {file: "http://myfavoritebeer.org/js/main.js" line: 95}]

Adding a few people from persona/identity to look into this. Do the logs in comment 5 and above make sense? Thanks guys!

Comment 9

[Marcia Knous [:marcia]]

Testing on Flame today I see the button doesn't work on 2.1, but on the latest 2.2 build I do get the Persona screen when I tap on the "Get Started" button.

I agree with Kevin that it seems like a Persona issue. Let's let that team take a look at the errors in Comment 8 and weigh in.

Comment 10

[Marcia Knous [:marcia]]

(In reply to Marcia Knous [:marcia - use needinfo] from comment #9)
> Testing on Flame today I see the button doesn't work on 2.1, but on the
> latest 2.2 build I do get the Persona screen when I tap on the "Get Started"
> button.
> 
> I agree with Kevin that it seems like a Persona issue. Let's let that team
> take a look at the errors in Comment 8 and weigh in.

Correction - I do see the issue on the latest nightly as well. It does work fine on the latest 2.0 build.

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

These lines do look pretty weird to me:

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #5)
> W/Browser ( 1882): [JavaScript Error: "Error: loggedInUser is not valid"
> {file: "jar:file:///system/b2g/omni.ja!/components/nsDOMIdentity.js" line:
> 116}]
> W/Browser ( 1882): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file:
> "https://affiliates.mozilla.org/static/js/base-min.js?build=697dba0" line:
> 2}]

Sam, do these look at all like the gecko bugs you've seen recently?

Comment 12

[Sam Penrose]

(In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #11)
> These lines do look pretty weird to me:
> 
> (In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from
> comment #5)
> > W/Browser ( 1882): [JavaScript Error: "Error: loggedInUser is not valid"
> > {file: "jar:file:///system/b2g/omni.ja!/components/nsDOMIdentity.js" line:
> > 116}]
> > W/Browser ( 1882): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file:
> > "https://affiliates.mozilla.org/static/js/base-min.js?build=697dba0" line:
> > 2}]
> 
> Sam, do these look at all like the gecko bugs you've seen recently?

The last time we saw this one [1], content was setting:

  {loggedInUser: null}

and Gecko was receiving:

  {loggedInUser: 'null'}

Assuming that's actually the issue and I can reproduce it, I should be able to throw up a disgusting one-line workaround. However, the "not a function" issue in comment 8 suggests a separate WebIDL vs TrustedUI conflict, or maybe just TrustedUI breakage a la bug 1094550 . While I'm looking at the loggedInUser, Kevin would you have cycles to chase an appropriate System peer on "not a function"?

[1] https://github.com/mozilla/gecko-dev/blob/master/dom/identity/nsDOMIdentity.js#L114

Comment 13

[Sam Penrose]

Persona appears to be busted at the Gaia level. If I bypass the "Error: loggedInUser is not valid" by any of:

  - setting it to null;
  - deleting the property
  - leaving it as "null" but bypassing the error test

everything goes great in Gecko, which lobs something like this to the SystemAppProxy at [1]. But the TrustedUI doesn't come up.

[1] https://github.com/mozilla/gecko-dev/blob/master/b2g/components/SignInToWebsite.jsm#L313

Comment 14

[Sam Penrose]

Comment aborted; here's what SignInToWebsite.jsm gives to the System AppProxy when I use the "Sign In" button on air.mozilla.org:

  {"type":"id-dialog-open",
   "showUI":true,
   "id":"id-dialog-open-{7367fa62-5542-9c4e-a155-ea7769241f16}",
   "requestId":"{b0d504f4-e29e-9446-83eb-3f02af66139d}",
   "uri":"https://firefoxos.persona.org"}

It should be received at [1]; I suspect it is and the failure is after the call to TrustedUIManager.open() at line 63.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/identity.js#L27

Comment 15

[Stephany Wilkes]

Triage is blocking based on comment #13.

Comment 16

[Sam Penrose]

Attachment: 1095617-fix-loggedInUser.patch

Fernando, this patch cleans up the workaround for bad stringification of null and undefined in the IPC/IDL mechanism that sends values from content to nsDOMIdentity.js. It does not address the TrustedUI failure. Thanks for taking a look.

Comment 17

[Sam Penrose]

I would not be surprised if we see failures due to TrustedUI here:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=644563bc05a9

Comment 18

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I can't get the Trusted UI to appear at all on master.

STR:
1. open browser
2. surf to login.persona.org
3. click 'sign in'

Expected: Trusted UI appears with Persona window
Actual: nothing happens

Working on getting QA help to repro this now. Not clear yet if the problem is with Persona or if the Trusted UI is broken.

nalexander reports in IRC that e10s broke Persona, and the suggestion was to remove it from the tree, rather than fix the bugs. I'll look for a bug number there.

Comment 19

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hmm, I'm not sure it could be e10s, because that has been enabled for FXOS since 1.0, according to fabrice.

Added the bug which prompted this discussion on the dev-identity list: https://groups.google.com/forum/#!topic/mozilla.dev.identity/YH5mzZSvj1c

Comment 20

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Could someone in QA generate a regression window, so we can narrow down the commits that may have caused this bug?

Comment 21

[Joshua Mitchell (Inactive)]

(In reply to Jared Hirsch [:_6a68] [NEEDINFO pls] from comment #20)
> Could someone in QA generate a regression window, so we can narrow down the
> commits that may have caused this bug?

Regression window was already completed at comment 3 but based on Kevin's comments it might not be accurate; we'll take another look

Comment 22

[Joshua Mitchell (Inactive)]

I've verified that the regression window from comment 3 is 'correct'. It is very definitive with a clear broken state and working state, the swaps are correct and it indicates a Gaia issue.

It is possible, as Kevin previously speculated, that it is due to some combination of changes and the window we have arrived at is only the final piece to the puzzle.

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Aw man, I missed that earlier! Sorry Joshua, thanks for re-verifying it anyway.

:edwong reports that he can reproduce the bug, but Persona works fine if the UI Test app is used. So, the Persona code on device does work.

Alive, kicking this bug over to you - seems like bug 1055761 may have broken the Persona Trusted UI for content inside a browser window. Mind taking a look?

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Mind to take a look?

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8522361 [details] [diff] [review]
1095617-fix-loggedInUser.patch

Review of attachment 8522361 [details] [diff] [review]:
-----------------------------------------------------------------

The patch LGTM, but it doesn't seem to be the reason of this failure, right?

::: dom/identity/nsDOMIdentity.js
@@ +107,5 @@
>      message["loggedInUser"] = aOptions["loggedInUser"];
> +    if (message.loggedInUser == "null") {
> +      message.loggedInUser = null;
> +    } else if (message.loggedInUser == "undefined") {
> +      delete message.loggedInUser;

You can do message.loggedInUser = null; in both cases. Deleting a object property is more expensive than assigning null.

Comment 26

[Sam Penrose]

Attachment: 1095617-fix-loggedInUser.patch

Fernando -- thanks for looking at this patch; I replaced the attribution deletion block per your suggestion. It fixes the error discussed in comments 5, 11, and 12, which breaks Persona as soon as content calls watch(). As I state in comment 14, even after we fix this Persona will not work until Gaia receives the message from SignInToWebsite.jsm and handles it appropriately, but we do need to fix this for that message to even be sent.

Comment 27

[Sam Penrose]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21336d56e879

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8523937 [details] [diff] [review]
1095617-fix-loggedInUser.patch

Fair enough

Comment 29

[Sam Penrose]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21336d56e879

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4c94eba2f88e

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c94eba2f88e

Comment 32

[Sam Penrose]

(In reply to Carsten Book [:Tomcat] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/4c94eba2f88e

Thanks for integrating this patch. The underlying problem has not been solved, so I am reopening the bug.

Comment 33

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

George, what's the state of this bug? I am a bit confused as you are the assignee but there was(is?) a Gecko bug landed under this bug #...

Comment 34

[Sam Penrose]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #33)
> George, what's the state of this bug? I am a bit confused as you are the
> assignee but there was(is?) a Gecko bug landed under this bug #...

The Gecko patch was necessary but not sufficient (see comment 14 ). We still need to determine why Gaia is not opening the TrustedUI.

Comment 35

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to gaia master

Hi Alive,
could you review my patch? I also add new unit test for what I've modified.

Comment 36

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

Comment on attachment 8525800 [details] [review]
PR to gaia master

Stealing review so that this can land. Looks good except that TrustedUIManager still need to be changed to constructor pattern in the future :)

Comment 37

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks Tim,
master: https://github.com/mozilla-b2g/gaia/commit/aad40f6d6eb8f626c6a20db55b9f00d2e832f113


I think master should fine with my gaia and gecko patch. However, I think 2.1 branch also have pb as comment 8. Perhaps we also need same fix as comment 23 for 2.1?

Comment 38

[Sam Penrose]

(In reply to George Duan [:gduan] [:喬智] from comment #37)
> Thanks Tim,
> master:
> https://github.com/mozilla-b2g/gaia/commit/
> aad40f6d6eb8f626c6a20db55b9f00d2e832f113
> 
> 
> I think master should fine with my gaia and gecko patch. However, I think
> 2.1 branch also have pb as comment 8. Perhaps we also need same fix as
> comment 23 for 2.1?

Since the initial bug was reported on 2.1, that sounds right to me. We should ask for verification when the fix lands.

Comment 39

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: Gaia PR to 2.1

Gaia PR to 2.1,
however, we still need proper fix for 2.1 gecko for comment 37.

Hi Sam,
I'm not sure if your commit can be uplifted to 2.1 directly. Could you ask for approval‑gaia‑v2.1 by yourself :)?

Comment 40

[Sam Penrose]

Comment on attachment 8523937 [details] [diff] [review]
1095617-fix-loggedInUser.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown IPC issue
User impact if declined: Persona won't work on FxOS for some established reliers
Testing completed: Automated; should get QA verification.
Risk to taking this patch (and alternatives if risky): Small chance creates new Persona or FxA issue.
String or UUID changes made by this patch: None

Comment 41

[bhavana bajaj [:bajaj]]

NI QA to help verify this on master (2.2) before we uplift.

Comment 42

[Dan Callahan [:callahad]]

Just a heads up (I spoke with :spenrose regarding this on IRC), but this change in the patch:

    +    if (message.loggedInUser == "null" || message.loggedInUser == "undefined") {
    +      message.loggedInUser = null;
    +    }

Coalesces "null" and "undefined" into `null`. Unfortunately, Persona treats `null` and `undefined` differently. This change should work for the vast majority of sites, but may cause issues with some fringe reliers.

Sam and I both agreed that "works for almost everyone" is better than "won't work for anyone," in this case.

Comment 43

[Brian Huang [:brianhuang][:brhuang]]

Hi Tony, Eric,

Could you have someone to help on verification? Thank you.

Brian

Comment 44

[Eric Chang [:ericcc] [:echang]]

Works on master 
http://youtu.be/KnB_xOl74Ds 

Build ID              20141124160209
Gaia Revision         e5d666d6f62480ced56c6d9352f5e12befb5a862
Gaia Date             2014-11-24 19:32:22
Gecko Revision        https://hg.mozilla.org/mozilla-central/rev/5c4d07e2199e
Gecko Version         36.0a1
Device Name           flame
Firmware(Release)     4.4.2
Firmware(Incremental) 39
Firmware Date         Thu Oct 16 18:19:14 CST 2014
Bootloader            L1TC00011880

Comment 45

[Chris Kreinbring [:CKreinbring]]

Verified on Flame 2.2 nightly with shallow flash.
Actual result: Tapping the Get Started button brings up the Persona login page.

BuildID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Platform Version: 36.0a1
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

--------------------------------------------------------------------------------------------------------

Not yet verified on Flame 2.1 nightly on shallow or full flash.
Actual result: Tapping the Get Started button does not generate a response.

BuildID: 20141125001201
Gaia: 1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko: db893274d9a6
Platform Version: 34.0
Firmware Version: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4646c3113273

v2.1: https://github.com/mozilla-b2g/gaia/commit/341358c80ed66a3ab18fcbb3e56552d0d2a9ad06

Comment 47

[Lance（leave from Mozilla）]

This issue has been verified successfully on Flame 2.1.
See attachment: 1121.MP4
Reproducing rate: 0/5

Repro Steps:
1) Update a Flame device to BuildID: 20141107001205
2) Launch "Settings" > Tap on "Device information"
3) Tap on "Your Privacy"
4) Tap on "Firefox OS"
5) Scroll down to "mozilla" and tap on "Firefox Affiliates" link
6) Tap on "Get stated" button
  
Actual Result:
Tapping the Get Started button brings up the Persona login page.

Flame 2.1 version:
Gaia-Rev        5372b675e018b6aac97d95ff5db8d4bd16addb9b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/f34377ae402b
Build-ID        20141127001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141127.035005
FW-Date         Thu Nov 27 03:50:16 EST 2014
Bootloader      L1TC00011880

Comment 48

[Lance（leave from Mozilla）]

Attachment: 1121.MP4