Bug 146538 – Memory leak for a protected element having pending events in ImageLoader

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[kyounga]

We found a memory leak possibility in ImageLoader.
See the code below
====
ImageLoader::~ImageLoader()
{
..
    if (m_elementIsProtected)
        element().deref();
}
void ImageLoader::updatedHasPendingEvent()
{
    bool wasProtected = m_elementIsProtected;
    m_elementIsProtected = m_hasPendingLoadEvent || m_hasPendingErrorEvent;
    if (wasProtected == m_elementIsProtected)
        return;

    if (m_elementIsProtected) {
        if (m_derefElementTimer.isActive())
            m_derefElementTimer.stop();
        else
            element().ref();
    } else {
        ASSERT(!m_derefElementTimer.isActive());
        m_derefElementTimer.startOneShot(0);
    }
}
void ImageLoader::timerFired()
{
    element().deref();
}
====

Let's supposed that an protected element is to be "unprotected".
The "m_elementIsProtected" flag is false immediately, but deref() is called later using Timer.
If ImageLoader is destroyed before the timer is fired, the element's refCount never be zero.
I didn't create a test case but there is logically 100% memory leak in this case.

[kyounga]

Created attachment 255984 [details]
Use RefPtr not calling ref/deref() directly

[Brady Eidson]

Comment on attachment 255984 [details]
Use RefPtr not calling ref/deref() directly

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

> Source/WebCore/loader/ImageLoader.cpp:363
> +            m_protectedElement= &element();

Type: Missing a space before the =.

(Really bummed that check-webkit-style didn't get this)

> Source/WebCore/loader/ImageLoader.cpp:372
> +    m_protectedElement.clear();

This should be:
m_protectedElement = nullptr;

[kyounga]

(In reply to comment #2)
> Comment on attachment 255984 [details]
> Use RefPtr not calling ref/deref() directly
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255984&action=review
> 
> > Source/WebCore/loader/ImageLoader.cpp:363
> > +            m_protectedElement= &element();
> 
> Type: Missing a space before the =.
> 
> (Really bummed that check-webkit-style didn't get this)
> 
> > Source/WebCore/loader/ImageLoader.cpp:372
> > +    m_protectedElement.clear();
> 
> This should be:
> m_protectedElement = nullptr;


m_protectedElement is RefPtr.
clear() will clear the internal pointer to nullptr.

I will change the typo.

[kyounga]

Created attachment 255993 [details]
Use RefPtr not calling ref/deref() directly

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 255984 [details]
> > Use RefPtr not calling ref/deref() directly
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255984&action=review
> > 
> > > Source/WebCore/loader/ImageLoader.cpp:363
> > > +            m_protectedElement= &element();
> > 
> > Type: Missing a space before the =.
> > 
> > (Really bummed that check-webkit-style didn't get this)
> > 
> > > Source/WebCore/loader/ImageLoader.cpp:372
> > > +    m_protectedElement.clear();
> > 
> > This should be:
> > m_protectedElement = nullptr;
> 
> 
> m_protectedElement is RefPtr.
> clear() will clear the internal pointer to nullptr.

I know.

Assigning a nullptr to a RefPtr is functionally the same as calling clear() on it.

But when somebody not familiar with the code is reading it, and they come across a call to ".clear()", they don't necessarily know you're null'ing out a RefPtr. They might think you're calling "clear()" on some object that has a whole bunch of behavior inside of its ::clear() method.

But by assigning nullptr, it's obvious to anybody reading the code "This is a pointer - it might be a raw pointer or one of our various types of smart pointers, but I know it's a pointer and this code is null'ing it out"

This request is about readability by people not familiar with the code.

[Brady Eidson]

Comment on attachment 255993 [details]
Use RefPtr not calling ref/deref() directly

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

> Source/WebCore/loader/ImageLoader.cpp:372
> -    element().deref();
> +    m_protectedElement.clear();

Change this to:
m_protectedElement = nullptr;

With this code, people have to ask themselves "what kind of object is this, and what does clear do?"
With nullptr assignment, anybody not familiar with the code knows it is a pointer being null'ed out.

[kyounga]

Created attachment 256057 [details]
Use RefPtr not calling ref/deref() directly

[kyounga]

(In reply to comment #6)
> Comment on attachment 255993 [details]
> Use RefPtr not calling ref/deref() directly
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255993&action=review
> 
> > Source/WebCore/loader/ImageLoader.cpp:372
> > -    element().deref();
> > +    m_protectedElement.clear();
> 
> Change this to:
> m_protectedElement = nullptr;
> 
> With this code, people have to ask themselves "what kind of object is this,
> and what does clear do?"
> With nullptr assignment, anybody not familiar with the code knows it is a
> pointer being null'ed out.

I thought calling clear() is more explicit. Also, I understand your thought.
I attached a changed patch.
Thanks for your comment.

[Brady Eidson]

(In reply to comment #8)
> (In reply to comment #6)
> > Comment on attachment 255993 [details]
> > Use RefPtr not calling ref/deref() directly
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=255993&action=review
> > 
> > > Source/WebCore/loader/ImageLoader.cpp:372
> > > -    element().deref();
> > > +    m_protectedElement.clear();
> > 
> > Change this to:
> > m_protectedElement = nullptr;
> > 
> > With this code, people have to ask themselves "what kind of object is this,
> > and what does clear do?"
> > With nullptr assignment, anybody not familiar with the code knows it is a
> > pointer being null'ed out.
> 
> I thought calling clear() is more explicit. Also, I understand your thought.
> I attached a changed patch.
> Thanks for your comment.

FWIW, this discussion spawned https://bugs.webkit.org/show_bug.cgi?id=146556 in which we intend to remove RefPtr::clear altogether :)

[WebKit Commit Bot]

Comment on attachment 256057 [details]
Use RefPtr not calling ref/deref() directly

Clearing flags on attachment: 256057

Committed r186267: <http://trac.webkit.org/changeset/186267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.