1028039 - document.hidden is false when launching an app from a system message

Status: RESOLVED FIXED

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

Description

[Anthony Ricaud (:rik)]

STR:
1) Apply https://github.com/Rik/gaia/tree/system-message-hidden-false
2) Make sure the Dialer app is killed
3) Press volume down to send a MMI

Expected result:
document.hidden is true and we set a high priority lock

Actual result:
document.hidden is false and we don't set the lock


Logcat output:
XXX ussd-received hidden: false
lock should have been set

Comment 1

[Gabriele Svelto [:gsvelto]]

I did some investigation on this and found out that when the ussd-received message is broadcast the system messenger is not asking for the apps to be put into the foreground, see here:

http://hg.mozilla.org/mozilla-central/file/335b6610fe0c/dom/messages/SystemMessageInternal.js#l597

|showApp| is set to false yet somehow the app is made visible shortly afterwards.

Comment 2

[Gabriele Svelto [:gsvelto]]

Following the call chain led me to this line in AppWindow:

https://github.com/mozilla-b2g/gaia/blob/3a3f6dcf99970348dd486d072f24a7d1df0df698/apps/system/js/app_window.js#L541

This is causing the app to become visible irrespective of the fact that we later set its visibility to false here:

https://github.com/mozilla-b2g/gaia/blob/3a3f6dcf99970348dd486d072f24a7d1df0df698/apps/system/js/app_window.js#L554

I'm not 100% of what's the expected behavior of this code but the fact that explicitly hiding the new window doesn't work tells me that there might be something amiss either here or in Gecko.

Comment 3

[Gabriele Svelto [:gsvelto]]

It doesn't seem like the system app is at fault here. I've been tracking the contents of the |mVisibilityState| in the underlying nsDocument class and during startup it starts as hidden, then becomes visible and then hidden again. However after this point |document.hidden| still returns false which is quite puzzling. Further investigation is needed.

Comment 4

[Gabriele Svelto [:gsvelto]]

I've dug further into this issue and I really don't understand why this is happening. Alive can you help me figure out what's wrong here? I've tried to trace all internal visibility changes and what I've come up with is the following:

- When the new frame is created and added by the window manager the document's hidden property is properly set to true

- Later on nsDocument::SetScriptGlobalObject() is called upon the associated document and it changes its visibility state, by the time the app is loaded the document is considered visible.

- I've tried calling nsDocument::GetVisibilityState() directly to figure out why this happens and I can see that nsDocument::IsVisible() is true which is puzzling as from my understanding the window manager shouldn't have marked the frame as visible yet.

Do you have any ideas why this is happening? It's really a weird scenario; I've even tried not calling app.launch() so I get stuck with an application that is not visible and yet |document.hidden| still evaluates to false.

Comment 5

[Etienne Segonzac (:etienne)]

Tried reproducing the issue with the clock app (and the mozalarm system message) since I'm not setup for telephony testing right now.

STR:
- set a timer for 1 minute
- kill the alarm app

When the app is launched by the system message, I log document.hidden in the message handler and it's true as expected.

Can you confirm this clock STR is also working for you?
If yes, we might want to have a look and see if any of the code is entry point dependent.

Comment 6

[Etienne Segonzac (:etienne)]

Okay, it may be because the clock app is slower to set the message handler (which is perfectly fine since the messages are queued on the gecko side).

The dialer gets the ussd-received with hidden == false, then 475ms later the visibility change and document.hidden is true.

Comment 7

[Etienne Segonzac (:etienne)]

Yes, it's because we screenshot the frame first.
The rationale to keep screenshoting first is pretty weak now that we have the overlays, I'll wip up a patch.

Comment 8

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

We use to screenshot apps launched in background in order to have something to display when the user edge swipes to one of the apps.

But this causes the app to be visible for a short moment at launch (and screws the document.hidden value for developers).

Now that we have the identification overlays, we can live without the screenshot in this case and swiping to one of those apps will just display the overlay.

Comment 9

[Gabriele Svelto [:gsvelto]]

Thanks a lot for your help here, this had been driving me nuts for two days in a row.

Comment 10

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

Comment on attachment 8446662 [details] [review]
Gaia PR

I didn't know screenshot will cause visibility to become true for app! The code looks alright with tests properly set, however I am not surely sure if the solution is right, since the identification overlay is semi-transparent -- if there isn't a screenshot underneath, that makes the background entirely white right? Is that an acceptable visual?

Comment 11

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

I set this to 1.4? because this is set to block a 1.3T bug, however the window management is entirely different in 1.3T so 1.3T might not affected by this bug. Etienne, could you confirm?

Comment 12

[Etienne Segonzac (:etienne)]

Looks like it's also happening on 1.3t [1] :/

I'll make a specific patch (we don't have the overlays there but the edges gestures are a dev-only option on 1.3t so it's not a problem).

[1] https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/system/js/window_manager.js#L1052

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> Comment on attachment 8446662 [details] [review]
> Gaia PR
> 
> I didn't know screenshot will cause visibility to become true for app! The
> code looks alright with tests properly set, however I am not surely sure if
> the solution is right, since the identification overlay is semi-transparent
> -- if there isn't a screenshot underneath, that makes the background
> entirely white right? Is that an acceptable visual?

You see the overlay and the system background behind. It's the same visual than when we don't have a screenshot in the orientation the phone is currently in. I'll confirm with Rob but it should be acceptable.

Comment 14

[Gabriele Svelto [:gsvelto]]

Anthony mentioned in bug 1016885 comment 33 that he wasn't running into this issue in v1.3t. Could it be possible that we've disabled screenshotting there on the Tarako? Or could it be just a timing issue (i.e. startup is slow enough on the Tarako that we don't see the value change to false).

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Gabriele Svelto [:gsvelto] from comment #14)
> Anthony mentioned in bug 1016885 comment 33 that he wasn't running into this
> issue in v1.3t. Could it be possible that we've disabled screenshotting
> there on the Tarako? Or could it be just a timing issue (i.e. startup is
> slow enough on the Tarako that we don't see the value change to false).

I'll probably put the patch here just in case (it's really simple) and ask for approval only if we see it's needed.

Comment 16

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/96f0c2de06037b3399307ba998e1c864c29ce50b

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #15)
> (In reply to Gabriele Svelto [:gsvelto] from comment #14)
> > Anthony mentioned in bug 1016885 comment 33 that he wasn't running into this
> > issue in v1.3t. Could it be possible that we've disabled screenshotting
> > there on the Tarako? Or could it be just a timing issue (i.e. startup is
> > slow enough on the Tarako that we don't see the value change to false).

Well while testing the 1.3t version of the patch I've confirmed that it's not needed :) (you're right screenshots are completely disabled)

Comment 18

[Preeti Raghunath(:Preeti)]

Not needed in 1.3T per comment 17 so unless this is needed for Dolphin please re-nom. Backlog till then.

Comment 19

[Gabriele Svelto [:gsvelto]]

(In reply to Preeti Raghunath(:Preeti) from comment #18)
> Not needed in 1.3T per comment 17 so unless this is needed for Dolphin
> please re-nom. Backlog till then.

We might want to ping the vendor because bug 1016885 is blocking as per their request. It's 1.3T+ currently but considering that Dolphin might have the same requirements as Tarako it might be required for 1.4 too.