Bug 199808 - Crash under DisplayRefreshMonitorManager::displayWasUpdated()
Summary: Crash under DisplayRefreshMonitorManager::displayWasUpdated()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 199847
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-15 14:22 PDT by Chris Dumez
Modified: 2019-07-16 19:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.99 KB, patch)
2019-07-15 14:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-07-15 14:22:24 PDT
50 WebCore: WebCore::DisplayRefreshMonitorManager::displayWasUpdated(unsigned int) <==
        50 WebKit: IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
          50 WebKit: WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14, void>::call()
            50 JavaScriptCore: WTF::RunLoop::performWork()
              50 JavaScriptCore: WTF::RunLoop::performWork(void*)
                50 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
                  50 CoreFoundation: __CFRunLoopDoSource0
                    50 CoreFoundation: __CFRunLoopDoSources0
                      50 CoreFoundation: __CFRunLoopRun
                        50 CoreFoundation: CFRunLoopRunSpecific
                          50 Foundation: -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
                            50 Foundation: -[NSRunLoop(NSRunLoop) run]
                              50 libxpc.dylib: _xpc_objc_main.cold.4
                                50 libxpc.dylib: _xpc_objc_main
                                  50 libxpc.dylib: xpc_main
                                    50 WebKit: WebKit::XPCServiceMain(int, char const**)
                                      50 libdyld.dylib: start
Comment 1 Chris Dumez 2019-07-15 14:22:38 PDT
<rdar://problem/53070144>
Comment 2 Chris Dumez 2019-07-15 14:31:24 PDT
Created attachment 374151 [details]
Patch
Comment 3 Geoffrey Garen 2019-07-15 14:41:27 PDT
Comment on attachment 374151 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2019-07-15 16:07:03 PDT
Comment on attachment 374151 [details]
Patch

Clearing flags on attachment: 374151

Committed r247459: <https://trac.webkit.org/changeset/247459>
Comment 5 WebKit Commit Bot 2019-07-15 16:07:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Said Abou-Hallawa 2019-07-16 18:27:26 PDT
Comment on attachment 374151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374151&action=review

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:134
> -    for (const auto& monitorWrapper : m_monitors) {
> -        auto& monitor = monitorWrapper.monitor;
> +    Vector<RefPtr<DisplayRefreshMonitor>> monitors = WTF::map(m_monitors, [](auto& monitorWrapper) {
> +        return monitorWrapper.monitor;
> +    });
> +    for (auto& monitor : monitors) {

Isn't PlatformDisplayID unique for every monitor? If so, why do we need to loop through all the monitors if we find one whose displayID matches the argument displayID? Can't we just break/return when we find the first one:

if (displayID == monitor->displayID()) {
    if (monitor->hasRequestedRefreshCallback())
        monitor->displayLinkFired();
    break;
}

DisplayRefreshMonitorManager::unregisterClient() has similar logic like the proposed one so it does not need to copy the monitors before looping through them.