Summary: | Memory leak for a protected element having pending events in ImageLoader | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | kyounga <kyounga.ra> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson, cdumez, commit-queue, japhet | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
kyounga
2015-07-01 19:12:16 PDT
Created attachment 255984 [details]
Use RefPtr not calling ref/deref() directly
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; (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. Created attachment 255993 [details]
Use RefPtr not calling ref/deref() directly
(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. 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. Created attachment 256057 [details]
Use RefPtr not calling ref/deref() directly
(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. (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 :) 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> All reviewed patches have been landed. Closing bug. |