Bug 223903 – UBSan: AlternativeTextController::dismiss()/dismissSoon(): runtime error: load of value nnn, which is not a valid value for type 'bool'

• Component: HTML Editing
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Running all layout tests with a Release+UBSan build of WebKit (see Bug 176131) results in ~449 tests hitting this UBSan warning at least once with different values of "nnn":

    editing/AlternativeTextController.cpp:180:10: runtime error: load of value nnn, which is not a valid value for type 'bool'
    editing/AlternativeTextController.cpp:190:10: runtime error: load of value nnn, which is not a valid value for type 'bool'

This seems to result from AlternativeTextController::m_isActive not being initialized in the constructor.

In both cases the code that hits the UBSan runtime error is:

    if (!m_isActive)  // UBSan runtime error on line 180 or 190 of editing/AlternativeTextController.cpp.
        return;

[David Kilzer (:ddkilzer)]

Created attachment 424576 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #1)
> Created attachment 424576 [details]
> Patch for EWS

I had a lot of macOS tests (46) fail locally with this patch, but I can't tell which ones were caused by this patch vs. some other issue, so I'm using EWS to check how many tests might be failing with this change in non-UBSan builds.

I saw both "EDITING DELEGATE" changes and layout/rendering changes locally with WebKit recompiled with UBSan, which is somewhat scary.

[Radar WebKit Bug Importer]

<rdar://problem/75972281>

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #2)
> (In reply to David Kilzer (:ddkilzer) from comment #1)
> > Created attachment 424576 [details]
> > Patch for EWS
> 
> I had a lot of macOS tests (46) fail locally with this patch, but I can't
> tell which ones were caused by this patch vs. some other issue, so I'm using
> EWS to check how many tests might be failing with this change in non-UBSan
> builds.
> 
> I saw both "EDITING DELEGATE" changes and layout/rendering changes locally
> with WebKit recompiled with UBSan, which is somewhat scary.

Heh, looks like the failures were just due to UBSan altering the timing of the tests.

[Alexey Proskuryakov]

If it's actually timing, maybe we should mark those 46 as flaky preemptively?

[David Kilzer (:ddkilzer)]

Comment on attachment 424576 [details]
Patch v1

Marking this for review.  No regressions found in EWS.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #5)
> If it's actually timing, maybe we should mark those 46 as flaky preemptively?

Sorry, I already overwrote the results.  I'm going to run them again soon, so I'll take a closer look at the results and maybe suggest some suggestions.

[David Kilzer (:ddkilzer)]

Comment on attachment 424576 [details]
Patch v1

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

> Source/WebCore/editing/AlternativeTextController.h:125
> +    bool m_isActive { };
> +    bool m_isDismissedByEditing { };

Note for reviewers: only m_isActive was identified by UBSSan as being used uninitialized.

However, m_isDismissedByEditing is also uninitialized in the current constructor, so I'm making this change at the same time.

[David Kilzer (:ddkilzer)]

Review ping! :)

[EWS]

Committed r275436: <https://commits.webkit.org/r275436>

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