David has some suspicion regarding the reference counting. Looking at the example code: https://docs.microsoft.com/en-us/windows/win32/api/locationapi/nf-locationapi-ilocation-registerforreport ```pLocationEvents``` there is definitely kept alive until the ```UnregisterForReport``` is done. But it seems to me that it would have to be a bug for whoever uses ```ILocationEvents``` to drop the reference and then use the object later, so barring concurrency this doesn't seem like it can be the problem (but it could very well work around it, or prevent us from hitting an issue that anyone using the reference code would never hit). An easy fix for our use would then to be to move ```event``` to be a member ```mEvent``` on ```mLocation``` and be automatically ```Released()``` when ```mLocation``` is ```Released()```, though some fiddling might be needed if we ever use multiple callbacks - but I think we only ever have a single global ```nsGeolocationService``` that itself handles multiple callbacks, from looking at the code. Looking at another crash report: https://crash-stats.mozilla.org/report/index/084fde43-9dc5-4732-83f5-7a3580220601#allthreads ``` Main Thread 1 user32.dll unsigned long RealMsgWaitForMultipleObjectsEx(unsigned long, void* const*, unsigned long, unsigned long, unsigned long) None 2 combase.dll CCliModalLoop::BlockFn(void**, unsigned long, unsigned long*) onecore\com\combase\dcomrem\callctrl.cxx:2156 ... 13 xul.dll mozilla::dom::WindowsLocationProvider::SetHighAccuracy(bool) dom/system/windows/WindowsLocationProvider.cpp:270 Worker Thread 5 ntdll.dll RtlEnterCriticalSection None 6 LocationApi.dll ATL::CComCritSecLock<class ATL::CComAutoCriticalSection>::CComCritSecLock<class ATL::CComAutoCriticalSection>(class ATL::CComAutoCriticalSection&, bool) None 7 LocationApi.dll void CLegacyPositionRequest::GetLocationFromSession(bool) None 8 LocationApi.dll static void CLegacyPositionRequest::WaitForLocationInformationThreadProc(struct _TP_CALLBACK_INSTANCE*, void*, struct _TP_WAIT*, long) None 9 ntdll.dll TppExecuteWaitCallback None Another Worker Thread 5 ntdll.dll RtlEnterCriticalSection None 6 LocationApi.dll ATL::CComCritSecLock<class ATL::CComAutoCriticalSection>::CComCritSecLock<class ATL::CComAutoCriticalSection>(class ATL::CComAutoCriticalSection&, bool) None 7 LocationApi.dll void CLegacyPositionRequest::DispatchStatusChanged(enum LOCATION_REPORT_STATUS) None 8 LocationApi.dll long CLegacyPositionRequest::StartLocationSession None 9 LocationApi.dll long CLegacyPositionRequest::RestartLocationSession None 10 LocationApi.dll virtual long CLegacyPositionRequest::SetDesiredAccuracy(struct _GUID const&, enum LOCATION_DESIRED_ACCURACY) None 11 LocationApi.dll virtual long CLegacyLocationApiImpl::SetDesiredAccuracy(struct _GUID const&, enum LOCATION_DESIRED_ACCURACY) None 12 rpcrt4.dll Invoke None Ø Crashing Thread 0 locationapi.dll void CLegacyPositionRequest::GetLocationFromSession(bool) context 1 locationapi.dll static void CLegacyPositionRequest::WaitForLocationInformationThreadProc(struct _TP_CALLBACK_INSTANCE*, void*, struct _TP_WAIT*, long) cfi 2 ntdll.dll TppExecuteWaitCallback cfi ``` Notice how there's 3 LocationAPI.dll calls running on different threads. They're blocked on a COM Lock (waiting for "LocationSession" whatever that is?), but this at least explains how it would be possible for a worker thread to race with the ```Unregister``` call even if the latter blocks the main thread. Unfortunately, if that's the problem, then the ILocationEvents really needs to be a global object with the application lifetime, and itself have threadsafe support for adding the callbacks (and setting mProvider...because if the above happens it could outlive that as well).
Bug 1704500 Comment 14 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
David has some suspicion regarding the reference counting. Looking at the example code: https://docs.microsoft.com/en-us/windows/win32/api/locationapi/nf-locationapi-ilocation-registerforreport ```pLocationEvents``` there is definitely kept alive until the ```UnregisterForReport``` is done. But it seems to me that it would have to be a bug for whoever uses ```ILocationEvents``` to drop the reference and then use the object later, so barring concurrency this doesn't seem like it can be the problem (but it could very well work around it, or prevent us from hitting an issue that anyone using the reference code would never hit). ~~An easy fix for our use would then to be to move ```event``` to be a member ```mEvent``` on ```mLocation``` and be automatically ```Released()``` when ```mLocation``` is ```Released()```~~ (see below), though some fiddling might be needed if we ever use multiple callbacks - but I think we only ever have a single global ```nsGeolocationService``` that itself handles multiple callbacks, from looking at the code. Looking at another crash report: https://crash-stats.mozilla.org/report/index/084fde43-9dc5-4732-83f5-7a3580220601#allthreads ``` Main Thread 1 user32.dll unsigned long RealMsgWaitForMultipleObjectsEx(unsigned long, void* const*, unsigned long, unsigned long, unsigned long) None 2 combase.dll CCliModalLoop::BlockFn(void**, unsigned long, unsigned long*) onecore\com\combase\dcomrem\callctrl.cxx:2156 ... 13 xul.dll mozilla::dom::WindowsLocationProvider::SetHighAccuracy(bool) dom/system/windows/WindowsLocationProvider.cpp:270 Worker Thread 5 ntdll.dll RtlEnterCriticalSection None 6 LocationApi.dll ATL::CComCritSecLock<class ATL::CComAutoCriticalSection>::CComCritSecLock<class ATL::CComAutoCriticalSection>(class ATL::CComAutoCriticalSection&, bool) None 7 LocationApi.dll void CLegacyPositionRequest::GetLocationFromSession(bool) None 8 LocationApi.dll static void CLegacyPositionRequest::WaitForLocationInformationThreadProc(struct _TP_CALLBACK_INSTANCE*, void*, struct _TP_WAIT*, long) None 9 ntdll.dll TppExecuteWaitCallback None Another Worker Thread 5 ntdll.dll RtlEnterCriticalSection None 6 LocationApi.dll ATL::CComCritSecLock<class ATL::CComAutoCriticalSection>::CComCritSecLock<class ATL::CComAutoCriticalSection>(class ATL::CComAutoCriticalSection&, bool) None 7 LocationApi.dll void CLegacyPositionRequest::DispatchStatusChanged(enum LOCATION_REPORT_STATUS) None 8 LocationApi.dll long CLegacyPositionRequest::StartLocationSession None 9 LocationApi.dll long CLegacyPositionRequest::RestartLocationSession None 10 LocationApi.dll virtual long CLegacyPositionRequest::SetDesiredAccuracy(struct _GUID const&, enum LOCATION_DESIRED_ACCURACY) None 11 LocationApi.dll virtual long CLegacyLocationApiImpl::SetDesiredAccuracy(struct _GUID const&, enum LOCATION_DESIRED_ACCURACY) None 12 rpcrt4.dll Invoke None Ø Crashing Thread 0 locationapi.dll void CLegacyPositionRequest::GetLocationFromSession(bool) context 1 locationapi.dll static void CLegacyPositionRequest::WaitForLocationInformationThreadProc(struct _TP_CALLBACK_INSTANCE*, void*, struct _TP_WAIT*, long) cfi 2 ntdll.dll TppExecuteWaitCallback cfi ``` Notice how there's 3 LocationAPI.dll calls running on different threads. They're blocked on a COM Lock (waiting for "LocationSession" whatever that is?), but this at least explains how it would be possible for a worker thread to race with the ```Unregister``` call even if the latter blocks the main thread. Unfortunately, if that's the problem, then the ```ILocationEvents``` really needs to be a global object with the application lifetime, and itself have threadsafe support for adding the callbacks (and setting ```mProvider```...because if the above happens it could outlive that as well).