Bug 145488

Summary:

Web Inspector: Crash closing a related tab with Web Inspector open while page is refreshing

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	none
[PATCH] Proposed Fix	ap: review+

Description Joseph Pecoraro 2015-05-29 20:17:53 PDT

* SUMMARY
Crash closing a related tab with Web Inspector open while page is refreshing.

* STEPS TO REPRODUCE
1. Inspect <https://www.tumblr.com>
2. Expand <head> in DOM Tree
3. Click URL from <link rel="icon" sizes="any" mask="" href="https://secure.assets.tumblr.com/images/tumblr_t_logo.svg?_v=fe2b348bb13d366e9e93d5bf6a8b2fdd"> to open new page in the same WebProcess
4. Refresh the tumblr.com page with the Web Inspector
5. Close Tab (⌘W) while the page is refreshing
  => occasionally a CRASH

* CRASH
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       EXC_I386_GPFLT

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit             std::__1::__function::__func<IPC::Connection::connectionDidClose()::$_8, std::__1::allocator<IPC::Connection::connectionDidClose()::$_8>, void ()>::operator()() + 28
1   com.apple.JavaScriptCore     WTF::RunLoop::performWork() + 850 (RunLoop.cpp:106)
2   com.apple.JavaScriptCore     WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
3   com.apple.CoreFoundation     __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
4   com.apple.CoreFoundation     __CFRunLoopDoSources0 + 269
5   com.apple.CoreFoundation     __CFRunLoopRun + 927
6   com.apple.CoreFoundation     CFRunLoopRunSpecific + 296
7   com.apple.HIToolbox          RunCurrentEventLoopInMode + 235
8   com.apple.HIToolbox          ReceiveNextEventCommon + 431
9   com.apple.HIToolbox          _BlockUntilNextEventMatchingListInModeWithFilter + 71
10  com.apple.AppKit             _DPSNextEvent + 978
11  com.apple.AppKit             -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
12  com.apple.AppKit             -[NSApplication run] + 594
13  com.apple.AppKit             NSApplicationMain + 1832
14  libxpc.dylib                 _xpc_objc_main + 793
15  libxpc.dylib                 xpc_main + 490
16  com.apple.WebKit.WebContent.Development main + 21 (XPCServiceMain.Development.mm:90)
17  libdyld.dylib                start + 1

* LLDB (Release)
- Looks like the client is a garbage object, it never invalidated the connection properly.

(lldb) Process 33413 stopped
* thread #1: tid = 0x20b823, 0x0000000109ea6c8e WebKit`std::__1::__function::__func<IPC::Connection::connectionDidClose()::$_8, std::__1::allocator<IPC::Connection::connectionDidClose()::$_8>, void ()>::operator()() [inlined] IPC::Connection::connectionDidClose()::$_8::operator()() const + 24 at Connection.cpp:781, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x0000000109ea6c8e WebKit`std::__1::__function::__func<IPC::Connection::connectionDidClose()::$_8, std::__1::allocator<IPC::Connection::connectionDidClose()::$_8>, void ()>::operator()() [inlined] IPC::Connection::connectionDidClose()::$_8::operator()() const + 24 at Connection.cpp:781
   778 	        Client* client = connection->m_client;
   779 	        connection->m_client = nullptr;
   780 	
-> 781 	        client->didClose(*connection);
   782 	    });
   783 	}
   784 	

(lldb) p client
(IPC::Connection::Client *) $0 = 0x00007fcfd0529118

(lldb) p client
(IPC::Connection::Client) $11 = {}

Comment 1 Joseph Pecoraro 2015-05-29 20:18:23 PDT

<rdar://problem/21142346>

Comment 2 Joseph Pecoraro 2015-05-29 20:21:16 PDT

FWIW, the case where we would close without invalidating is under WebPage::close():

* thread #1: tid = 0x246321, 0x00000001093baa81 WebKit`WebKit::WebInspector::~WebInspector(this=0x00007ffce6442a58) + 71 at WebInspector.cpp:78, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00000001093baa81 WebKit`WebKit::WebInspector::~WebInspector(this=0x00007ffce6442a58) + 71 at WebInspector.cpp:78
    frame #1: 0x000000010949e506 WebKit`-[WKObject dealloc](self=0x00007ffce6442a40, _cmd=<unavailable>) + 25 at WKObject.mm:40
    frame #2: 0x00007fff99d2e89c libobjc.A.dylib`objc_object::sidetable_release(bool) + 236
    frame #3: 0x00007fff8c942db0 CoreFoundation`CFRelease + 304
    frame #4: 0x00000001093d9c7f WebKit`WebKit::WebPage::close() [inlined] void WTF::derefIfNotNull<WebKit::WebInspector>(ptr=<unavailable>) + 231 at PassRefPtr.h:42
    frame #5: 0x00000001093d9c75 WebKit`WebKit::WebPage::close() [inlined] WTF::RefPtr<WebKit::WebInspector>::~RefPtr() at RefPtr.h:59
    frame #6: 0x00000001093d9c75 WebKit`WebKit::WebPage::close() [inlined] WTF::RefPtr<WebKit::WebInspector>::~RefPtr() at RefPtr.h:59
    frame #7: 0x00000001093d9c75 WebKit`WebKit::WebPage::close() [inlined] WTF::RefPtr<WebKit::WebInspector>::operator=(WebKit::WebInspector*) + 18 at RefPtr.h:142
    frame #8: 0x00000001093d9c63 WebKit`WebKit::WebPage::close(this=0x00007ffce6809410) + 203 at WebPage.cpp:925
    frame #9: 0x00000001093f4cad WebKit`WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::MessageDecoder&) [inlined] void IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(), std::__1::tuple<> >(object=0x00007ffce6809410)(), std::__1::tuple<>&&, std::index_sequence<>) + 8 at HandleMessage.h:16
    frame #10: 0x00000001093f4ca5 WebKit`WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::MessageDecoder&) [inlined] void IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(), std::__1::tuple<>, std::make_index_sequence<0ul> >(std::__1::tuple<>&&, WebKit::WebPage*, void (WebKit::WebPage::*)()) at HandleMessage.h:22
    frame #11: 0x00000001093f4ca5 WebKit`WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::MessageDecoder&) [inlined] void IPC::handleMessage<Messages::WebPage::Close, WebKit::WebPage, void (WebKit::WebPage::*)()>(decoder=<unavailable>)()) at HandleMessage.h:92
    frame #12: 0x00000001093f4ca5 WebKit`WebKit::WebPage::didReceiveWebPageMessage(this=0x00007ffce6809410, connection=<unavailable>, decoder=<unavailable>) + 5585 at WebPageMessageReceiver.cpp:655

This does:

    if (m_inspector) {
        m_inspector->disconnectFromPage();
        m_inspector = nullptr;
    }

And disconnectFromPage just does close() without going further to invalidate.

We could make disconnectFromPage invalidate, but I figure putting it in the destructor we are guaranteed it will happen no matter how we destruct.

Comment 3 Joseph Pecoraro 2015-05-29 20:24:24 PDT

Created attachment 253948 [details]
[PATCH] Proposed Fix

Comment 4 Joseph Pecoraro 2015-05-29 20:25:47 PDT

Created attachment 253949 [details]
[PATCH] Proposed Fix

Comment 5 Joseph Pecoraro 2015-05-29 20:56:02 PDT

Committed r185030: <http://trac.webkit.org/changeset/185030>