Summary: | REGRESSION(r253231): Debug assertion failures under ~WebDeviceOrientationUpdateProvider(Proxy) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Quesada <david_quesada> | ||||||
Component: | WebKit2 | Assignee: | 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
David Quesada
2019-12-06 22:51:47 PST
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! |