1211258 - window.open doesn't mark the original now-background window to be in background

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: deviceorientation test

When window.open is used, the original window object doesn't get marked to be in background.
<a href="..." target="_blank"> doesn't seem to have that issue.
Based on the test in bug 1197901, on Android the original window is marked to be in background. (and on desktop we open a new window, which causes the original window to be
marked "non-active"). So this is FFOS specific issue.

This effectively breaks the fix for bug 681562.

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Olli Pettay [:smaug] from comment #0)
> This effectively breaks the fix for bug 681562.
or perhaps more accurate is to say that fixing bug 1197901 isn't possible.

Comment 2

[Gregor Wagner [:gwagner]]

[Blocking Requested - why for this release]:

Comment 3

[Andrew McCreight [:mccr8]]

Olli, does this mean that the original window will continue to get inputs while it is in the background? That sounds bad.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Not input, but it gets sensor events, while per bug 681562, or its variant bug 1197901, it shouldn't.
Also, I think this means the original window keeps processing JS timers as if the window was in foreground, so this could lead to higher power usage than it should (but I haven't yet tested setTimeout handling).

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

kanru, do you think you could find someone to look at this? I'm not familiar with ffos/gaia window management.
I think we need to fix this before the patch for bug 1197901 can land.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

The gaia system window manager lacks a owner. Tim, do you know who could look at this? Etienne, from the log it seems you are the most possible one.

Comment 7

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

Etienne and :albertopq are the owners.

This is never documented anywhere I think (unfortunately), but depend on the feature string we are using different code path (and UI) to show the opened window. Maybe only one of that has this problem?

Comment 8

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

mozbrowseropenwindow event is handled in ChildWindowFactory and setVisible() should be called by AppTransitionController. We must have some rule somewhere prevent setVisible(false) being called there, to work around maybe keeping the rendering or memory priority before. Find it and remove that and this bug will be fixed.

Does this bug means any kind of workaround that delays setVisible(false) is considered a security risk and should never be used in the future?

Comment 9

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

(In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18; please ni? to queue) from comment #8)
> mozbrowseropenwindow event is handled in ChildWindowFactory and setVisible()
> should be called by AppTransitionController. We must have some rule
> somewhere prevent setVisible(false) being called there, to work around maybe
> keeping the rendering or memory priority before. Find it and remove that and
> this bug will be fixed.
> 
> Does this bug means any kind of workaround that delays setVisible(false) is
> considered a security risk and should never be used in the future?

Correction: AppTransitionController instance on the ChildWindow instance (the popup) only control the visibility of the popup itself. The problem here [1] compare to [2] is explained in the comment on [1]: we don't have a "ChildWindowManager" to transition away (and setVisible(false) the underlining window.

[1] https://github.com/mozilla-b2g/gaia/blob/77d463a009a1425e413edaae92b237e116708560/apps/system/js/popup_window.js#L105-L114
[2] https://github.com/mozilla-b2g/gaia/blob/77d463a009a1425e413edaae92b237e116708560/apps/system/js/app_window.js#L1937-L1950

It might still possible to fix this by overloading the ChildWindowFactory and ask it to take some responsibility of the ChildWindowManager for now ...

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18; please ni? to queue) from comment #8)
> mozbrowseropenwindow event is handled in ChildWindowFactory and setVisible()
> should be called by AppTransitionController. We must have some rule
> somewhere prevent setVisible(false) being called there, to work around maybe
> keeping the rendering or memory priority before. Find it and remove that and
> this bug will be fixed.
> 
> Does this bug means any kind of workaround that delays setVisible(false) is
> considered a security risk and should never be used in the future?

Since the rendering cache purging behavior has been fixed, IMO setVisible(false) has no reason to be delayed.

Comment 11

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

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #10)
> (In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18; please ni? to
> queue) from comment #8)
> > mozbrowseropenwindow event is handled in ChildWindowFactory and setVisible()
> > should be called by AppTransitionController. We must have some rule
> > somewhere prevent setVisible(false) being called there, to work around maybe
> > keeping the rendering or memory priority before. Find it and remove that and
> > this bug will be fixed.
> > 
> > Does this bug means any kind of workaround that delays setVisible(false) is
> > considered a security risk and should never be used in the future?
> 
> Since the rendering cache purging behavior has been fixed, IMO
> setVisible(false) has no reason to be delayed.

Right, but I am sorry, comment 8 is actually not the cause of this bug.

Comment 12

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

The fix turned out to be quite easy...

Comment 13

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

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

The reason this is easier than what I've thought is because there can be only one frontWindow per AppWindow, so if a frame attempts to window.open() many times, the last popup will replace the previous ones.

Like wise, if an popup opens another popup, itself will not be replaced but instead itself will become the rearWindow of the new frontWindow.

Therefore, the fix is simple -- make sure the code calls requestOpen|requestClose of the PopupWindow and make sure PopupWindow toggle the visibility of the to-be-hidden rearWindow.

(If this get r+ after Thu noon Taipei time, someone have to merge this for me.)

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8670668 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/32289

Commented on github, I think there's a simpler fix.

Comment 15

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

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

New patch according to your suggestion.

I am taking PTO so please merge this for me.

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 8671123 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/32313

(In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18; please ni? to queue) from comment #15)
> Created attachment 8671123 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/32313
> 
> New patch according to your suggestion.
> 
> I am taking PTO so please merge this for me.

Arg.
Just found a case we're not handling properly.
I'll send a patch and ask Alberto for review not to block this bug.

Comment 17

[Etienne Segonzac (:etienne)]

Attachment: New PR

Comment 18

[Alberto Pastor [:albertopq]]

Comment on attachment 8671254 [details] [review]
New PR

LGTM. Thanks!

Comment 19

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/ea4dd0d9149f28c6daf3a627be47000b23336ca0

Thanks!