Bug 140968 – Web Inspector: Crash when closing inspected page

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* SUMMARY
WebContentProcess crash when closing inspected page.

* STEPS TO REPRODUCE
1. Login to iCloud.com
2. Open Pages
3. Create a new file in that app (causes a new tab to be made)
4. Open inspector in new tab
5. Close new tab
  => CRASH

* CRASH:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000070

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010ec6828b WebCore::Page::inspectorController() const + 43
1   com.apple.WebKit              	0x000000010ec66cc1 WebKit::WebInspector::close() + 33
2   com.apple.WebKit              	0x000000010ec6a339 WebKit::WebInspector::didClose(IPC::Connection&) + 25
3   com.apple.WebKit              	0x000000010ec6a3ff non-virtual thunk to WebKit::WebInspector::didClose(IPC::Connection&) + 47
4   com.apple.WebKit              	0x000000010e6b63fa IPC::Connection::connectionDidClose()::$_4::operator()() const + 122
5   com.apple.WebKit              	0x000000010e6b636c std::__1::__function::__func<IPC::Connection::connectionDidClose()::$_4, std::__1::allocator<IPC::Connection::connectionDidClose()::$_4>, void ()>::operator()() + 60
6   com.apple.JavaScriptCore      	0x0000000111d725ea std::__1::function<void ()>::operator()() const + 26 (functional:1755)
7   com.apple.JavaScriptCore      	0x0000000111e000f1 WTF::RunLoop::performWork() + 561 (RunLoop.cpp:106)
8   com.apple.JavaScriptCore      	0x0000000111e01424 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
...

* NOTES
(lldb) up
frame #2: 0x000000010ec66cc1 WebKit`WebKit::WebInspector::close(this=0x00007fec72fb43d8) + 33 at WebInspector.cpp:122
   119 	
   120 	void WebInspector::close()
   121 	{
-> 122 	    m_page->corePage()->inspectorController().close();
   123 	}
   124 	
   125 	void WebInspector::openInNewTab(const String& urlString)

(lldb) p m_page->corePage()
(WebCore::Page *) $1 = 0x0000000000000000

[Joseph Pecoraro]

<rdar://problem/19194743>

[Joseph Pecoraro]

Created attachment 245492 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 245492 [details]
[PATCH] Proposed Fix

I was also able to get this to happen on sendMessageToBackend. Seems we should do this in more places. I'll just check everywhere.

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000070

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x0000000113eac28b WebCore::Page::inspectorController() const + 43
1   com.apple.WebKit              	0x0000000113eab325 WebKit::WebInspector::sendMessageToBackend(WTF::String const&) + 37
2   com.apple.WebKit              	0x0000000113eb620f void IPC::callMemberFunctionImpl<WebKit::WebInspector, void (WebKit::WebInspector::*)(WTF::String const&), std::__1::tuple<WTF::String>, 0ul>(WebKit::WebInspector*, void (WebKit::WebInspector::*)(WTF::String const&), std::__1::tuple<WTF::String>&&, std::index_sequence<0ul>) + 159
3   com.apple.WebKit              	0x0000000113eb6168 void IPC::callMemberFunction<WebKit::WebInspector, void (WebKit::WebInspector::*)(WTF::String const&), std::__1::tuple<WTF::String>, std::make_index_sequence<1ul> >(std::__1::tuple<WTF::String>&&, WebKit::WebInspector*, void (WebKit::WebInspector::*)(WTF::String const&)) + 88
4   com.apple.WebKit              	0x0000000113eb60d6 void IPC::handleMessage<Messages::WebInspector::SendMessageToBackend, WebKit::WebInspector, void (WebKit::WebInspector::*)(WTF::String const&)>(IPC::MessageDecoder&, WebKit::WebInspector*, void (WebKit::WebInspector::*)(WTF::String const&)) + 230
5   com.apple.WebKit              	0x0000000113eb560a WebKit::WebInspector::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 1306
6   com.apple.WebKit              	0x0000000113eb5677 non-virtual thunk to WebKit::WebInspector::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 55
...

[Joseph Pecoraro]

Created attachment 245499 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

Comment on attachment 245499 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:262
> +    if (m_page->corePage()) {

Why no early return?

> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:273
> +    if (m_page->corePage())

Could store corePage in a local here and use on the next line.

[Joseph Pecoraro]

(In reply to comment #5)
> Comment on attachment 245499 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245499&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:262
> > +    if (m_page->corePage()) {
> 
> Why no early return?
> 
> > Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:273
> > +    if (m_page->corePage())
> 
> Could store corePage in a local here and use on the next line.

These two match in style because they are slightly different than the others.

As for storing into a local, I think the compiler is smart enough to do that for us here.

[WebKit Commit Bot]

Comment on attachment 245499 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 245499

Committed r179282: <http://trac.webkit.org/changeset/179282>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.