Bug 60931 – crash after r86584 due to calling willRemoveWheelEventHandler too many times

• Component: Frames
• Status: NEW

[Andrew Wilson]

r86584 relies on didAddWheelEventHandler() and willRemoveWheelEventHandler() getting called the same number of times.

We're getting errors on a few tests after this CL, and I think the implication is that willRemoveWheelEventHandler() is being called too often - I haven't been able to debug this after a few hours of trying, so I'm not sure of the cause:

ASSERTION FAILED: m_wheelEventHandlerCount > 0
Backtrace:
	WebCore::Document::didRemoveWheelEventHandler [0x01428ED6+70] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:5095)
	WebCore::FrameView::willRemoveHorizontalScrollbar [0x013D9242+98] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\frameview.cpp:323)
	WebCore::ScrollView::setHasHorizontalScrollbar [0x018CEF26+310] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\platform\scrollview.cpp:100)
	WebCore::ScrollView::updateScrollbars [0x018D088B+315] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\platform\scrollview.cpp:477)
	WebCore::ScrollView::setScrollbarModes [0x018CF271+193] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\platform\scrollview.cpp:163)
	WebCore::ScrollView::setHorizontalScrollbarMode [0x013DB774+52] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\platform\scrollview.h:92)
	WebCore::FrameView::layout [0x013DB1AA+1754] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\frameview.cpp:942)
	WebCore::Document::implicitClose [0x0141C023+851] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:2162)
	WebCore::FrameLoader::checkCallImplicitClose [0x013BF994+132] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:980)
	WebCore::FrameLoader::checkCompleted [0x013BF6FD+237] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:929)
	WebCore::FrameLoader::finishedParsing [0x013BF468+152] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:863)
	WebCore::Document::finishedParsing [0x0142538C+332] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:4273)
	WebCore::HTMLTreeBuilder::finished [0x0219C6B4+100] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmltreebuilder.cpp:2803)
	WebCore::HTMLDocumentParser::end [0x0213C963+131] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:379)
	WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd [0x0213CA36+182] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:388)
	WebCore::HTMLDocumentParser::prepareToStopParsing [0x0213B7AC+188] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:152)
	WebCore::HTMLDocumentParser::attemptToEnd [0x0213CAA9+57] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:399)
	WebCore::HTMLDocumentParser::finish [0x0213CBF3+51] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:428)
	WebCore::Document::finishParsing [0x0141C61B+75] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:2285)
	WebCore::DocumentWriter::endIfNotLoadingMainResource [0x0141342D+125] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:225)
	WebCore::DocumentWriter::end [0x01413397+39] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:210)
	WebCore::DocumentWriter::replaceDocument [0x014123D6+230] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:85)
	WebCore::ScriptController::executeIfJavaScriptURL [0x014C2B97+391] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\scriptcontrollerbase.cpp:114)
	WebCore::FrameLoader::urlSelected [0x013BCAB7+135] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:288)
	WebCore::FrameLoader::changeLocation [0x013BC8FD+157] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:271)
	WebCore::DOMWindow::createWindow [0x014B9995+677] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:1752)
	WebCore::DOMWindow::open [0x014B9EA7+983] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:1821)
	WebCore::V8DOMWindow::openCallback [0x016305CF+463] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\v8\custom\v8domwindowcustom.cpp:456)
	v8::internal::HandleApiCallHelper<0> [0x0064EE3C+892] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1126)
	v8::internal::Builtin_Impl_HandleApiCall [0x00649CD4+20] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1143)
	v8::internal::Builtin_HandleApiCall [0x00649C52+66] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\builtins.cc:1142)
	(No symbol) [0x0CC300B6]
	(No symbol) [0x3DBC413C]
	(No symbol) [0x0CC3115F]
	(No symbol) [0x0CC438D1]
	(No symbol) [0x0CC30B22]
	v8::internal::Invoke [0x004DAD1C+412] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:122)
	v8::internal::Execution::Call [0x004DAB62+34] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\execution.cc:153)
	v8::Function::Call [0x00478AD7+407] (e:\b\build\slave\webkit_win__dbg__2_\build\src\v8\src\api.cc:3297)
	WebCore::V8Proxy::callFunction [0x01381190+464] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\v8\v8proxy.cpp:483)
	WebCore::V8EventListener::callListenerFunction [0x01633907+167] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\v8\v8eventlistener.cpp:77)
	WebCore::V8AbstractEventListener::invokeEventHandler [0x01818E6C+332] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\v8\v8abstracteventlistener.cpp:154)
	WebCore::V8AbstractEventListener::handleEvent [0x01818B43+275] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\bindings\v8\v8abstracteventlistener.cpp:99)
	WebCore::EventTarget::fireEventListeners [0x01544F75+261] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\eventtarget.cpp:360)
	WebCore::EventTarget::fireEventListeners [0x01544E06+262] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\eventtarget.cpp:331)
	WebCore::DOMWindow::dispatchEvent [0x014B8E4E+238] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:1588)
	WebCore::DOMWindow::dispatchTimedEvent [0x014B9066+150] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:1601)
	WebCore::DOMWindow::dispatchLoadEvent [0x014B8BB2+242] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\page\domwindow.cpp:1564)
	WebCore::Document::dispatchWindowLoadEvent [0x0142145F+95] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:3523)
	WebCore::Document::implicitClose [0x0141BE66+406] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:2119)
	WebCore::FrameLoader::checkCallImplicitClose [0x013BF994+132] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:980)
	WebCore::FrameLoader::checkCompleted [0x013BF6FD+237] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:929)
	WebCore::FrameLoader::finishedParsing [0x013BF468+152] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\frameloader.cpp:863)
	WebCore::Document::finishedParsing [0x0142538C+332] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:4273)
	WebCore::HTMLTreeBuilder::finished [0x0219C6B4+100] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmltreebuilder.cpp:2803)
	WebCore::HTMLDocumentParser::end [0x0213C963+131] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:379)
	WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd [0x0213CA36+182] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:388)
	WebCore::HTMLDocumentParser::prepareToStopParsing [0x0213B7AC+188] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:152)
	WebCore::HTMLDocumentParser::attemptToEnd [0x0213CAA9+57] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:399)
	WebCore::HTMLDocumentParser::finish [0x0213CBF3+51] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\html\parser\htmldocumentparser.cpp:428)
	WebCore::Document::finishParsing [0x0141C61B+75] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\dom\document.cpp:2285)
	WebCore::DocumentWriter::endIfNotLoadingMainResource [0x0141342D+125] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\loader\documentwriter.cpp:225)

It doesn't seem to fail on the webkit.org bots, nor can I get it to fail in a chromium build, so I think that it may just be a chromium DRT/TestShell issue. I'm going to disable the tests that break as a result of this so I can roll forward.

[Jon Lee]

<rdar://problem/9449486>

[Jon Lee]

*** Bug 64954 has been marked as a duplicate of this bug. ***

[Rob Bradford]

My notes from the duplicated bug:

void Document::didRemoveWheelEventHandler()
{
    ASSERT(m_wheelEventHandlerCount > 0);
    --m_wheelEventHandlerCount;
    Frame* mainFrame = page() ? page()->mainFrame() : 0;
    if (mainFrame)
        mainFrame->notifyChromeClientWheelEventHandlerCountChanged();
}

When navigating from the page.

I observe the following from the backtraces

#0  WebCore::Document::didAddWheelEventHandler (this=0x74b240)
    at /home/rob/src/efl-webkit/source/WebKit/Source/WebCore/dom/Document.cpp:5050

and then

#0  WebCore::Document::didRemoveWheelEventHandler (this=0x856fa0)
    at /home/rob/src/efl-webkit/source/WebKit/Source/WebCore/dom/Document.cpp:5059

and then

#0  WebCore::Document::didAddWheelEventHandler (this=0x856fa0)
    at /home/rob/src/efl-webkit/source/WebKit/Source/WebCore/dom/Document.cpp:5050

So from my observations the Document inside the Frame has changed before the code that's doing the tidy up to remove the handlers gets called.

i.e. we would expect the call to didRemoveWheelEventHandler to act on 0x74b240.

[Rob Bradford]

Created attachment 101733 [details]
Patch to detach scrollbars

The problem is that when the document gets detached from the frame the scrollbars don't get removed.

One solution that seems to work for me is to always detach the scrollbars when you detach the the document from the frame.

Here is a patch to do that. First patch to WebKit, be gentle. :-)

[Rob Bradford]

Comment on attachment 101733 [details]
Patch to detach scrollbars

Patch should have been review? not review+

[WebKit Review Bot]

Attachment 101733 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Document.cpp:1793:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Rob Bradford]

Created attachment 101882 [details]
Updated patch to address automated review issue

[Mario Sanchez Prada]

This bug seems to be the cause of some tests crashing in the GTK bots too. See the backtrace here:

 0x00002b12b17b1d2b in WebCore::Document::didRemoveWheelEventHandler (this=0x728d780) at ../../Source/WebCore/dom/Document.cpp:5167
#1  0x00002b12b1bf13b0 in WebCore::FrameView::willRemoveHorizontalScrollbar (this=0x7330890, scrollbar=0x73ed700) at ../../Source/WebCore/page/FrameView.cpp:337
#2  0x00002b12b1d1a12c in WebCore::ScrollView::setHasHorizontalScrollbar (this=0x7330890, hasBar=false) at ../../Source/WebCore/platform/ScrollView.cpp:98
#3  0x00002b12b1d1b3f9 in WebCore::ScrollView::updateScrollbars (this=0x7330890, desiredOffset=...) at ../../Source/WebCore/platform/ScrollView.cpp:468
#4  0x00002b12b254a61a in WebCore::ScrollView::setScrollbarModes (this=0x7330890, horizontalMode=WebCore::ScrollbarAlwaysOff, verticalMode=WebCore::ScrollbarAlwaysOn, horizontalLock=false, verticalLock=false) at ../../Source/WebCore/platform/gtk/ScrollViewGtk.cpp:117
#5  0x00002b12b1bfa735 in WebCore::ScrollView::setHorizontalScrollbarMode (this=0x7330890, mode=WebCore::ScrollbarAlwaysOff, lock=false) at ../../Source/WebCore/platform/ScrollView.h:93
#6  0x00002b12b1bf2dfd in WebCore::FrameView::layout (this=0x7330890, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1059
#7  0x00002b12b17a62ac in WebCore::Document::implicitClose (this=0x728d780) at ../../Source/WebCore/dom/Document.cpp:2231
#8  0x00002b12b1b2d519 in WebCore::FrameLoader::checkCallImplicitClose (this=0x740a1d8) at ../../Source/WebCore/loader/FrameLoader.cpp:796
#9  0x00002b12b1b2d2ec in WebCore::FrameLoader::checkCompleted (this=0x740a1d8) at ../../Source/WebCore/loader/FrameLoader.cpp:744
#10 0x00002b12b1b2d053 in WebCore::FrameLoader::finishedParsing (this=0x740a1d8) at ../../Source/WebCore/loader/FrameLoader.cpp:678
#11 0x00002b12b17ae8da in WebCore::Document::finishedParsing (this=0x728d780) at ../../Source/WebCore/dom/Document.cpp:4306
#12 0x00002b12b1a07c86 in WebCore::HTMLTreeBuilder::finished (this=0x738dc80) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2826
#13 0x00002b12b19dd5dc in WebCore::HTMLDocumentParser::end (this=0x74b4f00) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:381
#14 0x00002b12b19dd6d9 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x74b4f00) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:390
#15 0x00002b12b19dc7ef in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x74b4f00) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:153
#16 0x00002b12b19dd71e in WebCore::HTMLDocumentParser::attemptToEnd (this=0x74b4f00) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:402
#17 0x00002b12b19dd7d7 in WebCore::HTMLDocumentParser::finish (this=0x74b4f00) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:429
#18 0x00002b12b1b28062 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x73210a0) at ../../Source/WebCore/loader/DocumentWriter.cpp:235
#19 0x00002b12b1b27f79 in WebCore::DocumentWriter::end (this=0x73210a0) at ../../Source/WebCore/loader/DocumentWriter.cpp:214
#20 0x00002b12b1b273f7 in WebCore::DocumentWriter::replaceDocument (this=0x73210a0, source="bar") at ../../Source/WebCore/loader/DocumentWriter.cpp:84
#21 0x00002b12b1627b37 in WebCore::ScriptController::executeIfJavaScriptURL (this=0x740a5f0, url=..., shouldReplaceDocumentIfJavaScriptURL=WebCore::ReplaceDocumentIfJavaScriptURL) at ../../Source/WebCore/bindings/ScriptControllerBase.cpp:128
#22 0x00002b12b1b2b54e in WebCore::FrameLoader::urlSelected (this=0x740a1d8, passedRequest=..., triggeringEvent=..., lockHistory=true, lockBackForwardList=false, referrerPolicy=WebCore::SendReferrer, shouldReplaceDocumentIfJavaScriptURL=WebCore::ReplaceDocumentIfJavaScriptURL) at ../../Source/WebCore/loader/FrameLoader.cpp:284
#23 0x00002b12b1b2b2f1 in WebCore::FrameLoader::changeLocation (this=0x740a1d8, securityOrigin=0x73f1c10, url=..., referrer="http://127.0.0.1:8000/security/cookies/cookie-theft-with-javascript-doc.html", lockHistory=true, lockBackForwardList=false, refresh=false) at ../../Source/WebCore/loader/FrameLoader.cpp:267
#24 0x00002b12b1b71378 in WebCore::ScheduledURLNavigation::fire (this=0x736f630, frame=0x740a120) at ../../Source/WebCore/loader/NavigationScheduler.cpp:109
#25 0x00002b12b1b70ba1 in WebCore::NavigationScheduler::timerFired (this=0x740a538) at ../../Source/WebCore/loader/NavigationScheduler.cpp:418
#26 0x00002b12b1b72d60 in WebCore::Timer<WebCore::NavigationScheduler>::fired (this=0x740a540) at ../../Source/WebCore/platform/Timer.h:100
#27 0x00002b12b1d40f70 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0xbe4fc0) at ../../Source/WebCore/platform/ThreadTimers.cpp:115
#28 0x00002b12b1d40ea7 in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:93
#29 0x00002b12b254ac16 in WebCore::timeout_cb () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49
#30 0x00002b12b66afddb in g_timeout_dispatch (source=0x7470510, callback=<optimized out>, user_data=<optimized out>) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3882
#31 0x00002b12b66ae4a3 in g_main_dispatch (context=0xb11e60) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:2440
#32 g_main_context_dispatch (context=0xb11e60) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3013
#33 0x00002b12b66aec80 in g_main_context_iterate (context=0xb11e60, block=1, dispatch=1, self=<optimized out>) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3091
#34 0x00002b12b66af2f2 in g_main_loop_run (loop=0x713b750) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3299
#35 0x00002b12b445c4cd in gtk_main () from /usr/lib/libgtk-3.so.0
#36 0x000000000042f2e5 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:710
#37 0x000000000042e91d in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:502
#38 0x0000000000430c58 in main (argc=2, argv=0x7fff16b2f508) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1205

Skipping in GTK in the meanwhile.

[Julien Chaffraix]

Comment on attachment 101882 [details]
Updated patch to address automated review issue

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

> Source/WebCore/ChangeLog:7
> +

It's unfortunate that there is no test case. The bug refers to 3 ASSERT in Chromium's test_expectations.txt, have you checked that your patch solves them? If it does not solve them, it looks like this is not a fix for the right bug. You would need an explanation as to why there is no test in this case.

> Source/WebCore/dom/Document.cpp:1793
> +            view->detachScrollbars();

There are 2 other callers to detachCustomScrollbars, wouldn't they also need to be converted to avoid such a bug too?

> Source/WebCore/page/FrameView.cpp:313
> +void FrameView::detachScrollbars()

It would also be neat to change FrameView::~FrameView to use this method for consistency.

> Source/WebCore/page/FrameView.cpp:316
> +        return;

I wonder if this check is needed. In detachCustomScrollbars, it looks like a legacy as none of the following code requires a Frame.

[Adam Klein]

(In reply to comment #9)
> (From update of attachment 101882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101882&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> It's unfortunate that there is no test case. The bug refers to 3 ASSERT in Chromium's test_expectations.txt, have you checked that your patch solves them? If it does not solve them, it looks like this is not a fix for the right bug. You would need an explanation as to why there is no test in this case.

These tests haven't crashed since the end of October, perhaps fixed by http://trac.webkit.org/changeset/98778/ (it's the only one in the span that mentions wheel events.  Any further info on this?

[Julien Chaffraix]

Comment on attachment 101882 [details]
Updated patch to address automated review issue

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

The change looks sane to me but I think Andersca should give the final review as he knows the code a lot more than I do.

>>> Source/WebCore/ChangeLog:7
>>> +
>> 
>> It's unfortunate that there is no test case. The bug refers to 3 ASSERT in Chromium's test_expectations.txt, have you checked that your patch solves them? If it does not solve them, it looks like this is not a fix for the right bug. You would need an explanation as to why there is no test in this case.
> 
> These tests haven't crashed since the end of October, perhaps fixed by http://trac.webkit.org/changeset/98778/ (it's the only one in the span that mentions wheel events.  Any further info on this?

Fine enough, this change would need a test case then or an explanation as to why it lacks one as nothing seems to be checking that the calls are properly paired.

[Johnny(Jianning) Ding]

(In reply to comment #11)
> Fine enough, this change would need a test case then or an explanation as to why it lacks one as nothing seems to be checking that the calls are properly paired.

One idea to implement the test is to add a method to LayoutTestController to enable/disable log of adding/removing scrollbar.

By the way, the related tests still crashed according to my investigation.

[Johnny(Jianning) Ding]

Seems this bug hasn't attracted attention for a while.

After reading the comments for Rob's patch, I was curious why there was only customized scrollbar got removed before, so I dug the old related bug, which is https://bugs.webkit.org/show_bug.cgi?id=26326. In the comment 9 of bug 26326, Dave said "You don't want to always destroy the scrollbars.  You only want to nuke them if the document supplied them.  If they were custom basically.".

@Dave,would you like give the context about why we should not always destroy the scrollbars when the view's document gets detached. This issue is just because we don't do that.

@Rob, will you have time to continue working on this issue?

[Ryosuke Niwa]

Comment on attachment 101882 [details]
Updated patch to address automated review issue

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

> Source/WebCore/ChangeLog:5
> +        Ensure the scrollbars are always removed when the page is detached
> +        otherwise unpaired calls to Document::didAddWheelEventHandler and
> +        Document::didRemoveWheelEventHandler are made.

This doesn't match the bug summary. Please move the description to below the bug url followed by a blank line.
Also, you're missing "Reviewed by" below. r- because of this.