Bug 30167

Summary:

[Qt] Memory leak caught in WebCore/platform/network/qt/QNetworkReplyHandler.cpp

Product:

WebKit

Reporter:

Chang Shu <cshu>

Component:

Platform

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

S60 Hardware

OS:

S60 3rd edition

Attachments:

Description	Flags
patch	cshu: review-
2nd patch	none
update ChangeLog	none

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.