Closed
Bug 1335606
Opened 7 years ago
Closed 7 years ago
[webvr] Add `display` value to Navigator.vrdisplayconnect, Navigator.vrdisplaydisconnect, and Navigator.vrdisplaypresentchange events
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: kip, Assigned: kip)
References
()
Details
Attachments
(1 file)
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•7 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•7 years ago
|
||
Added dependency to Bug 1337441, as these events will have the same issue.
Depends on: 1337441
Assignee | ||
Comment 6•7 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•7 years ago
|
Attachment #8834715 -
Flags: review?(continuation)
Updated•7 years ago
|
Attachment #8834715 -
Flags: review?(continuation) → review?(bugs)
Comment 7•7 years ago
|
||
I don't really know anything about events. Olli could review this.
Comment 8•7 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•7 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.
Comment 10•7 years ago
|
||
Why are we implementing some old version of the spec?
Comment 11•7 years ago
|
||
or I guess one could ask, why is the new version of the spec totally incompatible with the old one?
Comment 12•7 years ago
|
||
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•7 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•7 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•7 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.
Comment 16•7 years ago
|
||
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•7 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.
Comment 19•7 years ago
|
||
UA dispatched events should be by default be trusted. Nothing, or very little to do with 'user gesture'.
Assignee | ||
Comment 20•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af8dc8f5668c
Status: NEW → RESOLVED
Closed: 7 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.
Description
•