Closed Bug 1313239 Opened 8 years ago Closed 7 years ago

[webvr] Emit `vrdisplayactivate` event during document navigation

Categories

(Core :: WebVR, defect)

Product:
Core
Component:
WebVR
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kip, Assigned: kip)

References

(
URL
)

Details

(Whiteboard: [webvr])

Attachments

(1 file)

bug 1313239 - Emit vrdisplayactivate event during document navigation
59 bytes, text/x-review-board-request
smaug
: review+
Details 
When a VR presentation is active and the URI changes, the destination page should see a navigator.onvrdisplayactivate event fire with a reason of "navigation" and the VRDisplay to present to passed in with VRDisplayEvent.
Summary: [webvr] Trigger Navigator.onvrdisplayactivate during link traversal → [webvr] Emit `vrdisplayactivate` event during document navigation
Whiteboard: [webvr]
Attachment #8839707 - Flags: review?(bugs)
This is my first time touching DocShell and nsDocumentViewer.  Please advise if there is a better place to persist the VR display ID for navigation while in WebVR.
Flags: needinfo?(bugs)
So the new page should get vrdisplayactivate event if previous page was using VR?
That is quite surprising. Do we do anything like that with other features in the web platform?

Anyhow, if we really want this, it feels like the display ID should be stored in the outer window, since that object is reused (it has 1-1 mapping with nsDocShell, but outer window is kind of closer to DOM).

Trying to find the relevant spec. It is not https://w3c.github.io/webvr/ but the older spec...
Flags: needinfo?(bugs)
But it doesn't really spec vrdisplayactivate for this case.

Any links to some spec helping with the review?
Comment on attachment 8839707 [details]
bug 1313239 - Emit vrdisplayactivate event during document navigation

https://reviewboard.mozilla.org/r/114278/#review116062

This is most probably not the right way to do this (storing the relevant information in outer window should be easier), but lack of a proper spec makes me just guess what should happen.
Attachment #8839707 - Flags: review?(bugs) → review-
I'd expect a spec to say exactly when the event should be dispatched. Like, is it synchronously right after load event, also after the load event for the containing <iframe>? Or should it actually happen asynchronously?
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #5)
> So the new page should get vrdisplayactivate event if previous page was
> using VR?
Yes, this is correct.  It should also apply during forward-backward navigation and refresh.

> That is quite surprising. Do we do anything like that with other features in
> the web platform?
This design was carefully chosen to enable navigation from one VR environment to another without leaving VR presentation and without allowing other content to take over your view.  There are multitasking scenarios to consider such as multitasking with 2d browser windows within a VR environment.  Following a link within the non-vr window should not trigger vr presentation in the linked page; however, following a link within the background vr presentation should trigger vr presentation, causing the 3d environment to be replaced.

> 
> Anyhow, if we really want this, it feels like the display ID should be
> stored in the outer window, since that object is reused (it has 1-1 mapping
> with nsDocShell, but outer window is kind of closer to DOM).
> 
> Trying to find the relevant spec. It is not https://w3c.github.io/webvr/ but
> the older spec...
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #9)
> I'd expect a spec to say exactly when the event should be dispatched. Like,
> is it synchronously right after load event, also after the load event for
> the containing <iframe>? Or should it actually happen asynchronously?
I am attempting to match the behavior of browsers that have already shipped; however, I agree regarding the spec and will file a spec issue.
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #6)
> Maybe it was https://w3c.github.io/webvr/archive/prerelease/1.1/
This is the correct and currently shipping spec, WebVR 1.1.  There was an error in updating the site which is being corrected to reflect that WebVR 2.0 is WIP and WebVR 1.1 is the current spec.
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #8)
> Comment on attachment 8839707 [details]
> bug 1313239 - Emit vrdisplayactivate event during document navigation
> 
> https://reviewboard.mozilla.org/r/114278/#review116062
> 
> This is most probably not the right way to do this (storing the relevant
> information in outer window should be easier), but lack of a proper spec
> makes me just guess what should happen.

Thanks for the feedback.  I'll update the patch to use the outer window and re-submit.
Do you know exactly when other implementations dispatch the event?
Perhaps links to the relevant source code?
Blocks: 1254776
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #14)
> Do you know exactly when other implementations dispatch the event?
> Perhaps links to the relevant source code?

After some investigation in the Chromium source and asking around, I found that Chromium does not yet emit vrdisplayactivate for link traversal -- only in response to the proximity sensor that detects the user wearing the HMD.  Carmel browser also does not trigger this yet.

When vrdisplayactivate is triggered by the proximity sensor, it could happen at any time.  (With "reason" passed in as "mounted" in the event).

I'll file a spec bug to formalize the order of events for the link traversal case, which seems natural to  happen serially after "load" fires.
(In reply to :kip (Kearwood Gilbert) from comment #15)
> (In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment
> #14)
> > Do you know exactly when other implementations dispatch the event?
> > Perhaps links to the relevant source code?
> 
> After some investigation in the Chromium source and asking around, I found
> that Chromium does not yet emit vrdisplayactivate for link traversal -- only
> in response to the proximity sensor that detects the user wearing the HMD. 
> Carmel browser also does not trigger this yet.
> 
> When vrdisplayactivate is triggered by the proximity sensor, it could happen
> at any time.  (With "reason" passed in as "mounted" in the event).
> 
> I'll file a spec bug to formalize the order of events for the link traversal
> case, which seems natural to  happen serially after "load" fires.

Spec issue is here:

https://github.com/w3c/webvr/issues/193
I have updated the patch to use the outer window...  Much simpler now, thanks!

Also, the Chrome team has agreed on the ordering of the events, which will be made explicit in the spec.
I really wish there was some more concrete proposal for the spec (like a PR or so), but ok, reviewing
I think this feature needs some sort of security review before landing, since it feels awkward to leak a state from the previous page.
So, please ask another review from someone in security team.
Comment on attachment 8839707 [details]
bug 1313239 - Emit vrdisplayactivate event during document navigation

https://reviewboard.mozilla.org/r/114278/#review116712

::: dom/base/nsGlobalWindow.cpp:13524
(Diff revision 3)
>      mNavigator->NotifyActiveVRDisplaysChanged();
>    }
>  }
>  
> +uint32_t
> +nsGlobalWindow::GetAutoActivateVRDisplayID()

I think I'd prefer GetAutoActivateVRDisplayID()
which would do something like
uint32_t retVal = mAutoActivateVRDisplayID;
mAutoActivateVRDisplayID = 0;
return retVal;

Then one wouldn't need to explicitly call 
SetAutoActivateVRDisplayID(0)

::: dom/base/nsGlobalWindow.cpp:13548
(Diff revision 3)
>    for (auto display : mVRDisplays) {
> -    if (display->DisplayId() == aDisplayID
> -        && !display->IsAnyPresenting()) {
> +    if (display->DisplayId() == aDisplayID) {
> +      if (aReason != VRDisplayEventReason::Navigation &&
> +          display->IsAnyPresenting()) {
> -      // We only want to trigger this event if nobody is presenting to the
> +        // We only want to trigger this event if nobody is presenting to the
> -      // display already.
> +        // display already or during navigation away from a page with an

This comment isn't right. We aren't dispatching the event when navigating away, but when we just navigated to some page.
Attachment #8839707 - Flags: review?(bugs) → review+
NI'ing :arroway for triage of security review.

TL;DR for security, copied from IRC:

I have been asked to have someone on Security take a look at my patch for Bug 1313239 before landing.  It's a pretty small change, but involves persisting state during link traversal.

Essentially, we wish to inform pages that are linked to from a page that is presenting to the VR headset that they should present to the headset after they are loaded.  We don't want pages to do this unless linked to from another VR page, so they don't take over the headset that may be used by content outside the browser.

So the information passed from page to page is an "id" (sequentially increasing) identifying the headset to present to, which if non-zero will trigger an event in the destination page during traversal
Flags: needinfo?(stephouillon)
This is planned for release in Firefox 54.  Will there be time to review soon?
I had a look at it and think it's fine, but I'm asking pauljt for a second advice.
Flags: needinfo?(stephouillon) → needinfo?(ptheriault)
Component: Graphics → WebVR
Rebased patch to avoid bitrot
Thanks Stephanie! Paul, Do you know when you will have some time to review this? This patch enables link traversal in VR and we're going to make a PR push associated to the feature. Thanks!
Sorry this is on my radar, but Ive been swamped. I'll get to it today.
Flags: needinfo?(ptheriault)
I can't think of any risk scenarios that are a problem here. I've considered:

- Advertisement trying to track a user between pages: this event doesn't really gain trackers anything since there is no place to store information, tracking is already possible without this. 
- Fingerprinting: the presence of webvr hardware at all would be more of a fingerprinting risk
- same-origin bypass: as far as I understand there isn't any session information attached to this event (page receiving the event during load doesn't get to read parameters from the existing VR session, has to renegotiate parameters (resolution, frame buffer etc)

Thats about all the threats I can think of so this would seem ok to me.
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f4fd0bbc9a0
Emit vrdisplayactivate event during document navigation r=smaug
https://hg.mozilla.org/mozilla-central/rev/5f4fd0bbc9a0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: