Closed Bug 791261 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+)

VERIFIED FIXED
blocking-basecamp +

People

(Reporter: etienne, Assigned: justin.lebar+bug)

References

(Blocks 1 open bug)

Details

(Whiteboard: [LOE:S])

Attachments

(2 files)

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.
The dialer (incoming calls) and the clock app (alarms) are concerned by this.
If an OOP window calls navigator.vibrate, does that work?  That is, does window.open have to be involved here?
(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
blocking-basecamp: --- → ?
Justin, can you take a look here?
Assignee: nobody → justin.lebar+bug
blocking-basecamp: ? → +
(In reply to Andrew Overholt [:overholt] from comment #4)
> Justin, can you take a look here?

Definitely.
This is happening because TabParent::Active() is returning false.
...which happens because the popup window is never focused.

If you click on the "focus me!" textbox, the vibrator starts up.
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.
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 9 sounds like a pretty good workaround to me. the security implications are really low for device vibration
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.)
Whiteboard: [LOE:S]
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.
(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?
(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.
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?
(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.
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?
> 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.
(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.
Attached patch Patch, v1Splinter Review
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.
Attachment #662388 - Flags: review?(jones.chris.g)
(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 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.
Attachment #662388 - Flags: review?(jones.chris.g) → review+
(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?
Yes, a bug to fix what this check was trying to do.
https://hg.mozilla.org/mozilla-central/rev/a82d8da3e983
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 792891
Okay, I filed bug 792891.
Verified fix on 10-01 daily build.  vibration works on incoming call.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.