Bug 228901 – UBSan: KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value nnn, which is not a valid value for type 'bool'

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

UBSan: KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value nnn, which is not a valid value for type 'bool'

Occurs here:

    void KeyboardScrollingAnimator::handleKeyUpEvent()
    {
        if (!m_scrollTriggeringKeyIsPressed)   // UBSan warning
            return;

        stopKeyboardScrollAnimation();
        m_scrollTriggeringKeyIsPressed = false;
    }

Caused by the m_wasAccumulatingRepaintRegion instance variable not being initialized:

    class KeyboardScrollingAnimator {
        [...]
    private:
        [...]
        bool m_scrollTriggeringKeyIsPressed;  // BUG: No default initialization.
        [...]
    };

Affects the following 78 layout tests:

accessibility/aria-slider-value-change.html
accessibility/insert-children-assert.html
accessibility/mac/focus-moves-cursor.html
accessibility/mac/input-type-change-crash-2.html
accessibility/mac/input-type-change-crash.html
accessibility/mac/selection-initial.html
accessibility/mac/text-marker-line-boundary.html
accessibility/spinbutton-crash.html
editing/caret/emoji.html
editing/deleting/5729680.html
editing/input/caret-at-the-edge-of-input.html
editing/input/cocoa/autocorrect-off.html
editing/inserting/typing-tab-designmode-forms.html
editing/mac/deleting/backward-delete.html
editing/mac/spelling/autocorrection-blockquote-crash.html
editing/pasteboard/emacs-ctrl-a-k-y.html
editing/selection/context-menu-text-selection-lookup.html
editing/selection/move-begin-end.html
editing/selection/verify-editing-behavior-for-line-granularity.html
fast/dom/MutationObserver/inline-event-listener.html
fast/dom/access-key-iframe.html
fast/dom/fragment-activation-focuses-target.html
fast/dom/hidden-iframe-no-focus.html
fast/dom/mutation-details-focus.html
fast/events/autoscroll-should-not-stop-on-keypress.html
fast/events/beforeunload-alert-handled-keydown.html
fast/events/event-input-contentEditable.html
fast/events/focus-label-legend-elements-with-tab.html
fast/events/select-element.html
fast/events/tab-focus-anchor.html
fast/forms/access-key-case-insensitive.html
fast/forms/call-text-did-change-in-text-field-when-typing.html
fast/forms/datalist/datalist-option-labels.html
fast/forms/datetimelocal/datetimelocal-editable-components/datetimelocal-editable-components-keyboard-events.html
fast/forms/disabled-search-input.html
fast/forms/input-first-letter-edit.html
fast/forms/legend-access-key.html
fast/forms/month/month-editable-components/month-editable-components-focus-and-blur-events.html
fast/forms/onchange-enter-submit.html
fast/forms/radio/input-radio-checked-tab.html
fast/forms/range/range-keyboard-oninput-event.html
fast/forms/search-event-delay.html
fast/forms/tabbing-input-iframe.html
fast/forms/time/time-editable-components/time-editable-components-focus-and-blur-events.html
fast/forms/validation-message-maxLength.html
fast/frames/focus-controller-crash-change-event.html
fast/frames/iframe-window-focus.html
fast/html/details-keyboard-show-hide.html
fast/html/progress-user-modify.html
fast/repaint/fixed-move-after-keyboard-scroll.html
fast/scrolling/arrow-key-scroll-in-rtl-document.html
fast/text/scroll-text-overflow-ellipsis.html
fullscreen/full-screen-crash-custom-scrollbars.html
fullscreen/full-screen-iframe-allowed-prefixed.html
fullscreen/full-screen-table-section.html
http/tests/fullscreen/fullscreenelement-different-origin.html
http/tests/navigation/keyboard-events-during-provisional-subframe-navigation.html
http/tests/pointer-lock/iframe-sandboxed-nested-allow-pointer-lock.html
http/tests/storageAccess/aggregate-sorted-data-with-storage-access.html
http/tests/storageAccess/deny-without-prompt-preserves-gesture.html
http/tests/storageAccess/request-and-grant-access-cross-origin-non-sandboxed-iframe-ephemeral.html
http/tests/storageAccess/request-and-grant-access-cross-origin-non-sandboxed-iframe.html
http/tests/storageAccess/request-and-grant-access-then-navigate-same-site-should-have-access.html
http/tests/storageAccess/request-and-grant-access-with-per-page-scope-access-from-another-frame.html
imported/blink/fast/events/click-focus-keydown-no-ring.html
imported/w3c/web-platform-tests/css/css-contain/content-visibility/content-visibility-047.html
imported/w3c/web-platform-tests/css/css-scroll-snap/input/keyboard.html
imported/w3c/web-platform-tests/css/selectors/focus-visible-008.html
imported/w3c/web-platform-tests/html/interaction/focus/focus-keyboard-js.html
imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/maxlength-number.html
imported/w3c/web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-negative-delegatesFocus.html
imported/w3c/web-platform-tests/shadow-dom/focus/focus-tabindex-order-shadow-negative.html
platform/mac/fast/events/non-roman-key-code.html
pointer-lock/lock-element-not-in-dom.html
scrollbars/scrollbar-miss-mousemove-disabled.html
svg/custom/focus-event-handling-keyboard.xhtml
tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-horizontal-with-keyboard-scaled.html
webaudio/audiocontext-restriction-audiobuffersourcenode-start.html

How to find list of tests:

$ cd OpenSource/LayoutTests
$ $ for F in `grep -l -r 'KeyboardScrollingAnimator.cpp:303:10' ../WebKitBuild/layout-test-results/ | sed -e 's#^.*//##' -e 's/-stderr.txt/*/'`; do ls $F | grep -v 'expected'; done | sort | pbcopy

[David Kilzer (:ddkilzer)]

Regressed in r280492 for Bug 228009.

[David Kilzer (:ddkilzer)]

Created attachment 435140 [details]
Patch v1

[Radar WebKit Bug Importer]

<rdar://problem/81660796>

[Alexey Proskuryakov]

Comment on attachment 435140 [details]
Patch v1

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

> Source/WebCore/platform/KeyboardScrollingAnimator.h:54
> +    bool m_scrollTriggeringKeyIsPressed { false };

Is there a bug here, or just another false positive that we trade a little bit of perf for?

[Tim Horton]

Comment on attachment 435140 [details]
Patch v1

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

>> Source/WebCore/platform/KeyboardScrollingAnimator.h:54
>> +    bool m_scrollTriggeringKeyIsPressed { false };
> 
> Is there a bug here, or just another false positive that we trade a little bit of perf for?

I think this one is legit; we read the bit in beginKeyboardScrollGesture, which is before it's set for the first time

[Tim Horton]

(also this is all really really new and off-by default code, so it's not super surprising that there are bugs)

[David Kilzer (:ddkilzer)]

(In reply to Tim Horton from comment #5)
> Comment on attachment 435140 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435140&action=review
> 
> >> Source/WebCore/platform/KeyboardScrollingAnimator.h:54
> >> +    bool m_scrollTriggeringKeyIsPressed { false };
> > 
> > Is there a bug here, or just another false positive that we trade a little bit of perf for?
> 
> I think this one is legit; we read the bit in beginKeyboardScrollGesture,
> which is before it's set for the first time

What Tim said.  Here are a list of invalid values when I ran layout tests locally:

   6 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 115, which is not a valid value for type 'bool'
   4 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 32, which is not a valid value for type 'bool'
   3 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 6, which is not a valid value for type 'bool'
   3 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 116, which is not a valid value for type 'bool'
   2 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 97, which is not a valid value for type 'bool'
   2 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 108, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 98, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 84, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 59, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 49, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 40, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 4, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 24, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 111, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 104, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 101, which is not a valid value for type 'bool'
   1 platform/KeyboardScrollingAnimator.cpp:303:10: runtime error: load of value 100, which is not a valid value for type 'bool'

[EWS]

Committed r280764 (240349@main): <https://commits.webkit.org/240349@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435140 [details].