[webvr] Add `display` value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
The WebVR 1.1 Spec includes `display` and `reason` attributes in VRDisplayEvent.

Bug 1293333 adds VRDisplayEvent but does not set a value for `display` as expected for Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

This is a WIP patch, don't commit
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
Added dependency to Bug 1337441, as these events will have the same issue.
Depends on: 1337441
(Assignee)

Comment 6

2 years ago
I have done some manual testing to ensure these events are working properly.  These patches are ready for review, but should land after Bug 1337441.
(Assignee)

Updated

2 years ago
Attachment #8834715 - Flags: review?(continuation)
Attachment #8834715 - Flags: review?(continuation) → review?(bugs)
I don't really know anything about events. Olli could review this.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review113048

::: dom/base/nsGlobalWindow.cpp:13468
(Diff revision 3)
>  }
>  
> +void
> +nsGlobalWindow::DispatchVRDisplayConnect(uint32_t aDisplayID)
> +{
> +  for (auto display : mVRDisplays) {

Since event dispatching tends to cause event listeners to be called, and those may do anything, like close windows and what not, it is guaranteed that this ranged-loop is doing the right thing?

Luckily we don't crash anymore when using ranged loop with nsTArray, but the result can be still unexpected.

::: dom/base/nsGlobalWindow.cpp:13479
(Diff revision 3)
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplayconnect"),

I don't see vrdisplayconnect anywhere in https://w3c.github.io/webvr/ so can't really review this. Which spec defines this stuff?

https://w3c.github.io/webvr/#vr-events talks about displayconnect, not about vrdisplayconnect. And the event should be dispatched to VR object, not to Window

::: dom/base/nsGlobalWindow.cpp:13480
(Diff revision 3)
> +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplayconnect"),
> +                                    init);

You want to make the event trusted before dispatching.
SetTrusted(true)

::: dom/base/nsGlobalWindow.cpp:13491
(Diff revision 3)
> +}
> +
> +void
> +nsGlobalWindow::DispatchVRDisplayDisconnect(uint32_t aDisplayID)
> +{
> +  for (auto display : mVRDisplays) {

Again, is this loop safe and doing what you expect it to do in case some event listener ends up changing the content of the array?

::: dom/base/nsGlobalWindow.cpp:13502
(Diff revision 3)
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplaydisconnect
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplaydisconnect"),

Don't see this in the spec.

::: dom/base/nsGlobalWindow.cpp:13504
(Diff revision 3)
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplaydisconnect"),
> +                                    init);
> +      bool defaultActionEnabled;

The event should be marked trusted.

::: dom/base/nsGlobalWindow.cpp:13514
(Diff revision 3)
> +}
> +
> +void
> +nsGlobalWindow::DispatchVRDisplayPresentChange(uint32_t aDisplayID)
> +{
> +  for (auto display : mVRDisplays) {

And here. Is this safe loop? And if so, why?

::: dom/base/nsGlobalWindow.cpp:13525
(Diff revision 3)
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplaypresentchange
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplaypresentchange"),

Don't see this event in the spec.

::: dom/base/nsGlobalWindow.cpp:13527
(Diff revision 3)
> +
> +      RefPtr<VRDisplayEvent> event =
> +        VRDisplayEvent::Constructor(this,
> +                                    NS_LITERAL_STRING("vrdisplaypresentchange"),
> +                                    init);
> +      bool defaultActionEnabled;

event should be marked trusted

::: dom/vr/VREventObserver.cpp
(Diff revision 3)
>     * content.
>     */
>    if (mWindow->AsInner()->IsCurrentInnerWindow()) {
>      MOZ_ASSERT(nsContentUtils::IsSafeToRunScript());
> -    mWindow->GetOuterWindow()->DispatchCustomEvent(
> +    mWindow->DispatchVRDisplayConnect(aDisplayID);
> -      NS_LITERAL_STRING("vrdisplayconnected"));

oh, I see the old code dispatches these events too. Why is the old code right?
Why does it dispatch events not defined in the spec?
Or am I looking at wrong spec?
Attachment #8834715 - Flags: review?(bugs) → review-
(Assignee)

Comment 9

2 years ago
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8834715 [details]
> Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect,
> Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events
> 
> https://reviewboard.mozilla.org/r/110556/#review113048
> 
> ::: dom/base/nsGlobalWindow.cpp:13468
> (Diff revision 3)
> >  }
> >  
> > +void
> > +nsGlobalWindow::DispatchVRDisplayConnect(uint32_t aDisplayID)
> > +{
> > +  for (auto display : mVRDisplays) {
> 
> Since event dispatching tends to cause event listeners to be called, and
> those may do anything, like close windows and what not, it is guaranteed
> that this ranged-loop is doing the right thing?
> 
> Luckily we don't crash anymore when using ranged loop with nsTArray, but the
> result can be still unexpected.
> 
> ::: dom/base/nsGlobalWindow.cpp:13479
> (Diff revision 3)
> > +      init.mDisplay = display;
> > +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplayconnect"),
> 
> I don't see vrdisplayconnect anywhere in https://w3c.github.io/webvr/ so
> can't really review this. Which spec defines this stuff?
> 
> https://w3c.github.io/webvr/#vr-events talks about displayconnect, not about
> vrdisplayconnect. And the event should be dispatched to VR object, not to
> Window
> 
> ::: dom/base/nsGlobalWindow.cpp:13480
> (Diff revision 3)
> > +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplayconnect"),
> > +                                    init);
> 
> You want to make the event trusted before dispatching.
> SetTrusted(true)
> 
> ::: dom/base/nsGlobalWindow.cpp:13491
> (Diff revision 3)
> > +}
> > +
> > +void
> > +nsGlobalWindow::DispatchVRDisplayDisconnect(uint32_t aDisplayID)
> > +{
> > +  for (auto display : mVRDisplays) {
> 
> Again, is this loop safe and doing what you expect it to do in case some
> event listener ends up changing the content of the array?
> 
> ::: dom/base/nsGlobalWindow.cpp:13502
> (Diff revision 3)
> > +      init.mDisplay = display;
> > +      // VRDisplayEvent.reason is not set for onvrdisplaydisconnect
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplaydisconnect"),
> 
> Don't see this in the spec.
> 
> ::: dom/base/nsGlobalWindow.cpp:13504
> (Diff revision 3)
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplaydisconnect"),
> > +                                    init);
> > +      bool defaultActionEnabled;
> 
> The event should be marked trusted.
> 
> ::: dom/base/nsGlobalWindow.cpp:13514
> (Diff revision 3)
> > +}
> > +
> > +void
> > +nsGlobalWindow::DispatchVRDisplayPresentChange(uint32_t aDisplayID)
> > +{
> > +  for (auto display : mVRDisplays) {
> 
> And here. Is this safe loop? And if so, why?
> 
> ::: dom/base/nsGlobalWindow.cpp:13525
> (Diff revision 3)
> > +      init.mDisplay = display;
> > +      // VRDisplayEvent.reason is not set for onvrdisplaypresentchange
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplaypresentchange"),
> 
> Don't see this event in the spec.
> 
> ::: dom/base/nsGlobalWindow.cpp:13527
> (Diff revision 3)
> > +
> > +      RefPtr<VRDisplayEvent> event =
> > +        VRDisplayEvent::Constructor(this,
> > +                                    NS_LITERAL_STRING("vrdisplaypresentchange"),
> > +                                    init);
> > +      bool defaultActionEnabled;
> 
> event should be marked trusted
> 
> ::: dom/vr/VREventObserver.cpp
> (Diff revision 3)
> >     * content.
> >     */
> >    if (mWindow->AsInner()->IsCurrentInnerWindow()) {
> >      MOZ_ASSERT(nsContentUtils::IsSafeToRunScript());
> > -    mWindow->GetOuterWindow()->DispatchCustomEvent(
> > +    mWindow->DispatchVRDisplayConnect(aDisplayID);
> > -      NS_LITERAL_STRING("vrdisplayconnected"));
> 
> oh, I see the old code dispatches these events too. Why is the old code
> right?
> Why does it dispatch events not defined in the spec?
> Or am I looking at wrong spec?

Thanks for reviewing!

The spec at the URL attached to this bug has very recently been updated.

The version we are implementing is here:

https://w3c.github.io/webvr/archive/prerelease/1.1/

Sorry for any confusion.
Why are we implementing some old version of the spec?
or I guess one could ask, why is the new version of the spec totally incompatible with the old one?
But yes, if we're implementing the old one, and everyone else too, and the new spec isn't something anyone will implement, then the patch looks better :)
However please add the SetTrusted stuff, and explain why those array iterations are ok.

And file spec bugs to make the new spec to follow reality.
(Assignee)

Comment 13

2 years ago
(In reply to Olli Pettay [:smaug] from comment #10)
> Why are we implementing some old version of the spec?


The WebVR 1.1 spec is the one implemented in all shipping WebVR browsers, and the one that content has been authored for.  The WebVR 1.2 spec is still in active development and is not ready for implementation yet -- the github landing page is misleading and I was surprised to see it had been updated when I saw your comment.

Our intention is to support WebVR 1.1 with FF 54, and implement the updated spec once the spec work has been completed.  Forward and backwards compatibility will be provided by a polyfill, which will be shared between browser vendors and distributed as a system add-on.  There are breaking changes in WebVR 1.2 primarily to support WebVR in webworkers and augmented reality.  I meet bi-weekly with the other implementers in the WebVR working group and will see if we can update the site to reflect the accurate state of WebVR 1.1 and WebVR 1.2.

Thanks for your other comments in the review.  I'll update the patch based on your feedback.
(Assignee)

Comment 14

2 years ago
(In reply to Olli Pettay [:smaug] from comment #12)
> But yes, if we're implementing the old one, and everyone else too, and the
> new spec isn't something anyone will implement, then the patch looks better
> :)
> However please add the SetTrusted stuff, and explain why those array
> iterations are ok.
> 
> And file spec bugs to make the new spec to follow reality.

Thanks again, I will update the patch and follow up with the WebVR working group on the spec.
(Assignee)

Comment 15

2 years ago
(In reply to :kip (Kearwood Gilbert) from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #12)
> > But yes, if we're implementing the old one, and everyone else too, and the
> > new spec isn't something anyone will implement, then the patch looks better
> > :)
> > However please add the SetTrusted stuff, and explain why those array
> > iterations are ok.
> > 
> > And file spec bugs to make the new spec to follow reality.
> 
> Thanks again, I will update the patch and follow up with the WebVR working
> group on the spec.

There was consensus in the latest WebVR implementers meeting to change the landing page to reflect that WebVR 1.1 is the current version of the spec.

The WebVR 1.2 spec will also be renamed to WebVR 2.0 in order to reflect that it is a new spec that will not be backwards compatible with WebVR 1.1.
Sounds super weird. Why break backwards compatibility when there isn't need to (at least in this event handler case), and that forces browsers then support both v1.1 and 2.0.
Comment hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review113048

> I don't see vrdisplayconnect anywhere in https://w3c.github.io/webvr/ so can't really review this. Which spec defines this stuff?
> 
> https://w3c.github.io/webvr/#vr-events talks about displayconnect, not about vrdisplayconnect. And the event should be dispatched to VR object, not to Window

The WebVR Spec github page will be updated to reflect the proper status of the 1.1 spec, which is being implemented here.

> You want to make the event trusted before dispatching.
> SetTrusted(true)

Only VRDisplayActivate is expected to be treated as a "user gesture", so I have intentionally called SetTrusted(true) only on that one.  Could I be misunderstanding SetTrusted()?

> Again, is this loop safe and doing what you expect it to do in case some event listener ends up changing the content of the array?

We break out of the loop after dispatching the event and don't call any member variables after the dispatch.  I've updated with some comments and changed "break" to "return" in case someone comes by and adds more code after the loop without taking this into consideration.

> The event should be marked trusted.

See above comment.  I don't wish VRDisplayDisconnect to be treated as a user gesture.

> event should be marked trusted

See comments above regarding user gestures.
UA dispatched events should be by default be trusted. Nothing, or very little to do with 'user gesture'.
(Assignee)

Comment 20

2 years ago
(In reply to Olli Pettay [:smaug] from comment #19)
> UA dispatched events should be by default be trusted. Nothing, or very
> little to do with 'user gesture'.
Thanks for clarifying!  The spec doesn't preclude us from making all of these events trusted.

I'll update the patch once more.

Comment 21

2 years ago
mozreview-review
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review114356

::: dom/base/nsGlobalWindow.cpp:13541
(Diff revision 4)
> +  // event if found.
> +  for (auto display : mVRDisplays) {
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;

Nothing in the spec says the event should bubble. So, false here.

::: dom/base/nsGlobalWindow.cpp:13543
(Diff revision 4)
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;

The spec doesn't actually say mDisplay should be initialized, but I vaguely implies that. Want to file a spec bug?

::: dom/base/nsGlobalWindow.cpp:13546
(Diff revision 4)
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> +
> +      RefPtr<VRDisplayEvent> event =

This should be marked trusted

::: dom/base/nsGlobalWindow.cpp:13568
(Diff revision 4)
> +  // event if found.
> +  for (auto display : mVRDisplays) {
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;

I don't see anything in the spec saying the event should bubble. So, = false;

::: dom/base/nsGlobalWindow.cpp:13571
(Diff revision 4)
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplaydisconnect

the event name isn't onvrdisplaydisconnect, but
vrdisplaydisconnect

::: dom/base/nsGlobalWindow.cpp:13595
(Diff revision 4)
> +  // event if found.
> +  for (auto display : mVRDisplays) {
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;

= false; for the bubbles

::: dom/base/nsGlobalWindow.cpp:13597
(Diff revision 4)
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;

The spec doesn't say .display should be set to any value, but I guess vaguely implies that.

::: gfx/vr/ipc/VRManagerChild.cpp:570
(Diff revision 4)
>  
>  void
> -VRManagerChild::FireDOMVRDisplayConnectEventInternal()
> +VRManagerChild::FireDOMVRDisplayConnectEventInternal(uint32_t aDisplayID)
>  {
>    for (auto& listener : mListeners) {
> -    listener->NotifyVRDisplayConnect();
> +    listener->NotifyVRDisplayConnect(aDisplayID);

Not a new issue from this patch, but what if dispatching the event modifies mListeners array?

If entries after the current entry are removed from mListeners, the process crash (safely). I don't think we want that here.
And since event listeners may do whatever, like spin event loop, GC and CC may run and nsGlobalWindows get deleted...

I think you want to take a copy of the array
(nsTArray<RefPtr<VREventObserver>>)
Attachment #8834715 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 years ago
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8834715 [details]
> Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect,
> Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events
> 
> https://reviewboard.mozilla.org/r/110556/#review114356
> 
> ::: dom/base/nsGlobalWindow.cpp:13541
> (Diff revision 4)
> > +  // event if found.
> > +  for (auto display : mVRDisplays) {
> > +    if (display->DisplayId() == aDisplayID) {
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> 
> Nothing in the spec says the event should bubble. So, false here.
> 
> ::: dom/base/nsGlobalWindow.cpp:13543
> (Diff revision 4)
> > +    if (display->DisplayId() == aDisplayID) {
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> > +      init.mCancelable = false;
> > +      init.mDisplay = display;
> 
> The spec doesn't actually say mDisplay should be initialized, but I vaguely
> implies that. Want to file a spec bug?
> 
> ::: dom/base/nsGlobalWindow.cpp:13546
> (Diff revision 4)
> > +      init.mBubbles = true;
> > +      init.mCancelable = false;
> > +      init.mDisplay = display;
> > +      // VRDisplayEvent.reason is not set for onvrdisplayconnect
> > +
> > +      RefPtr<VRDisplayEvent> event =
> 
> This should be marked trusted
> 
> ::: dom/base/nsGlobalWindow.cpp:13568
> (Diff revision 4)
> > +  // event if found.
> > +  for (auto display : mVRDisplays) {
> > +    if (display->DisplayId() == aDisplayID) {
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> 
> I don't see anything in the spec saying the event should bubble. So, = false;
> 
> ::: dom/base/nsGlobalWindow.cpp:13571
> (Diff revision 4)
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> > +      init.mCancelable = false;
> > +      init.mDisplay = display;
> > +      // VRDisplayEvent.reason is not set for onvrdisplaydisconnect
> 
> the event name isn't onvrdisplaydisconnect, but
> vrdisplaydisconnect
> 
> ::: dom/base/nsGlobalWindow.cpp:13595
> (Diff revision 4)
> > +  // event if found.
> > +  for (auto display : mVRDisplays) {
> > +    if (display->DisplayId() == aDisplayID) {
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> 
> = false; for the bubbles
> 
> ::: dom/base/nsGlobalWindow.cpp:13597
> (Diff revision 4)
> > +    if (display->DisplayId() == aDisplayID) {
> > +      // Fire event even if not presenting to the display.
> > +      VRDisplayEventInit init;
> > +      init.mBubbles = true;
> > +      init.mCancelable = false;
> > +      init.mDisplay = display;
> 
> The spec doesn't say .display should be set to any value, but I guess
> vaguely implies that.
> 
> ::: gfx/vr/ipc/VRManagerChild.cpp:570
> (Diff revision 4)
> >  
> >  void
> > -VRManagerChild::FireDOMVRDisplayConnectEventInternal()
> > +VRManagerChild::FireDOMVRDisplayConnectEventInternal(uint32_t aDisplayID)
> >  {
> >    for (auto& listener : mListeners) {
> > -    listener->NotifyVRDisplayConnect();
> > +    listener->NotifyVRDisplayConnect(aDisplayID);
> 
> Not a new issue from this patch, but what if dispatching the event modifies
> mListeners array?
> 
> If entries after the current entry are removed from mListeners, the process
> crash (safely). I don't think we want that here.
> And since event listeners may do whatever, like spin event loop, GC and CC
> may run and nsGlobalWindows get deleted...
> 
> I think you want to take a copy of the array
> (nsTArray<RefPtr<VREventObserver>>)

Thanks for the feedback!  I'll update the patch.

Comment 24

2 years ago
mozreview-review
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review114494

Did you upload a patch with only some fixes?

::: dom/base/nsGlobalWindow.cpp:13514
(Diff revision 5)
>      if (display->DisplayId() == aDisplayID && display->IsPresenting()) {
>        // We only want to trigger this event to content that is presenting to
>        // the display already.
>  
>        VRDisplayEventInit init;
>        init.mBubbles = true;

Btw, shouldn't this be mBubbles = false too?

::: dom/base/nsGlobalWindow.cpp:13545
(Diff revision 5)
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplayconnect

There is no onvrdisplayconnect, but
vrdisplayconnect

::: dom/base/nsGlobalWindow.cpp:13570
(Diff revision 5)
> +  // event if found.
> +  for (auto display : mVRDisplays) {
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;

mBubbles = false;

::: dom/base/nsGlobalWindow.cpp:13598
(Diff revision 5)
> +  // event if found.
> +  for (auto display : mVRDisplays) {
> +    if (display->DisplayId() == aDisplayID) {
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;

mBubbles = false

::: dom/base/nsGlobalWindow.cpp:13601
(Diff revision 5)
> +      // Fire event even if not presenting to the display.
> +      VRDisplayEventInit init;
> +      init.mBubbles = true;
> +      init.mCancelable = false;
> +      init.mDisplay = display;
> +      // VRDisplayEvent.reason is not set for onvrdisplaypresentchange

there is no event called onvrdisplaypresentchange, but
vrdisplaypresentchange

::: gfx/vr/ipc/VRManagerChild.cpp:569
(Diff revision 5)
>  }
>  
>  void
> -VRManagerChild::FireDOMVRDisplayConnectEventInternal()
> +VRManagerChild::FireDOMVRDisplayConnectEventInternal(uint32_t aDisplayID)
>  {
>    for (auto& listener : mListeners) {

So all this mListeners iteration is crash-prone as I said, so make sure to fix in some patch.
Attachment #8834715 - Flags: review?(bugs) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review113048

> Since event dispatching tends to cause event listeners to be called, and those may do anything, like close windows and what not, it is guaranteed that this ranged-loop is doing the right thing?
> 
> Luckily we don't crash anymore when using ranged loop with nsTArray, but the result can be still unexpected.

I have corrected this, thanks for finding it!  I needed to also add RefCounting to VREventListener and ensure that it's weak reference to nsGlobalWindow is cleared to prevent UAF in this scenario.

Comment 27

2 years ago
mozreview-review
Comment on attachment 8834715 [details]
Bug 1335606 - Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events

https://reviewboard.mozilla.org/r/110556/#review115198
Attachment #8834715 - Flags: review?(bugs) → review+

Comment 28

2 years ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af8dc8f5668c
Add 'display' value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events r=smaug

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af8dc8f5668c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.