Bug 204977 – REGRESSION(r253231): Debug assertion failures under ~WebDeviceOrientationUpdateProvider(Proxy)

• Component: WebKit2
• Status: RESOLVED FIXED

[David Quesada]

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()

[Radar WebKit Bug Importer]

<rdar://problem/57724251>

[David Quesada]

Created attachment 385084 [details]
Patch

[David Quesada]

Created attachment 385085 [details]
Patch v2

Tried to fix the GTK build.

[Per Arne Vollan]

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?

[David Quesada]

(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.

[WebKit Commit Bot]

Comment on attachment 385085 [details]
Patch v2

Clearing flags on attachment: 385085

Committed r253256: <https://trac.webkit.org/changeset/253256>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Per Arne Vollan]

(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!