[webkit-reviews] review denied: [Bug 136038] Resource leak in object - memory allocated in constructor but not freed in destructor. : [Attachment 236811] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 19 09:00:39 PDT 2014

Darin Adler <darin at apple.com> has denied Albert Malewski
<a.malewski at samsung.com>'s request for review:
Bug 136038: Resource leak in object - memory allocated in constructor but not
freed in destructor.

Attachment 236811: Patch

------- Additional Comments from Darin Adler <darin at apple.com>
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

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 =

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.

More information about the webkit-reviews mailing list