Bug 152213 - Web Inspector: Too many derefs when RemoteInspectorXPCConnection fails to validate connection
Summary: Web Inspector: Too many derefs when RemoteInspectorXPCConnection fails to val...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-12-11 23:27 PST by Joseph Pecoraro
Modified: 2015-12-11 23:44 PST (History)
13 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.96 KB, patch)
2015-12-11 23:29 PST, Joseph Pecoraro
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-12-11 23:27:43 PST
* SUMMARY
Too many derefs when RemoteInspectorXPCConnection fails to validate connection.

We call closeOnQueue() and deref(), but the closeOnQueue will cause an XPC_ERROR_CONNECTION_INVALID to deref() the connection again. We should only deref in one place, this code should just trigger the close.
Comment 1 Radar WebKit Bug Importer 2015-12-11 23:27:56 PST
<rdar://problem/23870973>
Comment 2 Joseph Pecoraro 2015-12-11 23:29:34 PST
Created attachment 267223 [details]
[PATCH] Proposed Fix
Comment 3 Ryosuke Niwa 2015-12-11 23:33:00 PST
Comment on attachment 267223 [details]
[PATCH] Proposed Fix

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

rs=me

> Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:-182
> -                m_closed = true;
> -                m_client = nullptr;

It looks like we always evaluate these two statements before calling closeOnQueue() elsewhere.
Is it really safe not to set these values?
Comment 4 Joseph Pecoraro 2015-12-11 23:43:07 PST
(In reply to comment #3)
> Comment on attachment 267223 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267223&action=review
> 
> rs=me
> 
> > Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm:-182
> > -                m_closed = true;
> > -                m_client = nullptr;
> 
> It looks like we always evaluate these two statements before calling
> closeOnQueue() elsewhere.
> Is it really safe not to set these values?

Thanks for the careful review! I don't think it is necessary here.

We don't want to set m_client = nullptr yet, because we do want to inform the client that we are closing, which will happen in the handling of XPC_ERROR_CONNECTION_INVALID. The case of `close` and `closeFromMessage` it is the client doing the closing, so they don't need to be informed that the connection is closing.

We could set m_close, but we might as well delay that until when we actually do the closing with XPC_ERROR_CONNECTION_INVALID triggered by this. So waiting and closing there feels more natural.
Comment 5 Joseph Pecoraro 2015-12-11 23:44:47 PST
<http://trac.webkit.org/changeset/194005>