Bug 156420 – Use WeakPtrs to avoid using deallocated Widgets and ScrollableAreas

• Component: UI Events
• Status: RESOLVED FIXED

[Brent Fulgham]

Bugs 150871 and 156409 were caused by Events destroying elements that were currently in use to handle wheel events. Since the EventHandler logic held onto raw pointers to these elements (not expecting them to be destroyed during the same method call), we would dereference these invalid pointers causing crashes and other problems.

We can do a better job by using WeakPtrs (instead of bare pointers) that will become null when the underlying object is destroyed. This will convert various use-after-free problems into null-pointer dereferences, which are much safer, and easier to find. By using proper null checking, we can avoid crashes and ensure proper behavior in these cases.

[Brent Fulgham]

Created attachment 276050 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/25637378>

[Brent Fulgham]

Created attachment 276060 [details]
Patch

[Brent Fulgham]

Oh -- I see I should have skipped this on iOS, just like the original "iframe" test case.

[Brent Fulgham]

Created attachment 276066 [details]
Patch

[Darin Adler]

Comment on attachment 276066 [details]
Patch

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

Generally I think this is a good solution, but I think it’s not as tight as it could be.

> Source/WebCore/page/EventHandler.cpp:2613
> +static WeakPtr<RenderWidget> widgetForElement(const Element& element)

This used to return the widget. Now it just returns the renderer. Keeping the class name the same is a little bit peculiar. Maybe even call it renderWidgetForElement. But also, it’s only called in one place; I don’t think it’s all that helpful a function.

> Source/WebCore/page/EventHandler.cpp:2622
> -    RenderElement* target = element.renderer();
> -    if (!target)
> -        return nullptr;
> +    WeakPtr<RenderWidget> widget;
>  
> -    if (!is<RenderWidget>(target))
> -        return nullptr;
> +    if (RenderElement* target = element.renderer()) {
> +        if (is<RenderWidget>(target))
> +            widget = downcast<RenderWidget>(*target).createWeakPtr();
> +    }
>  
> -    return downcast<RenderWidget>(*target).widget();
> +    return widget;

As long as we are rewriting this, I suggest we use early return to tighten it up:

    auto* renderer = element.renderer();
    if (!is<RenderWidget>(renderer))
        return nullptr;
    return downcast<RenderWidget>(*renderer).createWeakPtr();

> Source/WebCore/page/EventHandler.cpp:2680
> +                if (passWheelEventToWidget(event, *renderWidget->widget())) {
> +                    m_isHandlingWheelEvent = false;
> +
> +                    // We do another check on the widget because the event handler can run JS which results in the frame getting destroyed.
> +                    if (!renderWidget || !renderWidget->widget())
> +                        return false;
> +
> +                    if (scrollableArea)
> +                        scrollableArea->setScrolledProgrammatically(false);
> +                    platformNotifyIfEndGesture(adjustedEvent, scrollableArea);
> +                    if (!renderWidget->widget()->platformWidget())
> +                        return true;
> +                    return platformCompletePlatformWidgetWheelEvent(event, *renderWidget->widget(), scrollableContainer.get());
> +                }

I think we should consider putting all of this into a helper function and factoring it out of handleWheelEvent entirely; handleWheelEvent is pretty huge. Combined with the suggestion above, it would look something like this:

    auto* renderer = element.renderer();
    if (is<RenderWidget>(renderer) && downcast<RenderWidget>(*renderer).widget())
        return passWheelEventToWidget(event, adjustedEvent, downcast<RenderWidget>(*renderer), scrollableContainer, scrollableArea);

The WeakPtr for the RenderWidget could be created inside the passWheelEventToWidget function.

> Source/WebCore/page/mac/EventHandlerMac.mm:879
> -    Widget* widget = widgetForEventTarget(eventTarget);
> -    if (!widget)
> -        return nullptr;
> -    
> -    if (!widget->isScrollView())
> -        return nullptr;
> -    
> -    return reinterpret_cast<ScrollView*>(widget);
> +    WeakPtr<ScrollableArea> scrollableArea;
> +
> +    if (Widget* widget = widgetForEventTarget(eventTarget)) {
> +        if (widget->isScrollView())
> +            scrollableArea = reinterpret_cast<ScrollView*>(widget)->createScrollableAreaWeakPtr();
> +    }
> +
> +    return scrollableArea;

Again, early return makes a tighter function:

    auto* widget = widgetForEventTarget(eventTarget);
    if (!widget || !widget->isScrollView())
        return nullptr;
    return reinterpret_cast<ScrollView*>(widget)->createScrollableAreaWeakPtr();

By the way, really unclear why it’s reinterpret_cast and not static_cast. Please use static_cast instead unless there is some really good reason not to.

Also seems bad that EventHandler and EventHandlerMac have code that’s close to identical but copied and pasted. Maybe fix that by making some of these static member functions in EventHandler.cpp?

> Source/WebCore/page/mac/EventHandlerMac.mm:929
> -    ScrollableArea* scrollableArea = nullptr;
> +    WeakPtr<ScrollableArea> scrollableArea;
>  
> -    if (RenderBox* box = container.renderBox())
> -        scrollableArea = scrollableAreaForBox(*box);
> +    if (RenderBox* box = container.renderBox()) {
> +        ScrollableArea* scrollableAreaPtr = scrollableAreaForBox(*box);
> +        scrollableArea = scrollableAreaPtr->createScrollableAreaWeakPtr();
> +    }
>  
>      return scrollableArea;

Early return will be better here too:

    auto* box = container.renderBox();
    if (!box)
        return nullptr;
    return scrollableAreaForBox(*box)->createScrollableAreaWeakPtr();

What guarantees scrollableAreaForBox won’t return nullptr? The old code did not depend on this, and so maybe the new code should not either. If it’s guaranteed to return non-null, should it return a reference instead of a pointer?

> Source/WebCore/page/mac/EventHandlerMac.mm:992
> +            bool startingAtScrollLimit = scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea.get(), wheelEvent.deltaX(), wheelEvent.deltaY());

What guarantees scrollableArea.get() won’t return a nullptr? It’s not OK to use * on a nullptr.

> Source/WebCore/platform/ScrollableArea.h:67
> +    WeakPtr<ScrollableArea> createScrollableAreaWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }

I think this should just be named createWeakPtr. I don’t think it enhances anything to repeat the class name. I suppose at one point it seemed that we would add a weak pointer factory for both Widget and ScrollableArea and I suppose if we had done that then we would need to disambiguate either by using this kind of function name or by using a typecast. But I don’t think there is a conflict that matters in practice in the current patch.

Seems a bit inconsistent that for Widget we chose to use the WeakPtrFactory that already exists for RenderWidget and not create a new one for Widget, then re-get the Widget from the RenderWidget each time. But for ScrollableArea we chose to add a new factory. Not sure which is better.

[Brent Fulgham]

Created attachment 276131 [details]
Patch

[Brent Fulgham]

Comment on attachment 276066 [details]
Patch

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

Thank you for reviewing this. I thought these notes had been committed to the bug previously, but I guess I never committed the comments. I've tried to recapture them here.

>> Source/WebCore/page/EventHandler.cpp:2613
>> +static WeakPtr<RenderWidget> widgetForElement(const Element& element)
> 
> This used to return the widget. Now it just returns the renderer. Keeping the class name the same is a little bit peculiar. Maybe even call it renderWidgetForElement. But also, it’s only called in one place; I don’t think it’s all that helpful a function.

In keeping with some later comments you made, I'll change this tinderbox a WeakPtr to widget. That cleans up some other logic.

>> Source/WebCore/page/EventHandler.cpp:2622
>> +    return widget;
> 
> As long as we are rewriting this, I suggest we use early return to tighten it up:
> 
>     auto* renderer = element.renderer();
>     if (!is<RenderWidget>(renderer))
>         return nullptr;
>     return downcast<RenderWidget>(*renderer).createWeakPtr();

Will do.

>> Source/WebCore/page/EventHandler.cpp:2680
>> +                }
> 
> I think we should consider putting all of this into a helper function and factoring it out of handleWheelEvent entirely; handleWheelEvent is pretty huge. Combined with the suggestion above, it would look something like this:
> 
>     auto* renderer = element.renderer();
>     if (is<RenderWidget>(renderer) && downcast<RenderWidget>(*renderer).widget())
>         return passWheelEventToWidget(event, adjustedEvent, downcast<RenderWidget>(*renderer), scrollableContainer, scrollableArea);
> 
> The WeakPtr for the RenderWidget could be created inside the passWheelEventToWidget function.

I can take the innermost clause and make it a function, but the outermost tests gate whether we execute the inner clause and return early, or drop down to later processing.

I'm also going to rename 'passWheelEventToWidget' to better reflect that this is also a test that controls whether we perform the steps beneath.

>> Source/WebCore/page/mac/EventHandlerMac.mm:879
>> +    return scrollableArea;
> 
> Again, early return makes a tighter function:
> 
>     auto* widget = widgetForEventTarget(eventTarget);
>     if (!widget || !widget->isScrollView())
>         return nullptr;
>     return reinterpret_cast<ScrollView*>(widget)->createScrollableAreaWeakPtr();
> 
> By the way, really unclear why it’s reinterpret_cast and not static_cast. Please use static_cast instead unless there is some really good reason not to.
> 
> Also seems bad that EventHandler and EventHandlerMac have code that’s close to identical but copied and pasted. Maybe fix that by making some of these static member functions in EventHandler.cpp?

In my latest patch I tried to clean this up slightly, but I think it would be better to address more cleanup a second patch.

>> Source/WebCore/page/mac/EventHandlerMac.mm:929
>>      return scrollableArea;
> 
> Early return will be better here too:
> 
>     auto* box = container.renderBox();
>     if (!box)
>         return nullptr;
>     return scrollableAreaForBox(*box)->createScrollableAreaWeakPtr();
> 
> What guarantees scrollableAreaForBox won’t return nullptr? The old code did not depend on this, and so maybe the new code should not either. If it’s guaranteed to return non-null, should it return a reference instead of a pointer?

You are right -- that could be null and should be protected against. I'll revise the function.

>> Source/WebCore/page/mac/EventHandlerMac.mm:992
>> +            bool startingAtScrollLimit = scrolledToEdgeInDominantDirection(*scrollableContainer, *scrollableArea.get(), wheelEvent.deltaX(), wheelEvent.deltaY());
> 
> What guarantees scrollableArea.get() won’t return a nullptr? It’s not OK to use * on a nullptr.

I believe the WeakPtr book test (line 991) will fail if the pointer is null.

>> Source/WebCore/platform/ScrollableArea.h:67
>> +    WeakPtr<ScrollableArea> createScrollableAreaWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }
> 
> I think this should just be named createWeakPtr. I don’t think it enhances anything to repeat the class name. I suppose at one point it seemed that we would add a weak pointer factory for both Widget and ScrollableArea and I suppose if we had done that then we would need to disambiguate either by using this kind of function name or by using a typecast. But I don’t think there is a conflict that matters in practice in the current patch.
> 
> Seems a bit inconsistent that for Widget we chose to use the WeakPtrFactory that already exists for RenderWidget and not create a new one for Widget, then re-get the Widget from the RenderWidget each time. But for ScrollableArea we chose to add a new factory. Not sure which is better.

Will do. As I mentioned earlier, I revised the code to use a WeakPtr<Widget> as well.

[Brent Fulgham]

(In reply to comment #8)
> Comment on attachment 276066 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276066&action=review
> 
> In keeping with some later comments you made, I'll change this tinderbox a
> WeakPtr to widget. That cleans up some other logic.

'Tinderbox' is clearly iPhone speak for "to return"

[Darin Adler]

Comment on attachment 276131 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2617
> +    

Shouldn’t add this leading whitespace.

> Source/WebCore/page/EventHandler.cpp:2621
> -
> +    

Shouldn’t add this leading whitespace.

> Source/WebCore/page/EventHandler.cpp:2634
> +bool EventHandler::completeWidgetWheelEvent(const PlatformWheelEvent& event, WeakPtr<Widget>& widget, WeakPtr<ScrollableArea>& scrollableArea, ContainerNode* scrollableContainer)

The WeakPtr arguments should be const&, not non-const&.

> Source/WebCore/page/mac/EventHandlerMac.mm:909
> +        return WeakPtr<ScrollableArea>();

Can all the lines like this change to:

    return { };

If so, I think that reads better.

[Brent Fulgham]

Comment on attachment 276131 [details]
Patch

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

>> Source/WebCore/page/EventHandler.cpp:2617
>> +    
> 
> Shouldn’t add this leading whitespace.

Will fix.

>> Source/WebCore/page/EventHandler.cpp:2621
>> +    
> 
> Shouldn’t add this leading whitespace.

Will fix.

>> Source/WebCore/page/EventHandler.cpp:2634
>> +bool EventHandler::completeWidgetWheelEvent(const PlatformWheelEvent& event, WeakPtr<Widget>& widget, WeakPtr<ScrollableArea>& scrollableArea, ContainerNode* scrollableContainer)
> 
> The WeakPtr arguments should be const&, not non-const&.

Will fix.

>> Source/WebCore/page/mac/EventHandlerMac.mm:909
>> +        return WeakPtr<ScrollableArea>();
> 
> Can all the lines like this change to:
> 
>     return { };
> 
> If so, I think that reads better.

This works. I'll make the change.

[Brent Fulgham]

Committed r199331: <http://trac.webkit.org/changeset/199331>