New issue
Advanced search Search tips

Issue 921739 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 17
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug
Proj-VR
Proj-XR
Proj-XR-VR



Sign in to add a comment

Rare ACCESS_VIOLATION_EXCEPTION when closing browser w/ VR notification displayed

Project Member Reported by bsheedy@chromium.org, Jan 14

Issue description

Rarely, WebXrVrBrowserTestStandard.TestINSessionPermissionNotificationCloseWhileVisible will crash with an ACCESS_VIOLATION_EXCEPTION when trying to use VRBrowserRendererThreadWin's ui_ handle in SetVisibleExternalPromptNotificationOnRenderThread. The pointer isn't null at that point, so it's likely that the object the pointer refers to is getting destroyed before the VRBrowserRendererThreadWin instance is.

Running the test repeatedly should be sufficient to reproduce the issue.
 
Owner: billorr@chromium.org
Status: Assigned (was: Available)
This seems to be caused by the posted task to dismiss the notification firing after Cleanup is called. This can be fixed by either removing the post tasks (since we should always be on the same thread) or using weak pointers instead of unretained raw pointers.

The quick fix is to add an early return if the browser_renderer_ pointer is null, but assigning to billorr@ to properly fix with one of the above solutions.
Several methods post tasks to VRBrowserRendererThreadWin to get onto the "UI thread".  There are a couple issues with this, the main being that the posted tasks capture this with base::unretained, but this isn't safe since the tasks could run after we are destroyed.

We should either not post tasks (since we are on the main UI thread already), use weak pointers, or cancel the posted tasks in some other way.
Apparently this is getting hit very frequently on Windows 10 debug builds https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win10%20Debug%20(NVIDIA), which has prompted a revert of the CL that added the test that repros this.
Project Member

Comment 4 by bugdroid1@chromium.org, Jan 16 (6 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96fa6c14aa7e7e8902d6cb703ea2502e0c13215c

commit 96fa6c14aa7e7e8902d6cb703ea2502e0c13215c
Author: Bill Orr <billorr@chromium.org>
Date: Wed Jan 16 18:27:27 2019

Fix a crash in VRBrowserRendererThreadWin

VRBrowserRendererThreadWin behaved as though it lived on a separate
thread, posting tasks to do work instead of doing the work
synchronously.  If it was destroyed, those posted tasks would still run,
potentially using VRBrowserRendererThreadWin or its members after
destruction.

The fix is to stop pretending it is a thread, avoiding the posted tasks.

BUG= 921739 

Change-Id: I147cccf9487386b95c25401d444294321fa925af
Reviewed-on: https://chromium-review.googlesource.com/c/1413413
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Bill Orr <billorr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623303}
[modify] https://crrev.com/96fa6c14aa7e7e8902d6cb703ea2502e0c13215c/chrome/browser/vr/ui_host/vr_ui_host_impl.cc
[modify] https://crrev.com/96fa6c14aa7e7e8902d6cb703ea2502e0c13215c/chrome/browser/vr/win/vr_browser_renderer_thread_win.cc
[modify] https://crrev.com/96fa6c14aa7e7e8902d6cb703ea2502e0c13215c/chrome/browser/vr/win/vr_browser_renderer_thread_win.h

Comment 5 by billorr@chromium.org, Jan 17 (5 days ago)

Status: Fixed (was: Assigned)

Sign in to add a comment