Bug 155515 - [Win] Correct double-release of CFURLConnectionRef
Summary: [Win] Correct double-release of CFURLConnectionRef
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-15 15:14 PDT by Brent Fulgham
Modified: 2016-03-15 17:00 PDT (History)
2 users (show)

See Also:


Attachments
Patch (217 bytes, patch)
2016-03-15 15:21 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (1.09 KB, patch)
2016-03-15 15:25 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2016-03-15 16:05 PDT, Brent Fulgham
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-03-15 15:14:38 PDT
A double-release of a CFURLConnectionRef was identified in one of the WebDownload::init methods:

    CFURLConnectionRef connection = handle->connection();

    …

    m_download = adoptCF(CFURLDownloadCreateAndStartWithLoadingConnection(0, connection, request.cfURLRequest(UpdateHTTPBody), response.cfURLResponse(), &client));

    …

    // The CFURLDownload either starts successfully and retains the CFURLConnection, 
    // or it fails to creating and we have a now-useless connection with a dangling ref. 
    // Either way, we need to release the connection to balance out ref counts
    handle->releaseConnectionForDownload();
    CFRelease(connection);

The last line, the call to CFRelease(connection), is wrong and should be removed, because ResourceHandle::connection() just does 

    d->m_connection.get()

CFURLDownloadCreateAndStartWithLoadingConnection() can retain the connection per the comment, while ResourceHandle:: releaseConnectionForDownload() lets go of the connection.

But then we release the connection via the raw pointer we stole from the ResourceHandle, as if we thought ResourceHandle::connection() returned a retained connection!
Comment 1 Brent Fulgham 2016-03-15 15:15:30 PDT
<rdar://problem/25159143>
Comment 2 Brent Fulgham 2016-03-15 15:19:33 PDT
Note: It looks like this code is not tested in the LayoutTest system because the 'http/tests/downloads/' test suite is skipped on Windows due to missing DRT features.
Comment 3 Brent Fulgham 2016-03-15 15:21:34 PDT
Created attachment 274141 [details]
Patch
Comment 4 Brent Fulgham 2016-03-15 15:25:24 PDT
Created attachment 274143 [details]
Patch
Comment 5 Brent Fulgham 2016-03-15 16:05:02 PDT
Created attachment 274150 [details]
Patch
Comment 6 Brent Fulgham 2016-03-15 16:59:50 PDT
Testing infrastructure on Windows is needed to avoid breaking this in the future. See Bug 155522.
Comment 7 Brent Fulgham 2016-03-15 17:00:10 PDT
Committed r198244: <http://trac.webkit.org/changeset/198244>