These new classes' destructors introduced a few debug assertion failures I've been hitting while running tests against a debug build of WebKit. This assertion can fail in the UI process when a web view deallocates after having process swapped: ASSERTION FAILED: m_messageReceivers.contains(std::make_pair(messageReceiverName, destinationID)) /Users/david/Source/OpenSource/Source/WebKit/Platform/IPC/MessageReceiverMap.cpp(72) : void IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference, uint64_t) 1 0x107a6ab69 WTFCrash 2 0x10c398d9b WTFCrashWithInfo(int, char const*, char const*, int) 3 0x10c46fafa IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference, unsigned long long) 4 0x10cc380d6 WebKit::AuxiliaryProcessProxy::removeMessageReceiver(IPC::StringReference, unsigned long long) 5 0x10cc5d910 void WebKit::AuxiliaryProcessProxy::removeMessageReceiver<WebCore::PageIdentifierType>(IPC::StringReference, WTF::ObjectIdentifier<WebCore::PageIdentifierType>) 6 0x10d66f408 WebKit::WebDeviceOrientationUpdateProviderProxy::~WebDeviceOrientationUpdateProviderProxy() ... 9 0x10cdb2355 WebKit::WebPageProxy::~WebPageProxy() ... 16 0x10cb87835 -[WKWebView .cxx_destruct] And after fixing that, I encountered this failure in the Web process: ASSERTION FAILED: m_globalMessageReceivers.contains(messageReceiverName) /Users/david/Source/OpenSource/Source/WebKit/Platform/IPC/MessageReceiverMap.cpp(62) : void IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference) 1 0x1e9924b69 WTFCrash 2 0x1d80ab4bb WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1d8181ece IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference) 4 0x1d80dcfde WebKit::AuxiliaryProcess::removeMessageReceiver(IPC::StringReference) ... 7 0x1d937cbbc WebKit::WebDeviceOrientationUpdateProvider::~WebDeviceOrientationUpdateProvider() ... 15 0x1efcde4fc WebCore::DeviceOrientationClientIOS::~DeviceOrientationClientIOS() ... 20 0x1f0892ed4 WebCore::Document::~Document()
<rdar://problem/57724251>
Created attachment 385084 [details] Patch
Created attachment 385085 [details] Patch v2 Tried to fix the GTK build.
Comment on attachment 385085 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=385085&action=review Thanks for fixing this! R=me. > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.cpp:49 > + WebProcess::singleton().removeMessageReceiver(Messages::WebDeviceOrientationUpdateProvider::messageReceiverName(), m_pageIdentifier); The WebPage is already being stored, could you call m_page.identifier() instead? > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.h:57 > + WebCore::PageIdentifier m_pageIdentifier; Is this member needed, if you get the identifier from m_page?
(In reply to Per Arne Vollan from comment #4) > Comment on attachment 385085 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385085&action=review > > Thanks for fixing this! R=me. > > > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.cpp:49 > > + WebProcess::singleton().removeMessageReceiver(Messages::WebDeviceOrientationUpdateProvider::messageReceiverName(), m_pageIdentifier); > > The WebPage is already being stored, could you call m_page.identifier() > instead? > > > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.h:57 > > + WebCore::PageIdentifier m_pageIdentifier; > > Is this member needed, if you get the identifier from m_page? I originally added ASSERT(m_page) and used m_page->identifier() in the destructor, but m_page ended up being null at this point in some tests. It seemed reasonable to save a copy of the page ID, given that the reference to the page is weak and WebDeviceOrientationUpdateProvider already has a few null-checks for m_page.
Comment on attachment 385085 [details] Patch v2 Clearing flags on attachment: 385085 Committed r253256: <https://trac.webkit.org/changeset/253256>
All reviewed patches have been landed. Closing bug.
(In reply to David Quesada from comment #5) > (In reply to Per Arne Vollan from comment #4) > > Comment on attachment 385085 [details] > > Patch v2 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=385085&action=review > > > > Thanks for fixing this! R=me. > > > > > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.cpp:49 > > > + WebProcess::singleton().removeMessageReceiver(Messages::WebDeviceOrientationUpdateProvider::messageReceiverName(), m_pageIdentifier); > > > > The WebPage is already being stored, could you call m_page.identifier() > > instead? > > > > > Source/WebKit/WebProcess/WebCoreSupport/WebDeviceOrientationUpdateProvider.h:57 > > > + WebCore::PageIdentifier m_pageIdentifier; > > > > Is this member needed, if you get the identifier from m_page? > > I originally added ASSERT(m_page) and used m_page->identifier() in the > destructor, but m_page ended up being null at this point in some tests. It > seemed reasonable to save a copy of the page ID, given that the reference to > the page is weak and WebDeviceOrientationUpdateProvider already has a few > null-checks for m_page. Makes sense, great work on fixing this!