Bug 204977

Summary: REGRESSION(r253231): Debug assertion failures under ~WebDeviceOrientationUpdateProvider(Proxy)
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, pvollan, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch v2 none

Description David Quesada 2019-12-06 22:51:47 PST
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()
Comment 1 Radar WebKit Bug Importer 2019-12-06 22:54:08 PST
<rdar://problem/57724251>
Comment 2 David Quesada 2019-12-06 23:27:46 PST
Created attachment 385084 [details]
Patch
Comment 3 David Quesada 2019-12-06 23:40:59 PST
Created attachment 385085 [details]
Patch v2

Tried to fix the GTK build.
Comment 4 Per Arne Vollan 2019-12-07 08:13:53 PST
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?
Comment 5 David Quesada 2019-12-07 10:23:02 PST
(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 6 WebKit Commit Bot 2019-12-07 10:46:36 PST
Comment on attachment 385085 [details]
Patch v2

Clearing flags on attachment: 385085

Committed r253256: <https://trac.webkit.org/changeset/253256>
Comment 7 WebKit Commit Bot 2019-12-07 10:46:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Per Arne Vollan 2019-12-07 10:51:04 PST
(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!