60931 2011-05-16 16:07:12 -0700 crash after r86584 due to calling willRemoveWheelEventHandler too many times 2012-04-22 22:28:03 -0700 1 1 1 Unclassified WebKit Frames 528+ (Nightly build) Unspecified Unspecified NEW InRadar P2 Normal --- 69777 1 atwilson webkit-unassigned adamk andersca beidson dglazkov hyatt jam jchaffraix jnd jonlee mario rakuco rob webkit-bug-importer webkit.review.bot oldest_to_newest 404795 0 atwilson 2011-05-16 16:07:12 -0700 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. 404840 1 jonlee 2011-05-16 17:27:22 -0700 <rdar://problem/9449486> 440457 2 jonlee 2011-07-21 09:46:17 -0700 *** Bug 64954 has been marked as a duplicate of this bug. *** 440935 3 rob 2011-07-22 07:00:42 -0700 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. 440951 4 101733 rob 2011-07-22 07:59:46 -0700 Created attachment 101733 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. :-) 441690 5 101733 rob 2011-07-25 09:22:21 -0700 Comment on attachment 101733 Patch to detach scrollbars Patch should have been review? not review+ 441693 6 webkit.review.bot 2011-07-25 09:24:40 -0700 Attachment 101733 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. 441703 7 101882 rob 2011-07-25 09:47:22 -0700 Created attachment 101882 Updated patch to address automated review issue 481570 8 mario 2011-10-11 04:24:12 -0700 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. 499120 9 101882 jchaffraix 2011-11-09 10:34:45 -0800 Comment on attachment 101882 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. 505991 10 adamk 2011-11-18 13:06:31 -0800 (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? 506399 11 101882 jchaffraix 2011-11-19 07:45:19 -0800 Comment on attachment 101882 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. 509151 12 jnd 2011-11-25 06:49:35 -0800 (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. 603921 13 jnd 2012-04-17 03:59:47 -0700 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? 607463 14 101882 rniwa 2012-04-22 22:28:03 -0700 Comment on attachment 101882 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. 101733 2011-07-22 07:59:46 -0700 2011-07-25 09:47:22 -0700 Patch to detach scrollbars patch-to-detach-scrollbars.diff text/plain 2311 rob ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3ZTU1MDk0Li43OWUxZmQxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDctMjIgIFJvYiBCcmFkZm9yZCAgIDxyb2JAbGludXguaW50ZWwuY29tPgorCisg ICAgICAgIEVuc3VyZSB0aGUgc2Nyb2xsYmFycyBhcmUgYWx3YXlzIHJlbW92ZWQgd2hlbiB0aGUg cGFnZSBpcyBkZXRhY2hlZAorICAgICAgICBvdGhlcndpc2UgdW5wYWlyZWQgY2FsbHMgdG8gRG9j dW1lbnQ6OmRpZEFkZFdoZWVsRXZlbnRIYW5kbGVyIGFuZAorICAgICAgICBEb2N1bWVudDo6ZGlk UmVtb3ZlV2hlZWxFdmVudEhhbmRsZXIgYXJlIG1hZGUuCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MDkzMQorCisgICAgICAgICogcGFnZS9GcmFtZVZp ZXcuY3BwOgorICAgICAgICAqIHBhZ2UvRnJhbWVWaWV3Lmg6CisgICAgICAgIEFkZCBkZXRhY2hT Y3JvbGxiYXJzIG1ldGhvZC4KKyAgICAgICAgKiBkb20vRG9jdW1lbnQuaDoKKyAgICAgICAgRGV0 YWNoIHNjcm9sbGJhcnMgd2hlbiB0aGUgZG9jdW1lbnQgaXMgZGV0YWNoZWQuCisKIDIwMTEtMDct MjEgIFBhdmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBnb29nbGUuY29tPgogCiAgICAgICAgIFdlYiBJ bnNwZWN0b3I6IFtwcm90b2NvbF0gZXh0ZW5kIHNldEJyZWFrcG9pbnRCeVVybCByZXF1ZXN0IHRv IGFjY2VwdCBpbmNvbXBsZXRlIFVSTCAocHJlZGljYXRlcykKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnQuY3Bw CmluZGV4IGQyZTA5NDQuLjE2NDc0ZmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9E b2N1bWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmNwcApAQCAtMTc5 MCw3ICsxNzkwLDcgQEAgdm9pZCBEb2N1bWVudDo6ZGV0YWNoKCkKICAgICBpZiAobV9mcmFtZSkg ewogICAgICAgICBGcmFtZVZpZXcqIHZpZXcgPSBtX2ZyYW1lLT52aWV3KCk7CiAgICAgICAgIGlm ICh2aWV3KQotICAgICAgICAgICAgdmlldy0+ZGV0YWNoQ3VzdG9tU2Nyb2xsYmFycygpOworICAg ICAgICAgICAgdmlldy0+ZGV0YWNoU2Nyb2xsYmFycyAoKTsKIAogICAgIH0KIApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwIGIvU291cmNlL1dlYkNvcmUvcGFn ZS9GcmFtZVZpZXcuY3BwCmluZGV4IGJjODg4MzIuLjg2ZjRhNDQgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BhZ2UvRnJhbWVWaWV3LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL0Zy YW1lVmlldy5jcHAKQEAgLTMxMCw2ICszMTAsMjAgQEAgdm9pZCBGcmFtZVZpZXc6OmRldGFjaEN1 c3RvbVNjcm9sbGJhcnMoKQogICAgIH0KIH0KIAordm9pZCBGcmFtZVZpZXc6OmRldGFjaFNjcm9s bGJhcnMoKQoreworICAgIGlmICghbV9mcmFtZSkKKyAgICAgICAgcmV0dXJuOworCisgICAgc2V0 SGFzSG9yaXpvbnRhbFNjcm9sbGJhcihmYWxzZSk7CisgICAgc2V0SGFzVmVydGljYWxTY3JvbGxi YXIoZmFsc2UpOworCisgICAgaWYgKG1fc2Nyb2xsQ29ybmVyKSB7CisgICAgICAgIG1fc2Nyb2xs Q29ybmVyLT5kZXN0cm95KCk7CisgICAgICAgIG1fc2Nyb2xsQ29ybmVyID0gMDsKKyAgICB9Cit9 CisKIHZvaWQgRnJhbWVWaWV3OjpkaWRBZGRIb3Jpem9udGFsU2Nyb2xsYmFyKFNjcm9sbGJhciog c2Nyb2xsYmFyKQogewogICAgIGlmIChtX2ZyYW1lICYmIG1fZnJhbWUtPmRvY3VtZW50KCkpCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5oIGIvU291cmNlL1dlYkNv cmUvcGFnZS9GcmFtZVZpZXcuaAppbmRleCAwMzZiNThiLi44YmU0ZWU3IDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BhZ2Uv RnJhbWVWaWV3LmgKQEAgLTE0MCw2ICsxNDAsNyBAQCBwdWJsaWM6CiAgICAgdm9pZCByZXNldFNj cm9sbGJhcnMoKTsKICAgICB2b2lkIHJlc2V0U2Nyb2xsYmFyc0FuZENsZWFyQ29udGVudHNTaXpl KCk7CiAgICAgdm9pZCBkZXRhY2hDdXN0b21TY3JvbGxiYXJzKCk7CisgICAgdm9pZCBkZXRhY2hT Y3JvbGxiYXJzKCk7CiAgICAgdmlydHVhbCB2b2lkIHJlY2FsY3VsYXRlU2Nyb2xsYmFyT3Zlcmxh eVN0eWxlKCk7CiAKICAgICB2b2lkIGNsZWFyKCk7Cg== 101882 2011-07-25 09:47:22 -0700 2012-04-22 22:28:03 -0700 Updated patch to address automated review issue updated-patch.diff text/plain 2310 rob ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA3ZTU1MDk0Li43OWUxZmQxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDctMjIgIFJvYiBCcmFkZm9yZCAgIDxyb2JAbGludXguaW50ZWwuY29tPgorCisg ICAgICAgIEVuc3VyZSB0aGUgc2Nyb2xsYmFycyBhcmUgYWx3YXlzIHJlbW92ZWQgd2hlbiB0aGUg cGFnZSBpcyBkZXRhY2hlZAorICAgICAgICBvdGhlcndpc2UgdW5wYWlyZWQgY2FsbHMgdG8gRG9j dW1lbnQ6OmRpZEFkZFdoZWVsRXZlbnRIYW5kbGVyIGFuZAorICAgICAgICBEb2N1bWVudDo6ZGlk UmVtb3ZlV2hlZWxFdmVudEhhbmRsZXIgYXJlIG1hZGUuCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MDkzMQorCisgICAgICAgICogcGFnZS9GcmFtZVZp ZXcuY3BwOgorICAgICAgICAqIHBhZ2UvRnJhbWVWaWV3Lmg6CisgICAgICAgIEFkZCBkZXRhY2hT Y3JvbGxiYXJzIG1ldGhvZC4KKyAgICAgICAgKiBkb20vRG9jdW1lbnQuaDoKKyAgICAgICAgRGV0 YWNoIHNjcm9sbGJhcnMgd2hlbiB0aGUgZG9jdW1lbnQgaXMgZGV0YWNoZWQuCisKIDIwMTEtMDct MjEgIFBhdmVsIEZlbGRtYW4gIDxwZmVsZG1hbkBnb29nbGUuY29tPgogCiAgICAgICAgIFdlYiBJ bnNwZWN0b3I6IFtwcm90b2NvbF0gZXh0ZW5kIHNldEJyZWFrcG9pbnRCeVVybCByZXF1ZXN0IHRv IGFjY2VwdCBpbmNvbXBsZXRlIFVSTCAocHJlZGljYXRlcykKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnQuY3Bw CmluZGV4IGQyZTA5NDQuLjE2NDc0ZmYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9E b2N1bWVudC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvZG9tL0RvY3VtZW50LmNwcApAQCAtMTc5 MCw3ICsxNzkwLDcgQEAgdm9pZCBEb2N1bWVudDo6ZGV0YWNoKCkKICAgICBpZiAobV9mcmFtZSkg ewogICAgICAgICBGcmFtZVZpZXcqIHZpZXcgPSBtX2ZyYW1lLT52aWV3KCk7CiAgICAgICAgIGlm ICh2aWV3KQotICAgICAgICAgICAgdmlldy0+ZGV0YWNoQ3VzdG9tU2Nyb2xsYmFycygpOworICAg ICAgICAgICAgdmlldy0+ZGV0YWNoU2Nyb2xsYmFycygpOwogCiAgICAgfQogCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9wYWdlL0ZyYW1lVmlldy5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdl L0ZyYW1lVmlldy5jcHAKaW5kZXggYmM4ODgzMi4uODZmNGE0NCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BhZ2UvRnJh bWVWaWV3LmNwcApAQCAtMzEwLDYgKzMxMCwyMCBAQCB2b2lkIEZyYW1lVmlldzo6ZGV0YWNoQ3Vz dG9tU2Nyb2xsYmFycygpCiAgICAgfQogfQogCit2b2lkIEZyYW1lVmlldzo6ZGV0YWNoU2Nyb2xs YmFycygpCit7CisgICAgaWYgKCFtX2ZyYW1lKQorICAgICAgICByZXR1cm47CisKKyAgICBzZXRI YXNIb3Jpem9udGFsU2Nyb2xsYmFyKGZhbHNlKTsKKyAgICBzZXRIYXNWZXJ0aWNhbFNjcm9sbGJh cihmYWxzZSk7CisKKyAgICBpZiAobV9zY3JvbGxDb3JuZXIpIHsKKyAgICAgICAgbV9zY3JvbGxD b3JuZXItPmRlc3Ryb3koKTsKKyAgICAgICAgbV9zY3JvbGxDb3JuZXIgPSAwOworICAgIH0KK30K Kwogdm9pZCBGcmFtZVZpZXc6OmRpZEFkZEhvcml6b250YWxTY3JvbGxiYXIoU2Nyb2xsYmFyKiBz Y3JvbGxiYXIpCiB7CiAgICAgaWYgKG1fZnJhbWUgJiYgbV9mcmFtZS0+ZG9jdW1lbnQoKSkKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvRnJhbWVWaWV3LmggYi9Tb3VyY2UvV2ViQ29y ZS9wYWdlL0ZyYW1lVmlldy5oCmluZGV4IDAzNmI1OGIuLjhiZTRlZTcgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3BhZ2UvRnJhbWVWaWV3LmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9G cmFtZVZpZXcuaApAQCAtMTQwLDYgKzE0MCw3IEBAIHB1YmxpYzoKICAgICB2b2lkIHJlc2V0U2Ny b2xsYmFycygpOwogICAgIHZvaWQgcmVzZXRTY3JvbGxiYXJzQW5kQ2xlYXJDb250ZW50c1NpemUo KTsKICAgICB2b2lkIGRldGFjaEN1c3RvbVNjcm9sbGJhcnMoKTsKKyAgICB2b2lkIGRldGFjaFNj cm9sbGJhcnMoKTsKICAgICB2aXJ0dWFsIHZvaWQgcmVjYWxjdWxhdGVTY3JvbGxiYXJPdmVybGF5 U3R5bGUoKTsKIAogICAgIHZvaWQgY2xlYXIoKTsK