791261 - navigator.vibrate not working from a window opened by an OOP app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Etienne Segonzac (:etienne)]

Attachment: Test case, gaia patch

STR:

- Apply the gaia patch
- Open the "UI Tests" app
- Select "window.open tests"
- Select "window.open(url, name, 'attention')"
- The opened page calls |navigator.vibrate| and logs the |document.mozHidden| value

Expected:
- The phone vibrates, since mozHidden is false

Actual:
- The phone does not vibrate
- If you touch the screen then the phone starts vibrating

Note that when the "UI tests" app is in process the phone vibrates as expected.

Comment 1

[Etienne Segonzac (:etienne)]

The dialer (incoming calls) and the clock app (alarms) are concerned by this.

Comment 2

[Justin Lebar (not reading bugmail)]

If an OOP window calls navigator.vibrate, does that work?  That is, does window.open have to be involved here?

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> If an OOP window calls navigator.vibrate, does that work?  That is, does
> window.open have to be involved here?

yep

Comment 4

[Andrew Overholt [:overholt]]

Justin, can you take a look here?

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to Andrew Overholt [:overholt] from comment #4)
> Justin, can you take a look here?

Definitely.

Comment 6

[Justin Lebar (not reading bugmail)]

This is happening because TabParent::Active() is returning false.

Comment 7

[Justin Lebar (not reading bugmail)]

...which happens because the popup window is never focused.

If you click on the "focus me!" textbox, the vibrator starts up.

Comment 8

[Justin Lebar (not reading bugmail)]

We could solve this by automatically focusing popup windows, but I don't think that's the right approach.  For example, a browser might want to show a popup window in a background tab.

It's tempting to say that we should focus the frame when we setActive(true) and blur the frame when we setActive(false).  But we have one important situation where we have multiple, non-nested active frames visible concurrently: Keyboard entry.  When we show the keyboard, we make some mozbrowser visible (and presumably setActive(true)).  But we shouldn't focus the keyboard; we want to leave the other frame focused.

Maybe we could do something like make setActive(true) give the relevant frame focus if nobody else has focus.  But in a multi-process world, it's unclear how to do that without race conditions.  And I'm also not a fan of the footgun of calling setActive in the wrong order and having the vibrator mysteriously stop working.

So I'm not sure what to do here other than to add a browser API method which lets you focus a mozbrowser's content and rely on Gaia using it correctly.

Comment 9

[Justin Lebar (not reading bugmail)]

I guess another alternative would be to remove the TabParent->Active() check in the vibrator code.

That check is there to avoid the race condition in which a tab is deactivated but the child hasn't yet been informed and also to prevent misbehaving tabs from vibrating (that is, because we don't trust the child process to respect our is-visible checks).

Comment 10

[Andreas Gal :gal]

comment 9 sounds like a pretty good workaround to me. the security implications are really low for device vibration

Comment 11

[Justin Lebar (not reading bugmail)]

If we took that approach, I'd want to remove TabParent::Active, since you couldn't use it correctly.  That would prevent us from doing any other parent-process "is-foreground" checks on a TabParent, of course.  (At the moment, Active() is called only in one spot.)

Comment 12

[Justin Lebar (not reading bugmail)]

cjones, do you have an opinion here about how we should fix this (comments 8 and 9)?  Removing the Active() check and adding a method to the browser API to focus the frame are both simple changes.

Comment 13

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

(In reply to Justin Lebar [:jlebar] from comment #7)
> ...which happens because the popup window is never focused.
> 
> If you click on the "focus me!" textbox, the vibrator starts up.

Just to be clear, the popup is visible, but not focused, so we deny vibrator?

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> Just to be clear, the popup is visible, but not focused, so we deny vibrator?

Correct.  And the reason is that the relevant TabParent is not Active.  Focusing the popup Activates the TabParent.

Comment 15

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

Hmm ... does that mean that if there's only a single foreground app, and you swipe your finger over the status bar, vibration will be denied to the foreground app?

Comment 16

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> Hmm ... does that mean that if there's only a single foreground app, and you
> swipe your finger over the status bar, vibration will be denied to the
> foreground app?

Possibly?  Probably?

That honestly doesn't sound so bad to me, but I guess it would be worse if we restricted some other permission by is-active.

Comment 17

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

The intent of the original check was visible/not-visible.  I thought the check here was equivalent to the visibility status, but apparently not.  Having vibration randomly start/stop or work/not-work as we focus/blur various visible windows doesn't sound palatable to me.

Do we have the right visible/hidden status in the master process?  I think BEP has it, right?

Comment 18

[Justin Lebar (not reading bugmail)]

> Having vibration randomly start/stop or work/not-work as we focus/blur various visible windows 
> doesn't sound palatable to me.

To be clear, you only get into a mess if you have multiple windows visible from different processes.

I think we could probably use the data BEP has, yes.  That's a little harder, but sounds reasonable to me.

Comment 19

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

(In reply to Justin Lebar [:jlebar] from comment #18)
> > Having vibration randomly start/stop or work/not-work as we focus/blur various visible windows 
> > doesn't sound palatable to me.
> 
> To be clear, you only get into a mess if you have multiple windows visible
> from different processes.
> 

Which happens 100% of the time, basically :).

> I think we could probably use the data BEP has, yes.  That's a little
> harder, but sounds reasonable to me.

I wouldn't knock yourself out over this one; vibration from background tabs is a minor annoyance.  But that seems like the right solution.

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Remove the requirement that a TabParent must be Active in order to send
vibrations.  Also remove TabParent::Active().

The BEP fix was not worth it, I don't think.  If we want to restrict something
else to working only when the TabParent is active, we can re-visit.

Etienne, with this patch, when I load your open('attention') test, the device
immediately starts vibrating.  But when I press "home", it continues vibrating,
and a slice of the 'attention' window is shown at the top of the screen, over
the homescreen.  I presume this isn't at all related to this patch, but I
wanted to check that that's intentional, or at least known.

Comment 21

[Etienne Segonzac (:etienne)]

(In reply to Justin Lebar [:jlebar] from comment #20)
> Etienne, with this patch, when I load your open('attention') test, the device
> immediately starts vibrating.  But when I press "home", it continues
> vibrating,
> and a slice of the 'attention' window is shown at the top of the screen, over
> the homescreen.  I presume this isn't at all related to this patch, but I
> wanted to check that that's intentional, or at least known.

Yes it's intentional.

Comment 22

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

Comment on attachment 662388 [details] [diff] [review]
Patch, v1

OK, fair compromise to me.  Let's file a followup on re-disabling this for hidden (as opposed to un-focused) tabs and get that through triage.

r=me with that.

Comment 23

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> OK, fair compromise to me.  Let's file a followup on re-disabling this for
> hidden (as opposed to un-focused) tabs and get that through triage.

If the child thinks the window is hidden, it won't let the window vibrate.  Do you want a bug so that if the /parent/ thinks the window is hidden, we don't let it vibrate, or is what we have sufficient?

Comment 24

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

Yes, a bug to fix what this check was trying to do.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/a82d8da3e983

Comment 26

[Justin Lebar (not reading bugmail)]

Okay, I filed bug 792891.

Comment 27

[Tony Chung [:tchung]]

Verified fix on 10-01 daily build.  vibration works on incoming call.