993273 - Trusted UI doesn't show homescreen but show white background instead

Status: RESOLVED FIXED

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

Description

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

STR:

1. Go to marketplace app
2. Login, you will see trusted UI with Persona
3. Inspect the homescreen around the small trusted UI dialog

Expected:

1. see homescreen

Actual:

1. See white background

Please confirm the regression range and status of 1.4. I am seeing this on 1.5 yesterday.

Comment 1

[Matthew Vaughan]

This looks to be a gaia issue.

TINDERBOX GAIA/GECKO SWAP:
last working gaia/first broken gecko = NO REPRO
Gaia:  f09ec7d9d0bb7c40998ddb6b5bf397e578add541
Gecko: ba4c5a81d56a

first broken gaia/last working gecko = REPRO
Gaia:  70978988d048737b1379f5452a679429dadcd35f
Gecko: fb7fef480e48


B2G INBOUND:
- Last Working -
Device: Buri ENG Master MOZ RIL
BuildID: 20140314231912
Gaia: 56f8ff83f112b706270150b70b753a1034625a18
Gecko: ffa3c2440108
Version: 30.0a1
Firmware Version: v1.2-device.cfg

- First Broken -
Device: Buri ENG Master MOZ RIL
BuildID: 20140315043211
Gaia: 70978988d048737b1379f5452a679429dadcd35f
Gecko: 609a128858ad
Version: 30.0a1
Firmware Version: v1.2-device.cfg

Push log: https://github.com/mozilla-b2g/gaia/compare/56f8ff83f112b706270150b70b753a1034625a18...70978988d048737b1379f5452a679429dadcd35f

Comment 2

[Jason Smith [:jsmith]]

Caused by bug 897600.

Comment 3

[Jason Smith [:jsmith]]

Can we get a screenshot of the bug here?

Comment 4

[Tony Nguyen]

Attachment: screenshot1.4

Here is the screenshot of how it looks on 1.4.

Comment 5

[Tony Nguyen]

Attachment: screenshot1.5

On the Master M-C builds, the area surrounding the sign-in window is white instead of grey.

Comment 6

[Jason Smith [:jsmith]]

If we're going to evaluate a blocking decision here, then let's analyze this for the earliest affected release.

Paul - If there's no homescreen present in the background in a trusted UI session, how bad is this? Does this basically mean that the content of the window isn't really proven to the user that they can trust it?

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #6)
> If we're going to evaluate a blocking decision here, then let's analyze this
> for the earliest affected release.
> 
> Paul - If there's no homescreen present in the background in a trusted UI
> session, how bad is this? Does this basically mean that the content of the
> window isn't really proven to the user that they can trust it?

Yes that is accurate. The idea is that an app can't accurately spoof yourself homescreen.

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

As an aside, the reality is that this isn't a hugely strongly control, since the user is unlikely to know that they are supposed to see the homescreen in this situation, but it was the only solution proposed so far that has any effect on spoofing.

Comment 9

[Andrew Overholt [:overholt]]

Triage decided that this was probably bad enough to be detrimental to the point of the trusted UI so 1.4+.

Comment 10

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

Borja, could you verify if this bug is indeed cause by bug 897600 and fix it if so?

Comment 11

[Borja Salguero [:borjasalguero]]

Hi Tim!
Actually https://bugzilla.mozilla.org/show_bug.cgi?id=897600 is part of v1.4 (https://github.com/mozilla-b2g/gaia/commit/e5d343543ff5b47c7d06c9a1713956585be5651c), so it seems that this should not be related, due to the same code is in v1.4 & v1.5 (nothing changed from our side since a long time ago here).

Actually Firefox Accounts is using a separate container & iframe, so it seems not to be related with this issue (this screen is related with Persona flow, not with Firefox Accounts panel). Let me know if you need more info!

Comment 12

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

I am confused. It looks like comment 11 is contradict with regression range in comment 1.

Borja, Matt, do you have more information to contribute?

Comment 13

[Jason Smith [:jsmith]]

Adding window keyword to confirm the window is correct.

Comment 15

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #13)
> Adding window keyword to confirm the window is correct.

After retesting, I've found the window given in comment 1 to be correct. There is an obvious last working build and a first broken build. Also, the gaia/gecko swap yields that this is a gaia related issue.

One thing to note is in 1.4, the screen behind the trusted UI (Persona login screen) is gray where in Master it is white.

Comment 16

[Borja Salguero [:borjasalguero]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #12)
> I am confused. It looks like comment 11 is contradict with regression range
> in comment 1.
> 
> Borja, Matt, do you have more information to contribute?

Hi Tim. https://bugzilla.mozilla.org/show_bug.cgi?id=993273#c11 only defines that this is not related with Firefox Accounts (actually the screen is related with Persona Login, not Firefox Account), and I was adding extra info explaining that Firefox Accounts is part of v1.4 & v1.5 (now it's pref on by default, but it was there since v1.4).

So this means that probably there is a regression in System, so :alive can shed more light here.

Comment 17

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

You guys leave me no choice but to do bisect on my own... will post result.

Comment 18

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

Here is my bisect log, which again clearly shows bug 897600 indeed caused the regression.
I know the feature sounds unrelated, but it's possible some event listeners or evt.stopPrpgration() was incorrectly removed.

Borja, please fix the regressions caused by your bug.

==

git bisect start
# good: [56f8ff83f112b706270150b70b753a1034625a18] Merge pull request #16099 from jrburke/bug969609-email-notification-black-screen
git bisect good 56f8ff83f112b706270150b70b753a1034625a18
# bad: [19d2fa061e75c772b3ecd6b56d9fbf22c666f5ba] Merge pull request #18152 from alivedise/bugzilla/825435/dupe-alert
git bisect bad 19d2fa061e75c772b3ecd6b56d9fbf22c666f5ba
# bad: [632c6746a5124d852b2fd7c0b77f207e2d54ff8d] Merge pull request #17725 from wilsonpage/bug/972137
git bisect bad 632c6746a5124d852b2fd7c0b77f207e2d54ff8d
# bad: [423a99b9dcb75beb0a2a359826c878fe3ac004ac] Merge pull request #17501 from viorelaioia/bug_985903
git bisect bad 423a99b9dcb75beb0a2a359826c878fe3ac004ac
# bad: [71df836011d1d116c94551c23ffce522ded958ae] Merge pull request #17337 from vingtetun/bug981610.shared
git bisect bad 71df836011d1d116c94551c23ffce522ded958ae
# bad: [0a59f825fb9742560c1b2eccb077d45f03e27194] Bug 968091 -[Cost Control] Use lazy-load to load JS dependencies
git bisect bad 0a59f825fb9742560c1b2eccb077d45f03e27194
# bad: [9dce9b44b733ef3675af2af86e7bf8e49274de87] Merge pull request #17096 from fcampo/export-footer-stuck-977368
git bisect bad 9dce9b44b733ef3675af2af86e7bf8e49274de87
# bad: [a504a24be3b1e58713de020408189e3d7d124be8] Remove some CSS warnings on the settings app. r=ehung
git bisect bad a504a24be3b1e58713de020408189e3d7d124be8
# bad: [679dbd99f5160a2ea7cbe7ae1bbf1af9cfaf727a] Merge pull request #17224 from vingtetun/bug983739
git bisect bad 679dbd99f5160a2ea7cbe7ae1bbf1af9cfaf727a
# bad: [8f802237927c7d5e024fb7dca054dd5efef6b2e6] Merge pull request #16974 from KevinGrandon/bug_946852_search_mode
git bisect bad 8f802237927c7d5e024fb7dca054dd5efef6b2e6
# bad: [7e02be1fbe1325091a8cd3901b96628b38596259] Bug 982645 - [Gaia] [Build] unit test for preferences.js
git bisect bad 7e02be1fbe1325091a8cd3901b96628b38596259
# bad: [70978988d048737b1379f5452a679429dadcd35f] Merge pull request #16483 from borjasalguero/ftu_fxa
git bisect bad 70978988d048737b1379f5452a679429dadcd35f
# bad: [e5d343543ff5b47c7d06c9a1713956585be5651c] Bug 897600 Add firefox accounts screen to FTE flow r=arcturus,ferjm
git bisect bad e5d343543ff5b47c7d06c9a1713956585be5651c
# first bad commit: [e5d343543ff5b47c7d06c9a1713956585be5651c] Bug 897600 Add firefox accounts screen to FTE flow r=arcturus,ferjm

Comment 19

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

Methodology, for people who want to double confirm: I only flashed the System app package (with APP=system) to speed up bisecting. I am using an Unagi with Gecko 30.0a1 built on 20140408.

Comment 20

[Borja Salguero [:borjasalguero]]

Hi Tim,
This is *not related* with Firefox Accounts. Please, check the following:
https://github.com/mozilla-b2g/gaia/commit/439e40ef0e79393f94ca102b9fbd0785355e4b34#diff-ec9159f13f3a4ba569839c48aeb43cbcL4

As you can see, explicitly this style was removed during that patch:
Bug 961800 - Implement Child Window Factory (https://bugzilla.mozilla.org/show_bug.cgi?id=961800)

You can check the history of commits here:
https://github.com/mozilla-b2g/gaia/commits/master/apps/system/style/trusted_ui

It's the last commit. Actually if you restore that line you will see that the white background disappear and turns into grey (following comment https://bugzilla.mozilla.org/show_bug.cgi?id=993273#c5), which is the result found in v1.4 (check comment https://bugzilla.mozilla.org/show_bug.cgi?id=993273#c15).

Probably we should check with :alive, and check why in v1.4 is grey instead of transparent. I hope it helps!

Comment 21

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

I am not sure why git could lie, but I am going to take this bug and submit a fix anyway.

Comment 22

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

I can now confirm (with App Manager) the white background is an empty div: <div id="fxa_dialog"></div>. However I have not figure out when and why it's being added into the DOM.

Comment 23

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

I can now confirm the empty div is added because FxAccountsDialog constructor call render() when it is being constructed. I will provide a CSS fix for this dialog. I would be better if Borja could provide a JS fix. I cannot provide a JS fix because I cannot test the feature.

Comment 24

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#18360

Comment 25

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

Comment on attachment 8407429 [details] [review]
mozilla-b2g:master PR#18360

Alive, could you review this patch?

Borja, would you mind reading the previous comments and see if this fix is the best fix or not?

Comment 26

[Borja Salguero [:borjasalguero]]

Attachment: Patch to issue

This issue comes tied to the new WindowManager. Take into account that as every 'Dialog' is overwriting it own 'view', we need to force that the new element to be appended in:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/system_dialog.js#L79

must be *hidden* by default. Without it, we could have a lot of issues in the future.

This patch is forcing Fxa-Dialog to be 'hidden', but this should be automatically done by SystemDialog.

Comment 27

[Borja Salguero [:borjasalguero]]

Comment on attachment 8407429 [details] [review]
mozilla-b2g:master PR#18360

After taking a look, the issue is not a 'CSS' issue, it's related with the logic behind WindowManager & SystemDialog, which are working with 'hidden' property of HTML.

When a new Dialog is appended to SystemManager, it *must be hidden true by default*. Currently, SystemDialog is appending the element to the DOM:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/system_dialog.js#L79

Without any check if it's hidden or not. This is the bug.

The patch I've made is:
https://github.com/mozilla-b2g/gaia/pull/18418/files

It's a simple patch adding the hidden true by default, and everything works as expected.

@Alive,could you check this? Feel free to take my patch and modify as you want!

Comment 28

[Borja Salguero [:borjasalguero]]

Comment on attachment 8408157 [details] [review]
Patch to issue

Changing the flag to r?

Tim, could you take a look? After the discussion the issue seems to be easy to fix! Thanks :)

Comment 29

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

Comment on attachment 8408157 [details] [review]
Patch to issue

We could either add |hidden = true| in this place or fix system_dialog, and add it in system_dialog.js#L80 right after the |this.view()| call. Alive can make the call here.

Thank you for the fix.

Comment 30

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

Comment on attachment 8408157 [details] [review]
Patch to issue

Thanks!

Comment 31

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

Do we need test for this? Does other modules depend on SystemDialog expecting this behavior?

Comment 32

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

:/ Yeah, I think we should call .hide from fxa instead of do 'hide' in an invisible way in render().
Please set review again.

Comment 33

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

Comment on attachment 8408157 [details] [review]
Patch to issue

See last comment

Comment 34

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

How about this? It seems that SystemDialog expects the views are in hidden state when created, for all it's dependent. If the above statement is correct this is how we should fix this bug.

diff --git a/apps/system/js/fxa_dialog.js b/apps/system/js/fxa_dialog.js
index 8ac7041..42459e5 100644
--- a/apps/system/js/fxa_dialog.js
+++ b/apps/system/js/fxa_dialog.js
@@ -27,7 +27,7 @@
   FxAccountsDialog.prototype.DEBUG = false;
 
   FxAccountsDialog.prototype.view = function fxad_view() {
-    return '<div id="' + this.instanceID + '"></div>';
+    return '<div id="' + this.instanceID + '" hidden></div>';
   };
 
   FxAccountsDialog.prototype.getView = function fxad_view() {

Comment 35

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

Makes sense since SimPinSystemDialog.prototype.view has the same pattern.

Comment 36

[Borja Salguero [:borjasalguero]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #34)
> How about this? It seems that SystemDialog expects the views are in hidden
> state when created, for all it's dependent. If the above statement is
> correct this is how we should fix this bug.
> 
> diff --git a/apps/system/js/fxa_dialog.js b/apps/system/js/fxa_dialog.js
> index 8ac7041..42459e5 100644
> --- a/apps/system/js/fxa_dialog.js
> +++ b/apps/system/js/fxa_dialog.js
> @@ -27,7 +27,7 @@
>    FxAccountsDialog.prototype.DEBUG = false;
>  
>    FxAccountsDialog.prototype.view = function fxad_view() {
> -    return '<div id="' + this.instanceID + '"></div>';
> +    return '<div id="' + this.instanceID + '" hidden></div>';
>    };
>  
>    FxAccountsDialog.prototype.getView = function fxad_view() {

Hi! Please feel free to take this bug and create the patch, I was only pointing where the problem was, and there are several ways to fix it (for example the one explained by Tim, or setting through JS the property to the element). The final goal is to have all SystemDialogs 'hidden' by default, in order to not collide with the other previously created.
I would suggest to add a test in order to ensure that this is working as expected, and a comment within the code explaining why this is needed.
Thanks! :)

Comment 37

[howie [:howie]]

ni Tim for ticket assignment

Comment 38

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

Taking

Comment 39

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

Attachment: Github: https://github.com/mozilla-b2g/gaia/pull/18570

I revised the test strategy a bit so we could properly getting the resulting element. I agree we should have better API of the SystemDialog*, but let's not fix that in this bug.

*1 Asking user to overwrite certain method and do certain thing but allow silent failure (i.e. this bug) when it doesn't do something asked.
*2 Do things in constructor directly w/o giving external script the opportunity to overwrite some property per-instance.

Comment 40

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

Comment on attachment 8410911 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/18570

Thanks!

Comment 41

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

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v1.4 uplift.

Comment 43

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:v1.4 PR#18660

Comment 44

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

Comment on attachment 8412326 [details] [review]
mozilla-b2g:v1.4 PR#18660

These are no tests nor fxa_dialog.js on v1.4. So, one-line patch.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/d0917d51dc0fadb3385f34e80a0ba32e6a92f71b