Skip to content

Commit b83393d

Browse files
Jeffrey YoungChromium LUCI CQ
Jeffrey Young
authored and
Chromium LUCI CQ
committed
Revert "[views] Eliminate OnWidgetClosing from NudgeWidgetObserver"
This reverts commit fe0287f. Reason for revert: MSAN failure https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/27901/overview?q=ExactID%3Aninja%3A%2F%2Fash%3Aash_unittests%2FAdaptiveChargingNudgeControllerTest.NudgeShowExactlyOnce+VHash%3Ab5761b05c7110eb2 Original change's description: > [views] Eliminate OnWidgetClosing from NudgeWidgetObserver > > Favor OnWidgetDestroying over OnWidgetClosing. OnWidgetClosing is > only called where a Widget is closed directly, it fails to catch > the case when the system destroys the NativeWidget directly. This > can result in UAFs in situations where the OS directly destroys > the widget. > > OnWidgetDestroying will be called every time a widget is destroyed > and should be used instead in most cases. > > Bug: 1240365 > Change-Id: I043635fcfcc9a52dd68b21f4f9c37539bc0d7f34 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3641295 > Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org> > Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1002735} Cq-Include-Trybots: luci.chromium.try:linux_chromium_chromeos_msan_rel_ng Bug: 1240365 Change-Id: I69348c0efca1ffdd7362881489d2ee1b9631494f No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3646362 Auto-Submit: Jeffrey Young <cowmoo@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Owners-Override: Roman Arora <romanarora@google.com> Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org> Commit-Queue: Roman Arora <romanarora@google.com> Reviewed-by: Roman Arora <romanarora@google.com> Cr-Commit-Position: refs/heads/main@{#1002911}
1 parent 7cdef80 commit b83393d

File tree

1 file changed

+8
-8
lines changed

1 file changed

+8
-8
lines changed

ash/system/power/adaptive_charging_nudge_controller_unittest.cc

+8-8
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,13 @@ class NudgeWidgetObserver : public views::WidgetObserver {
3232
widget_observation_.Observe(widget);
3333
}
3434

35-
void WaitForDestruction() {
35+
void WaitForClose() {
3636
run_loop_ = std::make_unique<base::RunLoop>();
3737
run_loop_->Run();
3838
}
3939

4040
// views::WidgetObserver:
41-
void OnWidgetDestroying(views::Widget* widget) override {
41+
void OnWidgetClosing(views::Widget* widget) override {
4242
if (run_loop_)
4343
run_loop_->Quit();
4444
}
@@ -75,8 +75,8 @@ class AdaptiveChargingNudgeControllerTest : public AshTestBase {
7575

7676
AdaptiveChargingNudgeController* GetController() { return controller_.get(); }
7777

78-
void WaitForWidgetDestruction(AdaptiveChargingNudgeController* controller,
79-
SystemNudge* nudge) {
78+
void WaitForWidgetClose(AdaptiveChargingNudgeController* controller,
79+
SystemNudge* nudge) {
8080
views::Widget* nudge_widget = nudge->widget();
8181
ASSERT_TRUE(nudge_widget);
8282
EXPECT_FALSE(nudge_widget->IsClosed());
@@ -86,12 +86,12 @@ class AdaptiveChargingNudgeControllerTest : public AshTestBase {
8686
ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
8787

8888
// Pretend the hide nudge timer has elapsed.
89-
NudgeWidgetObserver widget_destruction_observer(nudge_widget);
89+
NudgeWidgetObserver widget_close_observer(nudge_widget);
9090
controller->FireHideNudgeTimerForTesting();
9191

9292
EXPECT_TRUE(nudge_widget->GetLayer()->GetAnimator()->is_animating());
9393

94-
widget_destruction_observer.WaitForDestruction();
94+
widget_close_observer.WaitForClose();
9595
}
9696

9797
private:
@@ -109,7 +109,7 @@ TEST_F(AdaptiveChargingNudgeControllerTest, ShowsAndHidesNudge) {
109109
SystemNudge* nudge = controller->GetSystemNudgeForTesting();
110110
ASSERT_TRUE(nudge);
111111

112-
WaitForWidgetDestruction(controller, nudge);
112+
WaitForWidgetClose(controller, nudge);
113113
}
114114

115115
TEST_F(AdaptiveChargingNudgeControllerTest, NoNudgeShowForDisabledFeature) {
@@ -135,7 +135,7 @@ TEST_F(AdaptiveChargingNudgeControllerTest, NudgeShowExactlyOnce) {
135135
controller->GetNudgeDelayTimerForTesting()->FireNow();
136136
SystemNudge* nudge1 = controller->GetSystemNudgeForTesting();
137137
ASSERT_TRUE(nudge1);
138-
WaitForWidgetDestruction(controller, nudge1);
138+
WaitForWidgetClose(controller, nudge1);
139139

140140
// No nudge for the second time.
141141
controller->ShowNudge();

0 commit comments

Comments
 (0)