Bug 45287 – [chromium] Leak in RenderViewTest::SimulateElementClick()

• Component: Platform
• Status: RESOLVED FIXED

[Hironori Bono]

(Copied from <http://crbug.com/54654>).

From: http://build.chromium.org/buildbot/memory/builders/Linux%20Tests%20(valgrind)(1)/builds/5806/steps/memory%20test:%20unit/logs/stdio

22:45:43 memcheck_analyze.py [ERROR] Command: src/sconsbuild/Release/unit_tests --gtest_filter=-RenderViewTest.FLAKY_OnHandleKeyboardEvent:VisitedLinkEventsTest.Coalescense:PredictorTest.FLAKY_MassiveConcurrentLookupTest:VisitedLinkEventsTest.FAILS_Coalescense:PredictorTest.MassiveConcurrentLookupTest:RenderViewTest.FAILS_OnHandleKeyboardEvent:RenderViewTest.FAILS_ImeComposition:VisitedLinkRelayTest.FAILS_Basics:VisitedLinkRelayTest.FLAKY_Basics:PredictorTest.FAILS_MassiveConcurrentLookupTest:ConnectionTesterTest.RunAllTests:VisitedLinkRelayTest.Basics:ConnectionTesterTest.FLAKY_RunAllTests:VisitedLinkEventsTest.FLAKY_Coalescense:ConnectionTesterTest.FAILS_RunAllTests:RenderViewTest.FLAKY_ImeComposition:URLFetcherBadHTTPSTest.FAILS_BadHTTPSTest:RenderViewTest.OnHandleKeyboardEvent:URLFetcherBadHTTPSTest.FLAKY_BadHTTPSTest:RenderViewTest.ImeComposition:URLFetcherBadHTTPSTest.BadHTTPSTest --gtest_print_time
Leak_DefinitelyLost
120 bytes in 1 blocks are definitely lost in loss record 43,384 of 46,252
  malloc (sr/local/google/valgrind-for-chromium-client/valgrind/scripts/valgrind-memcheck/coregrind/m_replacemalloc/vg_replace_malloc.c:241)
  WTF::fastMalloc(unsigned int) (uilder/build/src/third_party/WebKit/JavaScriptCore/wtf/FastMalloc.cpp:250)
  WTF::FastAllocBase::operator new(unsigned int) (uilder/build/src/third_party/WebKit/JavaScriptCore/wtf/FastAllocBase.h:96)
  WebCore::MouseEvent::create(WTF::AtomicString const&, bool, bool, WTF::PassRefPtr<WebCore::DOMWindow>, int, int, int, int, int, bool, bool, bool, bool, unsigned short, WTF::PassRefPtr<WebCore::EventTarget>, WTF::PassRefPtr<WebCore::Clipboard>, bool) (uilder/build/src/third_party/WebKit/WebCore/dom/MouseEvent.h:45)
  WebCore::Node::dispatchMouseEvent(WTF::AtomicString const&, int, int, int, int, int, int, bool, bool, bool, bool, bool, WebCore::Node*, WTF::PassRefPtr<WebCore::Event>) (uilder/build/src/third_party/WebKit/WebCore/dom/Node.cpp:2881)
  WebCore::Node::dispatchMouseEvent(WebCore::PlatformMouseEvent const&, WTF::AtomicString const&, int, WebCore::Node*) (uilder/build/src/third_party/WebKit/WebCore/dom/Node.cpp:2794)
  WebCore::EventHandler::dispatchMouseEvent(WTF::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) (uilder/build/src/third_party/WebKit/WebCore/page/EventHandler.cpp:1849)
  WebCore::EventHandler::handleMousePressEvent(WebCore::PlatformMouseEvent const&) (uilder/build/src/third_party/WebKit/WebCore/page/EventHandler.cpp:1309)
  WebKit::WebViewImpl::mouseDown(WebKit::WebMouseEvent const&) (uilder/build/src/third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:402)
  WebKit::WebViewImpl::handleInputEvent(WebKit::WebInputEvent const&) (uilder/build/src/third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:1050)
  RenderWidget::OnHandleInputEvent(IPC::Message const&) (uilder/build/src/chrome/renderer/render_widget.cc:345)
  RenderViewTest::SimulateElementClick(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (uilder/build/src/chrome/test/render_view_test.cc:328)
  RenderViewTest_PageClickTracker_Test::TestBody() (uilder/build/src/chrome/renderer/page_click_tracker_unittest.cc:70)
  testing::Test::Run() (uilder/build/src/testing/gtest/src/gtest.cc:2095)
Suppression (error hash=#FFFFFFFFF0F8F49A#):
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   fun:malloc
   fun:_ZN3WTF10fastMallocEj
   fun:_ZN3WTF13FastAllocBasenwEj
   fun:_ZN7WebCore10MouseEvent6createERKN3WTF12AtomicStringEbbNS1_10PassRefPtrINS_9DOMWindowEEEiiiiibbbbtNS5_INS_11EventTargetEEENS5_INS_9ClipboardEEEb
   fun:_ZN7WebCore4Node18dispatchMouseEventERKN3WTF12AtomicStringEiiiiiibbbbbPS0_NS1_10PassRefPtrINS_5EventEEE
   fun:_ZN7WebCore4Node18dispatchMouseEventERKNS_18PlatformMouseEventERKN3WTF12AtomicStringEiPS0_
   fun:_ZN7WebCore12EventHandler18dispatchMouseEventERKN3WTF12AtomicStringEPNS_4NodeEbiRKNS_18PlatformMouseEventEb
   fun:_ZN7WebCore12EventHandler21handleMousePressEventERKNS_18PlatformMouseEventE
   fun:_ZN6WebKit11WebViewImpl9mouseDownERKNS_13WebMouseEventE
   fun:_ZN6WebKit11WebViewImpl16handleInputEventERKNS_13WebInputEventE
   fun:_ZN12RenderWidget18OnHandleInputEventERKN3IPC7MessageE
   fun:_ZN14RenderViewTest20SimulateElementClickERKSs
   fun:_ZN36RenderViewTest_PageClickTracker_Test8TestBodyEv
}


It seems Chromium change r58597 <http://crrev.com/58597> unveils a leak in WebKit. It seems WebDOMEvent::assign() increases the reference count of a MouseEvent object without decreasing it. So, we cannot delete the MouseEvent object.

[Hironori Bono]

Created attachment 66701 [details]
A quick fix

Even though I'm not sure if this is a good fix, I would like to post it for your information.

Regards,

Hironori Bono

[Jay Civelli]

Created attachment 66787 [details]
Shorter patch

[Jay Civelli]

@Hironori
Thanks for looking at this!
I uploaded a shorter patch inspired on WebNode.
Let me know what you think.

Thanks.

Jay

[Hironori Bono]

(In reply to comment #3)
> @Hironori
> Thanks for looking at this!
> I uploaded a shorter patch inspired on WebNode.
> Let me know what you think.

Thank you for your quick work. I think it's better than mine. (I forgot making the destructor to a virtual function.)

Regards,

Hironori Bono

[Darin Fisher (:fishd, Google)]

Comment on attachment 66787 [details]
Shorter patch

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

> WebKit/chromium/public/WebDOMEvent.h:55
> +    virtual ~WebDOMEvent() { reset(); }
nit: please list the destructor first w/ a new line between it and WebDOMEvent (as is done in WebNode.h).

I'm also not sure why this destructor needs to be virtual.  This adds a virtual table to this class.  I notice that WebNode also has a virtual destructor, but I can't figure out why it should need one either.

R=me, but please consider making the above changes before committing.

[Jay Civelli]

Created attachment 67061 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 67061 [details]
Patch for landing

Clearing flags on attachment: 67061

Committed r67188: <http://trac.webkit.org/changeset/67188>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.