Bug 136038 - Resource leak in object - memory allocated in constructor but not freed in destructor.
Summary: Resource leak in object - memory allocated in constructor but not freed in de...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-18 04:39 PDT by Albert Malewski
Modified: 2016-09-17 07:10 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.47 KB, patch)
2014-08-18 05:18 PDT, Albert Malewski
no flags Details | Formatted Diff | Diff
Patch (3.69 KB, patch)
2014-08-19 07:38 PDT, Albert Malewski
no flags Details | Formatted Diff | Diff
Patch (1.51 KB, patch)
2014-08-25 00:16 PDT, Albert Malewski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Malewski 2014-08-18 04:39:35 PDT
Constructor of ResourceLoadScheduler allocates field m_nonHTTPProtocolHost. This field needs to be freed in destructor.
Comment 1 Albert Malewski 2014-08-18 05:18:17 PDT
Created attachment 236759 [details]
Patch
Comment 2 Andreas Kling 2014-08-18 09:18:58 PDT
Comment on attachment 236759 [details]
Patch

m_nonHTTPProtocolHost should be a std::unique_ptr<HostInformation> instead. That way we don't have to manually delete it.
Comment 3 Alexey Proskuryakov 2014-08-18 22:58:51 PDT
Does the leak occur in practice? It looks like ResourceLoadScheduler object is a singleton, and is never expected to be deleted.

Unless I'm misreading the code, what needs to be fixed is:
1. We should have ASSERT_NOT_REACHED in the destructor.
2. WebResourceLoadScheduler (a subclass of ResourceLoadScheduler) should not have a public constructor.
Comment 4 Albert Malewski 2014-08-19 03:11:53 PDT
(In reply to comment #3)
In my opinion, WebResourceLoadScheduler should have a public constructor because it is used in WebProcess class. Of course, we can mark WebProcess class as a friend of WebResourceLoadScheduler, but I think it does not make any sense, because in result it does not change anything..
Comment 5 Albert Malewski 2014-08-19 07:38:47 PDT
Created attachment 236811 [details]
Patch
Comment 6 Darin Adler 2014-08-19 09:00:48 PDT
Comment on attachment 236811 [details]
Patch

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

It’s kind of weak to use unique_ptr for m_nonHTTPProtocolHost but not do the same thing for the hosts in m_hosts.

> Source/WebCore/ChangeLog:3
> +        Fixed resource leak in object 

This is not a good name for a bug. It’s much too vague. Almost like “fixed bug in project”.

And also, it’s inaccurate. This patch doesn’t fix a leak. It makes some inconsequential changes in a class, which I suppose could be helpful if someone didn’t realize the object was an immortal singleton.

> Source/WebCore/ChangeLog:8
> +        Changed m_nonHTTPProtocolHost from HostInformation* to std::unique_ptr<HostInformation> and added ASSERT_NOT_REACHED() in the destructor.

It’s a little strange to do both of these things. It’s correct to assert that the destructor is never called. That’s what makes it clear we don’t really need a unique_ptr. But I suppose it’s OK to use unique_ptr just to be a little clearer about object ownership.

> Source/WebCore/loader/ResourceLoadScheduler.cpp:107
> +    m_nonHTTPProtocolHost = std::make_unique<ResourceLoadScheduler::HostInformation>(String(), maxRequestsInFlightForNonHTTPProtocols);

Why change from construction syntax to assignment syntax? For unique_ptr that’s less efficient for no good reason. Just seems like gratuitous “make coding style a little worse” change.
Comment 7 Alexey Proskuryakov 2014-08-19 10:20:34 PDT
> In my opinion, WebResourceLoadScheduler should have a public constructor because it is used in WebProcess class.

Yes, this is how it is used today. What I was getting at is that this design appears wrong, because it requires exposing a public constructor for a singleton.
Comment 8 Albert Malewski 2014-08-25 00:16:06 PDT
Created attachment 237061 [details]
Patch
Comment 9 Albert Malewski 2014-08-25 01:08:47 PDT
Created a patch with changed access to constructor of WebResourceLoadScheduler: https://bugs.webkit.org/show_bug.cgi?id=136126
Comment 10 Albert Malewski 2014-08-25 01:38:41 PDT
Created a patch with changed access to constructor of WebResourceLoadScheduler: https://bugs.webkit.org/show_bug.cgi?id=136126
Comment 11 Michael Catanzaro 2016-09-17 07:10:37 PDT
Comment on attachment 237061 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.