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;
Created attachment 424576 [details] Patch v1
(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.
<rdar://problem/75972281>
(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.
If it's actually timing, maybe we should mark those 46 as flaky preemptively?
Comment on attachment 424576 [details] Patch v1 Marking this for review. No regressions found in EWS.
(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 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.
Review ping! :)
Committed r275436: <https://commits.webkit.org/r275436> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424576 [details].