Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UWP app panics when shutting down with active webgl #23778

Closed
jdm opened this issue Jul 15, 2019 · 5 comments
Closed

UWP app panics when shutting down with active webgl #23778

jdm opened this issue Jul 15, 2019 · 5 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jul 15, 2019

It's an assertion in the webrender GL device's Drop impl that "renderer::deinit was not called":

 	simpleservo.dll!std::panicking::rust_panic() Line 523	Unknown
 	simpleservo.dll!std::panicking::rust_panic_with_hook() Line 497	Unknown
 	simpleservo.dll!std::panicking::begin_panic<str*>(str* msg, (str*, u32, u32) * file_line_col) Line 411	Unknown
 	simpleservo.dll!webrender::device::gl::{{impl}}::drop(webrender::device::gl::Program * self) Line 616	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<webrender::device::gl::Program>(webrender::device::gl::Program *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::option::Option<webrender::device::gl::Program>>(core::option::Option<webrender::device::gl::Program> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<webrender::shade::LazilyCompiledShader>(webrender::shade::LazilyCompiledShader *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<webrender::shade::Shaders>(webrender::shade::Shaders *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::cell::UnsafeCell<webrender::shade::Shaders>>(core::cell::UnsafeCell<webrender::shade::Shaders> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::cell::RefCell<webrender::shade::Shaders>>(core::cell::RefCell<webrender::shade::Shaders> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<alloc::rc::RcBox<core::cell::RefCell<webrender::shade::Shaders>>>(alloc::rc::RcBox<core::cell::RefCell<webrender::shade::Shaders>> *) Line 197	Unknown
 	simpleservo.dll!alloc::rc::{{impl}}::drop<core::cell::RefCell<webrender::shade::Shaders>>(alloc::rc::Rc<core::cell::RefCell<webrender::shade::Shaders>> * self) Line 881	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<alloc::rc::Rc<core::cell::RefCell<webrender::shade::Shaders>>>(alloc::rc::Rc<core::cell::RefCell<webrender::shade::Shaders>> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<webrender::renderer::Renderer>(webrender::renderer::Renderer *) Line 197	Unknown
>	simpleservo.dll!core::ptr::real_drop_in_place<compositing::compositor::IOCompositor<simpleservo::ServoWindowCallbacks>>(compositing::compositor::IOCompositor<simpleservo::ServoWindowCallbacks> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<servo::Servo<simpleservo::ServoWindowCallbacks>>(servo::Servo<simpleservo::ServoWindowCallbacks> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<simpleservo::ServoGlue>(simpleservo::ServoGlue *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::option::Option<simpleservo::ServoGlue>>(core::option::Option<simpleservo::ServoGlue> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::cell::UnsafeCell<core::option::Option<simpleservo::ServoGlue>>>(core::cell::UnsafeCell<core::option::Option<simpleservo::ServoGlue>> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>>(core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>> *) Line 197	Unknown
 	simpleservo.dll!core::ptr::real_drop_in_place<core::option::Option<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>>>(core::option::Option<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>> *) Line 197	Unknown
 	simpleservo.dll!core::mem::drop<core::option::Option<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>>>(core::option::Option<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>> _x) Line 686	Unknown
 	simpleservo.dll!std::thread::local::fast::destroy_value<core::cell::RefCell<core::option::Option<simpleservo::ServoGlue>>>(unsigned char * ptr) Line 464	Unknown
 	simpleservo.dll!std::sys_common::thread_local::register_dtor_fallback::run_dtors() Line 257	Unknown
 	simpleservo.dll!std::sys::windows::thread_local::on_tls_callback() Line 205	Unknown
 	ntdll.dll!00007ffbefd85031()	Unknown
 	ntdll.dll!00007ffbefd85113()	Unknown
 	ntdll.dll!00007ffbefd82b76()	Unknown
 	ntdll.dll!00007ffbefdccede()	Unknown
 	kernel32.dll!00007ffbee5c7bdc()	Unknown
 	ntdll.dll!00007ffbefdcce71()	Unknown
``
@jdm
Copy link
Member Author

@jdm jdm commented Jul 15, 2019

This is in a build with #23777 applied.

@jdm
Copy link
Member Author

@jdm jdm commented Jul 17, 2019

Turns out that Servo::~Servo is never invoked, so the embedding deinitialization never occurs.

@jdm
Copy link
Member Author

@jdm jdm commented Jul 17, 2019

There are a few reasons I see that this occurs:

  • mServo in OpenGLESPage is a raw pointer that is never deleted so its destructor is never invoked
  • even if I use std::unique_pointer instead for mServo, the destructor still never runs because of the UWP app lifecycle model
  • if we want to execute code when the app is terminating, we need to support either the OnSuspend handler or the EnterBackground handler
  • supporting these implies supporting the full lifecycle by also supporting resuming a suspend instance of the browser and recreating mServo
@jdm
Copy link
Member Author

@jdm jdm commented Jul 17, 2019

The following patch fixes the webrender panic:

commit 8a4e21df929cd078b2a031a638d4ed51f239e75b
Author: Josh Matthews <josh@joshmatthews.net>
Date:   Wed Jul 17 10:40:20 2019 -0400

    Deinitialize Servo on app suspend.

diff --git a/support/hololens/App.xaml.cpp b/support/hololens/App.xaml.cpp
index e084de34b4..938742d000 100644
--- a/support/hololens/App.xaml.cpp
+++ b/support/hololens/App.xaml.cpp
@@ -6,10 +6,17 @@
 #include "App.xaml.h"
 
 using namespace hlservo;
+using namespace Windows::ApplicationModel;
+using namespace Windows::ApplicationModel::Activation;
+using namespace Windows::Foundation;
+using namespace Windows::Storage;
+using namespace Windows::UI::Xaml;
 
 App::App()
 {
     InitializeComponent();
+    Suspending += ref new SuspendingEventHandler(this, &App::OnSuspending);
+    Resuming += ref new EventHandler<Object^>(this, &App::OnResuming);
 }
 
 void App::OnLaunched(Windows::ApplicationModel::Activation::LaunchActivatedEventArgs ^ e)
@@ -27,3 +34,31 @@ void App::OnLaunched(Windows::ApplicationModel::Activation::LaunchActivatedEvent
     Windows::UI::Xaml::Window::Current->Content = mPage;
     Windows::UI::Xaml::Window::Current->Activate();
 }
+
+/// <summary>
+/// Invoked when application execution is being suspended.  Application state is saved
+/// without knowing whether the application will be terminated or resumed with the contents
+/// of memory still intact.
+/// </summary>
+/// <param name="sender">The source of the suspend request.</param>
+/// <param name="e">Details about the suspend request.</param>
+void App::OnSuspending(Object^ sender, SuspendingEventArgs^ e)
+{
+	(void)sender;	// Unused parameter
+	(void)e;	// Unused parameter
+
+	mPage->SaveInternalState(ApplicationData::Current->LocalSettings->Values);
+}
+
+/// <summary>
+/// Invoked when application execution is being resumed.
+/// </summary>
+/// <param name="sender">The source of the resume request.</param>
+/// <param name="args">Details about the resume request.</param>
+void App::OnResuming(Object ^sender, Object ^args)
+{
+	(void)sender; // Unused parameter
+	(void)args; // Unused parameter
+
+	mPage->LoadInternalState(ApplicationData::Current->LocalSettings->Values);
+}
\ No newline at end of file
diff --git a/support/hololens/App.xaml.h b/support/hololens/App.xaml.h
index 4c109377d4..e305469220 100644
--- a/support/hololens/App.xaml.h
+++ b/support/hololens/App.xaml.h
@@ -13,6 +13,9 @@ ref class App sealed {
 public:
     App();
     virtual void OnLaunched(Windows::ApplicationModel::Activation::LaunchActivatedEventArgs ^ e) override;
+private:
+	void OnSuspending(Platform::Object^ sender, Windows::ApplicationModel::SuspendingEventArgs^ e);
+	void OnResuming(Platform::Object ^sender, Platform::Object ^args);
 
 private:
     OpenGLESPage ^ mPage;
diff --git a/support/hololens/OpenGLESPage.xaml.cpp b/support/hololens/OpenGLESPage.xaml.cpp
index c4c7dd21e3..fc7f87c647 100644
--- a/support/hololens/OpenGLESPage.xaml.cpp
+++ b/support/hololens/OpenGLESPage.xaml.cpp
@@ -2,6 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+#include <atomic>
 #include "pch.h"
 #include "OpenGLESPage.xaml.h"
 #include "Servo.h"
@@ -10,6 +11,7 @@ using namespace hlservo;
 using namespace Platform;
 using namespace Concurrency;
 using namespace Windows::Foundation;
+using namespace Windows::Foundation::Collections;
 
 static char sWakeupEvent[] = "SIGNAL_WAKEUP";
 
@@ -118,9 +120,9 @@ void OpenGLESPage::StartRenderLoop()
       EGLint panelHeight = 0;
       mOpenGLES->GetSurfaceDimensions(mRenderSurface, &panelWidth, &panelHeight);
       glViewport(0, 0, panelWidth, panelHeight);
-      mServo = new Servo(panelWidth, panelHeight);
+      mServo.reset(new Servo(panelWidth, panelHeight));
 
-      while (action->Status == Windows::Foundation::AsyncStatus::Started) {
+      while (action->Status == Windows::Foundation::AsyncStatus::Started && !mTerminate.load()) {
         // Block until Servo::sWakeUp is called.
         // Or run full speed if animating (see on_animating_changed),
         // it will endup blocking on SwapBuffers to limit rendering to 60FPS
@@ -129,6 +131,8 @@ void OpenGLESPage::StartRenderLoop()
         }
         mServo->PerformUpdates();
       }
+
+      mServo.reset(nullptr);
     };
 
     auto workItemHandler = ref new Windows::System::Threading::WorkItemHandler(loop);
@@ -147,8 +151,29 @@ void OpenGLESPage::StartRenderLoop()
 
 void OpenGLESPage::StopRenderLoop()
 {
+    mTerminate.store(true);
+    Servo::sWakeUp();
     if (mRenderLoopWorker) {
         mRenderLoopWorker->Cancel();
         mRenderLoopWorker = nullptr;
     }
 }
+
+// Saves the current state of the app for suspend and terminate events.
+void OpenGLESPage::SaveInternalState(IPropertySet^ state)
+{
+	// Stop rendering when the app is suspended.
+	StopRenderLoop();
+
+	// Put code to save app state here.
+}
+
+// Loads the current state of the app for resume events.
+void OpenGLESPage::LoadInternalState(IPropertySet^ state)
+{
+	// Put code to load app state here.
+
+	// Start rendering when the app is resumed.
+	StartRenderLoop();
+}
+
diff --git a/support/hololens/OpenGLESPage.xaml.h b/support/hololens/OpenGLESPage.xaml.h
index ab2c8c2fd8..1749304d65 100644
--- a/support/hololens/OpenGLESPage.xaml.h
+++ b/support/hololens/OpenGLESPage.xaml.h
@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include <atomic>
 #include "OpenGLES.h"
 #include "OpenGLESPage.g.h"
 #include "Servo.h"
@@ -17,6 +18,9 @@ public:
 
     internal : OpenGLESPage(OpenGLES* openGLES);
 
+	void SaveInternalState(Windows::Foundation::Collections::IPropertySet^ state);
+	void LoadInternalState(Windows::Foundation::Collections::IPropertySet^ state);
+
 private:
     void OnPageLoaded(Platform::Object ^ sender, Windows::UI::Xaml::RoutedEventArgs ^ e);
     void OnVisibilityChanged(Windows::UI::Core::CoreWindow ^ sender,
@@ -32,6 +36,7 @@ private:
     EGLSurface mRenderSurface;
     Concurrency::critical_section mRenderSurfaceCriticalSection;
     Windows::Foundation::IAsyncAction ^ mRenderLoopWorker;
-    Servo* mServo;
+    std::unique_ptr<Servo> mServo;
+    std::atomic<bool> mTerminate;
 };
 }

I'll hold off submitting it while #23790 is open, since it will require porting these changes to the new model.

@jdm jdm added the C-has-patch label Jul 17, 2019
@jdm jdm added this to To do in UWP port via automation Jul 17, 2019
@jdm jdm added this to To do in Windows ARM64 Jul 22, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Jul 29, 2019

Fixed by #23866.

@jdm jdm closed this Jul 29, 2019
UWP port automation moved this from To do to Done Jul 29, 2019
Windows ARM64 automation moved this from To do to Done Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
UWP port
  
Done
Windows ARM64
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.