New issue
Advanced search Search tips

Issue 723289 link

Starred by 2 users
Status: WontFix
Owner:
klausw@chromium.org
Closed: Apr 2018
Cc:
mthiesse@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug
Proj-XR



Sign in to add a comment

Potential deadlock in VRDisplay::submitFrame

Reported by artyo...@gmail.com, May 17 2017 
Steps to reproduce the problem:
None

What is the expected behavior?
VRDisplay::submitFrame waits on pending_previous_frame_render_ and pending_submit_frame_. Those are set to true and OnSubmitFrameTransferred / OnSubmitFrameComplete callbacks supposed to set those members to false and this would unlock the submitFrame:

    while (pending_submit_frame_) {
      if (!submit_frame_client_binding_.WaitForIncomingMethodCall()) {
        DLOG(ERROR) << "Failed to receive SubmitFrame response";
        break;
      }
    }

What went wrong?
However, since it waits in the loop, the C++ compiler may optimize and replace the pending_previous_frame_render_ and pending_submit_frame_ by registers, since it is not aware that those could be changed by someone else. I'd say these members require 'volatile' modifier.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: M60 master  Channel: n/a
OS Version: 
Flash Version:

Comment 1 by klausw@chromium.org, May 17 2017

Cc: mthiesse@chromium.org
Labels: Proj-VR
Status: Started (was: Unconfirmed)
I think it's correct as-is since this is not multithreaded code, and a compiler optimization which skips reading from the variable would be illegal. WaitForIncomingMethodCall is a re-entrant call within the same thread that processes received IPC data. As far as the compiler is concerned, it's basically a subroutine call where one of the functions called within the call stack modifies the member variable, then it returns to the toplevel. Any optimization which would assume that the member remains unmodified across this would also break a lot of other legal code.

@mthiesse for a second opinion, does this sound right? I do agree that this probably deserves a comment about the assumptions made.

Comment 2 by klausw@chromium.org, May 17 2017

Owner: klausw@chromium.org

Comment 3 by artyo...@gmail.com, May 18 2017 

You, probably, right. However, 'volatile' is a cheap insurance and improves readability.

Comment 4 by mthiesse@chromium.org, May 18 2017 

'volatile' certainly does not improve readability. Any reader would begin to wonder if there's some memory-mapped I/O happening and how this could possibly be happening inside of blink.

@klausw that sounds right. Adding a comment to make this clear sounds reasonable.

Comment 5 by cjgrant@chromium.org, Apr 17 2018

Status: WontFix (was: Started)
Bug scrub:  Klaus, this has been active for nearly a year, and if the action item was to add a comment, I think we should just close this off.  Please reopen if you disagree.

Comment 6 by btebbs@chromium.org, Jul 4

Components: Blink>WebXR
►

Sign in to add a comment