Bug 64954 – Unpaired calls to Document::didAddWheelEventHandler / Document::didRemoveWheelEventHandler

• Component: WebCore Misc.
• Status: UNCONFIRMED

[Rob Bradford]

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.

[Jon Lee]

This is a dupe of 60931.

*** This bug has been marked as a duplicate of bug 60931 ***

[Jon Lee]

Sorry, this should not have been marked as a dupe.  Back to unconfirmed, and assigning to me.

[Rob Bradford]

Created attachment 103748 [details]
Proposed patch

Here is the patch I proposed on the other bug.

[Eric Seidel (no email)]

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 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?

[David Levin]

(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.

[Ojan Vafai]

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.