Bug 223903 - UBSan: AlternativeTextController::dismiss()/dismissSoon(): runtime error: load of value nnn, which is not a valid value for type 'bool'
Summary: UBSan: AlternativeTextController::dismiss()/dismissSoon(): runtime error: loa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-29 13:54 PDT by David Kilzer (:ddkilzer)
Modified: 2021-04-02 14:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 (1.79 KB, patch)
2021-03-29 14:00 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2021-03-29 13:54:53 PDT
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;
Comment 1 David Kilzer (:ddkilzer) 2021-03-29 14:00:02 PDT
Created attachment 424576 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2021-03-29 14:05:54 PDT
(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.
Comment 3 Radar WebKit Bug Importer 2021-03-29 14:08:36 PDT
<rdar://problem/75972281>
Comment 4 David Kilzer (:ddkilzer) 2021-03-29 17:16:32 PDT
(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.
Comment 5 Alexey Proskuryakov 2021-03-29 18:32:20 PDT
If it's actually timing, maybe we should mark those 46 as flaky preemptively?
Comment 6 David Kilzer (:ddkilzer) 2021-03-30 15:27:03 PDT
Comment on attachment 424576 [details]
Patch v1

Marking this for review.  No regressions found in EWS.
Comment 7 David Kilzer (:ddkilzer) 2021-03-30 15:28:57 PDT
(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.
Comment 8 David Kilzer (:ddkilzer) 2021-03-30 15:31:39 PDT
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.
Comment 9 David Kilzer (:ddkilzer) 2021-03-31 12:30:35 PDT
Review ping! :)
Comment 10 EWS 2021-04-02 14:05:00 PDT
Committed r275436: <https://commits.webkit.org/r275436>

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