Bug 223903

Summary: UBSan: AlternativeTextController::dismiss()/dismissSoon(): runtime error: load of value nnn, which is not a valid value for type 'bool'
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: HTML EditingAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, dbates, ews-watchlist, mifenton, ryanhaddad, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=174604
https://bugs.webkit.org/show_bug.cgi?id=176131
https://bugs.webkit.org/show_bug.cgi?id=223902
Attachments:
Description Flags
Patch v1 none

David Kilzer (:ddkilzer)
Reported 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;
Attachments
Patch v1 (1.79 KB, patch)
2021-03-29 14:00 PDT, David Kilzer (:ddkilzer) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1 2021-03-29 14:00:02 PDT
Created attachment 424576 [details] Patch v1
David Kilzer (:ddkilzer)
Comment 2 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.
Radar WebKit Bug Importer
Comment 3 2021-03-29 14:08:36 PDT
<rdar://problem/75972281>
David Kilzer (:ddkilzer)
Comment 4 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.
Alexey Proskuryakov
Comment 5 2021-03-29 18:32:20 PDT
If it's actually timing, maybe we should mark those 46 as flaky preemptively?
David Kilzer (:ddkilzer)
Comment 6 2021-03-30 15:27:03 PDT
Comment on attachment 424576 [details] Patch v1 Marking this for review. No regressions found in EWS.
David Kilzer (:ddkilzer)
Comment 7 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.
David Kilzer (:ddkilzer)
Comment 8 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.
David Kilzer (:ddkilzer)
Comment 9 2021-03-31 12:30:35 PDT
Review ping! :)
EWS
Comment 10 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].
Note You need to log in before you can comment on or make changes to this bug.