762939 - [BrowserAPI] Correctly propagate setVisibile changes across <iframe mozbrowser>

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: camera.js I use for testing: see lines 172..194 for visibility code

Observed on b2g, pulled from github.com/mozilla-b2g/mozilla-central.git:1a7df179243a612b6836c0b6169338d70e677cd5.

Steps to reproduce:
1. patch gecko with patch from bug 740997
2. drop camera.js from bug 740997 in $B2G/gaia/apps/camera/js
3. ./build.sh gecko && ./flash.sh gecko && ./flash.sh gaia
4. adb logcat
5. unlock DUT and open the camera app
6. allow the screen to timeout
7. press the power button to wake up the DUT and get the lock screen

Expected behaviour:
On step 5, the camera hardware should be opened and the preview should be visible and active.  One step 6, the camera hardware should be closed and should remain closed.

Observed behaviour:
On step 7, the camera hardware is re-opened, even though the camera app is not visible.

adb logcat output (pruned to show GeckoConsole and power only):
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:196 in anonymous: Hello, hidden = 'mozHidden', visibilityChange = 'mozvisibilitychange'
 ...camera is opened and preview is started...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
 ...picture is taken...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:114 in takePictureSuccess: got image: 1294123 bytes, image/jpeg
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:123 in anonymous: image saved as 'img_20120608-105818.jpg'
 ...preview is resumed...
 ...display times out and turns off, camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder
I/power   (   78): *** set_screen_state 0
 ...pressed power button...
I/power   (   78): *** set_screen_state 1
 ...lock-screen is now visible on the screen, but the camera believes it is too and restarts the preview...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:191 in anonymous: camera app is now visible
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
E/GeckoConsole(   78): No chrome package registered for chrome:///content/images/throbber.png
 ...display times out...
I/power   (   78): *** set_screen_state 0
 ...camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder
 ...pressed power button...
I/power   (   78): *** set_screen_state 1
 ...again, lock-screen is now visible on the screen, but the camera believes it is too...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:191 in anonymous: camera app is now visible
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:77 in camera_setSource: Camera 0 focus modes: auto,infinity,macro
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:79 in camera_setSource: auto focus mode supported
 ...display times out...
I/power   (   78): *** set_screen_state 0
 ...camera is successfully closed/destroyed...
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:188 in anonymous: camera app is now hidden
E/GeckoConsole(   78): Content JS LOG at http://camera.gaiamobile.org/js/camera.js:93 in pause: pausing viewfinder

Comment 1

[Boris Zbarsky [:bzbarsky]]

The value document.hidden is completely controlled by the embedding (that is, the browser UI).  The DOM just reports the last thing it was told.

Sounds like some part of the b2g UI is not flagging things correctly....

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Do you mean that the UI literally has to set the value of |document.hidden|, or that it has to use docShell.isActive to indirectly set it?

We (should be) already doing the latter.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The latter.  The docshell then notifies all the documents inside itself.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Guys, we have to ensure the gaia window manager does setVisible(false) when we turn off the screen.

I think screen-off may be controlled by the b2g chrome code, which would make fixing this really hard.  We need gaia to control that.  And for that, I guess we need the idle timeout API.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Guys, we have to ensure the gaia window manager does setVisible(false) when
> we turn off the screen.
> 
> I think screen-off may be controlled by the b2g chrome code, which would
> make fixing this really hard.  We need gaia to control that.  And for that,
> I guess we need the idle timeout API.

That's exactly what happen.

Also it seems like there is an other bug when the top docshell is turned on (e.g the main system app), then all the sud-docshells (e.g apps contained as iframes) are turned on even if they have been turned off by setVisible(false) before.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The behavior of isActive on docshells is that setting it recursively sets in on all descendant docshells.  This is needed to make sure it propagates to subframes, for example, and makes total sense in the original use case (where the idea was that it would be set on a per-tab basis).

We could add some sort of "lock the visibility" API, or alternately the app needs to keep track of active state itself....

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The gaia window manager keeps track of active state for apps.  If we could selectively disable recursive (de)activation that would do the trick.  Perhaps disabling it automatically at mozapp boundaries makes sense?

We still want the recursive behavior for tabs (iframes) within the browser app, e.g.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Actually, the browser app itself wants to manually manage active state for its tabs.  So probably disabling at mozbrowser boundaries is the right way to go (gecko treats mozapp as a superset of mozbrowser).

Comment 9

[Justin Lebar (not reading bugmail)]

> Perhaps disabling it automatically at mozapp boundaries makes sense?

I think what logically needs to happen is that we have a hierarchy of visibility states

 "system level" -- screen is on or off
   "app level" -- app is foreground or background
     "browser tab level" -- browser tab is foreground or background

(Note that apps can nest.)

We save a visible state at each of these levels, but you can't be |on| unless everyone above you is |on|.  So you are Actually Visible (tm) if your Saved Visible State is |on| and your parent is Actually Visible.

I don't really care how we implement this...

Comment 10

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

My own tests shows that System app does receive mozvisibilitychange event from platform, and app frames is being set to false when switch away (by Gaia windows manager), and set to false when screen is off (by platform)

The problem seem to be that when the screen is turn back on, the app frames will be set to visibility *regardless they are on foreground (for Gaia) or not*.

I think the proper solution here would be

1) keep the logic Justin described (you can be visible only if everyone above you is on), and
2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to off, don't put it back to on when the screen is being turn back.

For condition (2), we would need a separate on/off state Boolean for the each frame, and do some AND/OR for between two.

Comment 11

[Justin Lebar (not reading bugmail)]

> For condition (2), we would need a separate on/off state Boolean for the each frame, and do some 
> AND/OR for between two.

Yes, this is what I was trying to propose in comment 9.

> 2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to 
> off, don't put it back to on when the screen is being turn back.

This isn't quite right.  Suppose Gaia sets the child frame to not-visible, then later sets it to visible.  We turn the screen off, then on.  That child frame is now visible, right?

Comment 12

[Justin Lebar (not reading bugmail)]

Marking depends-on browser-api mostly because I don't want to forget about this.  But also because the browser api lets us explicitly set the visibility state of frames.

Comment 13

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

(In reply to Justin Lebar [:jlebar] from comment #11)
> > For condition (2), we would need a separate on/off state Boolean for the each frame, and do some 
> > AND/OR for between two.
> 
> Yes, this is what I was trying to propose in comment 9.
> 
> > 2) if the parent frame (Gaia window manager) has ever set the visibility of the child frame to 
> > off, don't put it back to on when the screen is being turn back.
> 
> This isn't quite right.  Suppose Gaia sets the child frame to not-visible,
> then later sets it to visible.  We turn the screen off, then on.  That child
> frame is now visible, right?

Yes. (I don't see any difference between your comment and mine)

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Patch + tests, v1

bz, can you please review the docshell change?  

Dale, could you look at the other changes?  I think a peer should still review, but would you mind doing a first pass?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640466 [details] [diff] [review]
Patch + tests, v1

The docshell parts look fine as long as the browser app marks all its browserframes not active when it gets backgrounded and so forth.

I hate XPCOM signatures.  :(

Comment 16

[Justin Lebar (not reading bugmail)]

> The docshell parts look fine as long as the browser app marks all its browserframes not active when 
> it gets backgrounded and so forth.

If you meant actually meant "browser app" and not browserelement{parent,child}: The browser app doesn't have to do anything, under this patch.  BrowserElementParent listens to mozvisibilitychange on the window containing its iframe, and it propagates those visibility changes down to the BrowserElementChild.

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 640466 [details] [diff] [review]
Patch + tests, v1

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

::: docshell/base/nsDocShell.cpp
@@ +5052,4 @@
>    PRInt32 n = mChildList.Count();
>    for (PRInt32 i = 0; i < n; ++i) {
>        nsCOMPtr<nsIDocShell> docshell = do_QueryInterface(ChildAt(i));
> +      if (docshell) {

nit: for code readability, I think doing:
if (!docshell) {
  continue;
}
is better, but do whatever you want ;)

::: dom/browser-element/mochitest/browserElement_SetVisibleFrames2.js
@@ +34,5 @@
> +        SimpleTest.executeSoon(function() {
> +          SimpleTest.executeSoon(function() {
> +            SimpleTest.executeSoon(function() {
> +              SimpleTest.executeSoon(function() {
> +                SimpleTest.finish();

This is quite sad to see that but given that this will create random-green, I guess we can live with it.

::: dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Outer.html
@@ +36,5 @@
> +
> +document.body.appendChild(iframe1);
> +document.body.appendChild(iframe2);
> +iframe1.src = 'file_browserElement_SetVisibleFrames_Inner.html?child1';
> +iframe2.src = 'file_browserElement_SetVisibleFrames_Inner.html?child2';

nit: setting the src before adding a frame to the document is a good practice.

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b68a159759f