Bug 64954 - Unpaired calls to Document::didAddWheelEventHandler / Document::didRemoveWheelEventHandler
Summary: Unpaired calls to Document::didAddWheelEventHandler / Document::didRemoveWhee...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-21 08:59 PDT by Rob Bradford
Modified: 2012-05-01 14:51 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (2.26 KB, patch)
2011-08-12 00:51 PDT, Rob Bradford
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Bradford 2011-07-21 08:59:04 PDT
With the EFL backend and some Chromium use cases (according to https://bugs.webkit.org/show_bug.cgi?id=60779#c27) we trigger the assertion below:

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.
Comment 1 Jon Lee 2011-07-21 09:46:17 PDT
This is a dupe of 60931.

*** This bug has been marked as a duplicate of bug 60931 ***
Comment 2 Jon Lee 2011-08-11 17:37:42 PDT
Sorry, this should not have been marked as a dupe.  Back to unconfirmed, and assigning to me.
Comment 3 Rob Bradford 2011-08-12 00:51:04 PDT
Created attachment 103748 [details]
Proposed patch

Here is the patch I proposed on the other bug.
Comment 4 Eric Seidel (no email) 2011-12-19 10:38:36 PST
Comment on attachment 103748 [details]
Proposed patch

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

> Source/WebCore/page/FrameView.h:143
> +    void detachScrollbars();

Señor Levin wrote http://trac.webkit.org/changeset/44940, so it seems he should be involved in this review as well.
Comment 5 jochen 2012-01-26 01:15:01 PST
Comment on attachment 103748 [details]
Proposed patch

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

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

you're dropping the call to detachCustomScrollbars here. Should detachScrollbars() invoke detachCustomScrollbars() then?
Comment 6 David Levin 2012-01-26 05:24:12 PST
(In reply to comment #4)
> (From update of attachment 103748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103748&action=review
> 
> > Source/WebCore/page/FrameView.h:143
> > +    void detachScrollbars();
> 
> Señor Levin wrote http://trac.webkit.org/changeset/44940, so it seems he should be involved in this review as well.

2 1/2 years ago :( 
To my memory I was fixing a crash and consulted with several other folks about the right fix. Given all this my expertise is about none for this.
Comment 7 Ojan Vafai 2012-05-01 14:51:39 PDT
Comment on attachment 103748 [details]
Proposed patch

Is this visible to the web? If so, can you add layout tests for this? All WebKit patches need a test unless there's a good reason why it can't be tested.