Closed Bug 1311802 Opened 8 years ago Closed 7 years ago

[webvr] Implement Mochitest: onvrdisplaydeactivate should only trigger for content that is presenting

Categories

(Core :: WebVR, defect, P3)

Product:
Core
Component:
WebVR
Type:
defect

Tracking

()

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

People

(Reporter: kip, Assigned: cleu)

References

(Blocks 1 open bug)

Details

(Keywords: feature, Whiteboard: [gfx-noted][webvr])

Attachments

(3 files)

Bug 1311802 - Part1 - Add SetMountState interface for VRMockDisplay;
59 bytes, text/x-review-board-request
daoshengmu
: review+
baku
: review+
Details
Bug 1311802 - Part2 - Add Mochitest implementation;
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1311802 - Part3 - Initialize VRHMDSensorstate in VRMockDisplay to prevent crash;
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details 
We need a mochitest to ensure:

The onvrdisplaydeactivate events must never trigger for javascript contexts that are not holding an active VR presentation, even if a VR presentation is held by other contexts in another tab, window, iframe, etc..

This ensures that there is no information leakage on VR presentation state.
Keywords: feature
Priority: -- → P3
Whiteboard: [gfx-noted]
Whiteboard: [gfx-noted] → [gfx-noted][webvr]
Component: Graphics → WebVR
Assignee: nobody → cleu
Comment on attachment 8852768 [details]
Bug 1311802 - Part1 - Add SetMountState interface for VRMockDisplay;

https://reviewboard.mozilla.org/r/124924/#review127528

r=me. the changes in webidl need to seek for a dom peer to review.
Attachment #8852768 - Flags: review?(dmu) → review+
Comment on attachment 8852769 [details]
Bug 1311802 - Part2 - Add Mochitest implementation;

https://reviewboard.mozilla.org/r/124926/#review127522

Please fix the issues that I file and send me a r? again.

::: dom/vr/test/mochitest.ini:11
(Diff revision 1)
>    WebVRHelpers.js
>  
>  [test_vrDisplay_getFrameData.html]
>  [test_vrDisplay_exitPresent.html]
>  [test_vrDisplay_requestPresent.html]
> +[test_vrDisplay_onvrdisplaydeactivate_crosscontent.html]

Please follow the alphabetical order

::: dom/vr/test/test_vrDisplay_onvrdisplaydeactivate_crosscontent.html:24
(Diff revision 1)
> +        var iframe1 = document.getElementById("iframe1").contentWindow;
> +        var t = async_test("vrdisplaydeactivate crosscontent test");
> +
> +        window.addEventListener("vrdisplaydeactivate", () => {
> +          t.step(() => {
> +            assert_true(vrDisplay.isPresenting, "VRDisplay should be presenting now.");

Be presenting now? It is unmounted already why it is presenting?
Attachment #8852769 - Flags: review?(dmu)
Comment on attachment 8852768 [details]
Bug 1311802 - Part1 - Add SetMountState interface for VRMockDisplay;

https://reviewboard.mozilla.org/r/124924/#review127548
Attachment #8852768 - Flags: review?(amarchesini) → review+
(In reply to Daosheng Mu[:daoshengmu] from comment #4)
> Comment on attachment 8852769 [details]
> Bug 1311802 - Part2 - Add Mochitest implementation;
> 
> https://reviewboard.mozilla.org/r/124926/#review127522
> 
> Please fix the issues that I file and send me a r? again.
> 
> ::: dom/vr/test/mochitest.ini:11
> (Diff revision 1)
> >    WebVRHelpers.js
> >  
> >  [test_vrDisplay_getFrameData.html]
> >  [test_vrDisplay_exitPresent.html]
> >  [test_vrDisplay_requestPresent.html]
> > +[test_vrDisplay_onvrdisplaydeactivate_crosscontent.html]
> 
> Please follow the alphabetical order
> 
> ::: dom/vr/test/test_vrDisplay_onvrdisplaydeactivate_crosscontent.html:24
> (Diff revision 1)
> > +        var iframe1 = document.getElementById("iframe1").contentWindow;
> > +        var t = async_test("vrdisplaydeactivate crosscontent test");
> > +
> > +        window.addEventListener("vrdisplaydeactivate", () => {
> > +          t.step(() => {
> > +            assert_true(vrDisplay.isPresenting, "VRDisplay should be presenting now.");
> 
> Be presenting now? It is unmounted already why it is presenting?

Yes, because whether VR headset is mounted or not should not affect the presenting status,
I think my description is not precise enough, and I'll change it to make it more precise.
Comment on attachment 8852816 [details]
Bug 1311802 - Part3 - Initialize VRHMDSensorstate in VRMockDisplay to prevent crash;

https://reviewboard.mozilla.org/r/124982/#review127582
Attachment #8852816 - Flags: review?(dmu) → review+
Comment on attachment 8852769 [details]
Bug 1311802 - Part2 - Add Mochitest implementation;

https://reviewboard.mozilla.org/r/124926/#review127876

LGTM, thanks.
Attachment #8852769 - Flags: review?(dmu) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15d9c5895041
Part1 - Add SetMountState interface for VRMockDisplay; r=baku,daoshengmu
https://hg.mozilla.org/integration/autoland/rev/4bb94cbb9a24
Part2 - Add Mochitest implementation; r=daoshengmu
https://hg.mozilla.org/integration/autoland/rev/44bd57c9340f
Part3 - Initialize VRHMDSensorstate in VRMockDisplay to prevent crash; r=daoshengmu
sorry had to back this out for mochitest failure in test/test_vrDisplay_onvrdisplaydeactivate_crosscontent.html like https://treeherder.mozilla.org/logviewer.html#?job_id=87771236&repo=autoland&lineNumber=2602

https://hg.mozilla.org/integration/autoland/rev/7e3f9a865413
Flags: needinfo?(dmu)
:Lenzak, can you take a look at this?
Flags: needinfo?(dmu) → needinfo?(cleu)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/295df6974eba
Backed out changeset 7e3f9a865413 for failing browser/base/content/test/static/browser_parsable_css.js. r=backout
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/07452c042af0
Backed out the accidental backout of the backout of 3 changesets . r=backout/reland
I have tested it in my local machine for multiple times, I found that the time interval between we "unmount" the VRMockDisplay and we actually get the "vrdisplaydeactivate" event is quite unstable, sometime I can finish the test immediately while sometimes take more than 10 seconds to finish, I think it is why the test got time out on tryserver.

Hi Kip, do you have any thought about this one?
Flags: needinfo?(cleu) → needinfo?(kgilbert)
(In reply to Michael Leu[:Lenzak](UTC+8) from comment #18)
> I have tested it in my local machine for multiple times, I found that the
> time interval between we "unmount" the VRMockDisplay and we actually get the
> "vrdisplaydeactivate" event is quite unstable, sometime I can finish the
> test immediately while sometimes take more than 10 seconds to finish, I
> think it is why the test got time out on tryserver.
> 
> Hi Kip, do you have any thought about this one?

We internally update a structure (VRHMDInfo) that includes many flags, such as if the display is mounted, once per vsync interval.  Outside of VR presentation, this is driven by nsRefreshDriver at the 2d monitor refresh.

If there is any reason requestAnimationFrame could be throttled, this may affect the latency of updating the status of the display and firing events.
Flags: needinfo?(kgilbert)
It seems that the Compositor VSync is stuck for some reason.

We maintain a struct "VRDisplayInfo" storing needed information (isMounted, isPresenting...etc), and poll it manually in VRManager::NotifyVsync via compositor's vsync event to detect whether there are changes or not.

I found that NotifyVsync will stop for a random duration after we are in VR presenting mode, sometime it stops so long that the test cannot finish in limited time, causing a time out failure.

I'm not sure why this happen.

Hi Kip, do you have any idea?
Flags: needinfo?(kgilbert)
(In reply to Michael Leu[:Lenzak](UTC+8) from comment #20)
> It seems that the Compositor VSync is stuck for some reason.
> 
> We maintain a struct "VRDisplayInfo" storing needed information (isMounted,
> isPresenting...etc), and poll it manually in VRManager::NotifyVsync via
> compositor's vsync event to detect whether there are changes or not.
> 
> I found that NotifyVsync will stop for a random duration after we are in VR
> presenting mode, sometime it stops so long that the test cannot finish in
> limited time, causing a time out failure.
> 
> I'm not sure why this happen.
> 
> Hi Kip, do you have any idea?

There is a "watchdog"-like function that will ensure that the VRDisplayInfo details get replicated to the content process in the event that the VR render loop has not been iterating.  The watchdog is triggered by the non-vr vsync.  This could explain some of the variability.  This delay should not be perceptible, especially when under test as there is no physical hardware polling.

I am planning to change much of this mechanism as I replace many of our IPC messages with a shared memory based approach in Bug 1346927.
Flags: needinfo?(kgilbert)
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e007a9be3fa6
Part1 - Add SetMountState interface for VRMockDisplay; r=baku,daoshengmu
https://hg.mozilla.org/integration/autoland/rev/8802bed9ee83
Part2 - Add Mochitest implementation; r=daoshengmu
https://hg.mozilla.org/integration/autoland/rev/55e8ffef8fb6
Part3 - Initialize VRHMDSensorstate in VRMockDisplay to prevent crash; r=daoshengmu
I think we can land this test first, and let it be skipped. After doing some investigation and fix the problem of vsync, we can enable it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: