Bug 30167 - [Qt] Memory leak caught in WebCore/platform/network/qt/QNetworkReplyHandler.cpp
Summary: [Qt] Memory leak caught in WebCore/platform/network/qt/QNetworkReplyHandler.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-10-07 07:59 PDT by Chang Shu
Modified: 2009-10-22 12:56 PDT (History)
2 users (show)

See Also:

Attachments
patch (1.12 KB, patch)
2009-10-07 08:32 PDT, Chang Shu 		cshu: review-
Details | Formatted Diff | Diff
2nd patch (1.48 KB, patch)
2009-10-22 09:14 PDT, Chang Shu 		no flags Details | Formatted Diff | Diff
update ChangeLog (1.66 KB, patch)
2009-10-22 11:29 PDT, Chang Shu 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2009-10-07 07:59:19 PDT 
Memory leak caught in WebCore/platform/network/qt/QNetworkReplyHandler.cpp.
Function QNetworkReply::abort()
Comment 1 Chang Shu 2009-10-07 08:32:25 PDT 
Created attachment 40790 [details]
patch
Comment 2 Eric Seidel (no email) 2009-10-07 09:18:10 PDT 
Comment on attachment 40790 [details]
patch

Wow.  deleteLater() is a very scary memory management model.  But OK I trust you.  rubber-stamp=me.
Comment 3 Chang Shu 2009-10-08 08:17:40 PDT 
(In reply to comment #2)
> (From update of attachment 40790 [details])
> Wow.  deleteLater() is a very scary memory management model.  But OK I trust
> you.  rubber-stamp=me.

Eric, Please set this to r- as it may cause problem. I am working on that. Sorry.
Comment 4 Chang Shu 2009-10-22 09:14:00 PDT 
Created attachment 41662 [details]
2nd patch
Comment 5 Eric Seidel (no email) 2009-10-22 10:51:31 PDT 
What sort of problems did the other patch cause?  Can we test those problems with a unit test/layout test?
Comment 6 Chang Shu 2009-10-22 10:56:38 PDT 
(In reply to comment #5)
> What sort of problems did the other patch cause?  Can we test those problems
> with a unit test/layout test?

If m_reply is NOT unhooked from QNetworkReplyHandler in release() function, it may reference QNetworkReplyHandler after the latter is deleted. This is the problem of the first patch. Not sure how to come up with a test case... It's related to download.
Comment 7 Eric Seidel (no email) 2009-10-22 10:58:55 PDT 
Comment on attachment 41662 [details]
2nd patch

Ideally your ChangeLog would give explanations like this, and make some small statement about why this change is untestable.
Comment 8 Chang Shu 2009-10-22 11:29:37 PDT 
Created attachment 41670 [details]
update ChangeLog
Comment 9 Eric Seidel (no email) 2009-10-22 11:33:15 PDT 
Comment on attachment 41670 [details]
update ChangeLog

OK.  I'm not a Qt expert, but as far as I can tell this looks sane.
Comment 10 WebKit Commit Bot 2009-10-22 12:56:16 PDT 
Comment on attachment 41670 [details]
update ChangeLog

Clearing flags on attachment: 41670

Committed r49951: <http://trac.webkit.org/changeset/49951>
Comment 11 WebKit Commit Bot 2009-10-22 12:56:20 PDT 
All reviewed patches have been landed.  Closing bug.