Automated commit: libchrome r1132882 uprev
Merge with upstream commit eb2d2f5119b1fd920ffbf3c127cbf8fad6ac9a5d
BUG=None
TEST=sudo emerge libchrome
Change-Id: I84a4b00acbe6b15918ba62b7f4ad7fbc157ab351
diff --git a/BASE_VER b/BASE_VER
index b764763..022643a 100644
--- a/BASE_VER
+++ b/BASE_VER
@@ -1 +1 @@
-1132288
+1132882
diff --git a/base/android/java/src/org/chromium/base/ResettersForTesting.java b/base/android/java/src/org/chromium/base/ResettersForTesting.java
new file mode 100644
index 0000000..ecd25b0
--- /dev/null
+++ b/base/android/java/src/org/chromium/base/ResettersForTesting.java
@@ -0,0 +1,120 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+
+/**
+ * ResettersForTesting provides functionality for reset values set for testing. This class is used
+ * directly by test runners, but lives in prod code to simplify usage.
+ *
+ * It is required to invoke {@link #register(Runnable)} whenever a method called `set*ForTesting`,
+ * such `setFooForTesting(Foo foo)` is invoked. Typical usage looks like this:
+ *
+ * <code>
+ * class MyClass {
+ * private static MyClass sInstance;
+ *
+ * public static MyClass getInstance() {
+ * if (sInstance == null) sInstance = new MyClass();
+ * return sInstance;
+ * }
+ *
+ * public static void setMyClassForTesting(MyClass myClassObj) {
+ * sInstance = myClassObj;
+ * ResettersForTesting.register(() -> sInstance = null);
+ * }
+ * }
+ * </code>
+ *
+ * This is not only used for singleton instances, but can also be used for resetting other static
+ * members.
+ *
+ * <code>
+ * class NeedsFoo {
+ * private static Foo sFooForTesting;
+ *
+ * public void doThing() {
+ * Foo foo;
+ * if (sFooForTesting != null) {
+ * foo = sFooForTesting;
+ * } else {* foo = new FooImpl();
+ * }
+ * foo.doItsThing();
+ * }
+ *
+ * public static void setFooForTesting(Foo foo) {
+ * sFooForTesting = foo;
+ * ResettersForTesting.register(() -> sFooForTesting = null);
+ * }
+ * }
+ * </code>
+ *
+ * For cases where it is important that a particular resetter runs only once, even if the
+ * `set*ForTesting` method is invoked multiple times, there is another variation that can be used.
+ * In particular, since a lambda always ends up creating a new instance in Chromium builds, we can
+ * avoid this by having a single static instance of the resetter, like this:
+ *
+ * <code>
+ * private static class NeedsFooSingleDestroy {
+ * private static final class LazyHolder {
+ * private static Foo INSTANCE = new Foo();
+ * }
+ *
+ * private static LazyHolder sFoo;
+ *
+ * private static Runnable sOneShotResetter = () -> {
+ * sFoo.INSTANCE.destroy();
+ * sFoo = new Foo();
+ * };
+ *
+ * public static void setFooForTesting(Foo foo) {
+ * sFoo.INSTANCE = foo;
+ * ResettersForTesting.register(sResetter);
+ * }
+ * }
+ * </code>
+ */
+public class ResettersForTesting {
+ // LinkedHashSet is a set that provides ordering and enables one-shot resetters to only be
+ // invoked once. For example, the following `sResetter` will only be in the set a single time.
+ // <code>
+ // private static final Runnable sResetter = () -> { ... }
+ // ...
+ // ResettersForTesting.register(sResetter);
+ // </code>
+ private static final LinkedHashSet<Runnable> sResetters = new LinkedHashSet<>();
+
+ /**
+ * Register a {@link Runnable} that will automatically execute during test tear down.
+ * @param runnable the {@link Runnable} to execute.
+ */
+ public static void register(Runnable runnable) {
+ synchronized (sResetters) {
+ sResetters.add(runnable);
+ }
+ }
+
+ /**
+ * Execute and clear all the currently registered resetters.
+ *
+ * This is not intended to be invoked manually, but is intended to be invoked by the test
+ * runners automatically during tear down.
+ */
+ public static void executeResetters() {
+ ArrayList<Runnable> resetters;
+ synchronized (sResetters) {
+ resetters = new ArrayList<>(sResetters);
+ sResetters.clear();
+ }
+
+ // Ensure that resetters are run in reverse order, enabling nesting of values as well as
+ // being more similar to C++ destruction order.
+ Collections.reverse(resetters);
+ for (Runnable resetter : resetters) resetter.run();
+ }
+}
diff --git a/base/android/jni_generator/manual_jni_registration_proguard.flags b/base/android/jni_generator/manual_jni_registration_proguard.flags
deleted file mode 100644
index 23d7120..0000000
--- a/base/android/jni_generator/manual_jni_registration_proguard.flags
+++ /dev/null
@@ -1,8 +0,0 @@
-# Copyright 2023 The Chromium Authors
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-# Unused methods must be kept for manual jni registration.
--keepclasseswithmembers,includedescriptorclasses,allowaccessmodification class !org.chromium.base.library_loader.**,** {
- native <methods>;
-}
diff --git a/base/android/junit/src/org/chromium/base/ResettersForTestingTest.java b/base/android/junit/src/org/chromium/base/ResettersForTestingTest.java
new file mode 100644
index 0000000..eb369f6
--- /dev/null
+++ b/base/android/junit/src/org/chromium/base/ResettersForTestingTest.java
@@ -0,0 +1,145 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base;
+
+import static junit.framework.Assert.assertEquals;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+
+/**
+ * Unit tests for {@link ResettersForTesting}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class ResettersForTestingTest {
+ private static class ResetsToNull {
+ public static String str;
+ public static void setStrForTesting(String newStr) {
+ str = newStr;
+ ResettersForTesting.register(() -> str = null);
+ }
+ }
+
+ private static class ResetsToOldValue {
+ public static String str;
+
+ public static void setStrForTesting(String newStr) {
+ String oldValue = str;
+ str = newStr;
+ ResettersForTesting.register(() -> str = oldValue);
+ }
+ }
+
+ private static class ResetsToNullAndIncrements {
+ public static String str;
+ public static int resetCount;
+
+ public static void setStrForTesting(String newStr) {
+ str = newStr;
+ ResettersForTesting.register(() -> {
+ str = null;
+ resetCount++;
+ });
+ }
+ }
+
+ private static class ResetsToNullAndIncrementsWithOneShotResetter {
+ public static String str;
+ public static int resetCount;
+ private static Runnable sResetter = () -> {
+ str = null;
+ resetCount++;
+ };
+
+ public static void setStrForTesting(String newStr) {
+ str = newStr;
+ ResettersForTesting.register(sResetter);
+ }
+ }
+
+ @Test
+ public void testTypicalUsage() {
+ ResetsToNull.setStrForTesting("foo");
+ assertEquals("foo", ResetsToNull.str);
+ ResettersForTesting.executeResetters();
+ Assert.assertNull(ResetsToNull.str);
+ }
+
+ @Test
+ public void testResetsToPreviousValue() {
+ // Inject a previous value to verify that we can get back to it.
+ ResetsToOldValue.str = "bar";
+
+ ResetsToOldValue.setStrForTesting("foo");
+ assertEquals("foo", ResetsToOldValue.str);
+
+ // After resetting the value, it should be back to the first value.
+ ResettersForTesting.executeResetters();
+ assertEquals("bar", ResetsToOldValue.str);
+ }
+
+ @Test
+ public void testMultipleResets() {
+ // Inject an outer value to verify we can get back to this.
+ ResetsToOldValue.str = "qux";
+
+ // Then set the next value.
+ ResetsToOldValue.setStrForTesting("foo");
+ assertEquals("foo", ResetsToOldValue.str);
+
+ // Now, next that value into another one to ensure the unwinding works.
+ ResetsToOldValue.setStrForTesting("bar");
+ assertEquals("bar", ResetsToOldValue.str);
+
+ // Since we are invoking the resetters in the reverse order, we should now be back to start.
+ ResettersForTesting.executeResetters();
+ assertEquals("qux", ResetsToOldValue.str);
+ }
+
+ @Test
+ public void testResettersExecutedOnlyOnce() {
+ // Force set this to 0 for this particular test.
+ ResetsToNullAndIncrements.resetCount = 0;
+ ResetsToNullAndIncrements.str = null;
+
+ // Set the initial value and register the resetter.
+ ResetsToNullAndIncrements.setStrForTesting("some value");
+ assertEquals("some value", ResetsToNullAndIncrements.str);
+
+ // Now, execute all resetters and ensure it's only executed once.
+ ResettersForTesting.executeResetters();
+ assertEquals(1, ResetsToNullAndIncrements.resetCount);
+
+ // Execute the resetters again, and verify it does not invoke the same resetter again.
+ ResettersForTesting.executeResetters();
+ assertEquals(1, ResetsToNullAndIncrements.resetCount);
+ }
+
+ @Test
+ public void testResettersExecutedOnlyOnceForOneShotResetters() {
+ // Force set this to 0 for this particular test.
+ ResetsToNullAndIncrementsWithOneShotResetter.resetCount = 0;
+ ResetsToNullAndIncrementsWithOneShotResetter.str = null;
+
+ // Set the initial value and register the resetter twice.
+ ResetsToNullAndIncrementsWithOneShotResetter.setStrForTesting("some value");
+ ResetsToNullAndIncrementsWithOneShotResetter.setStrForTesting("some other value");
+ assertEquals("some other value", ResetsToNullAndIncrementsWithOneShotResetter.str);
+
+ // Now, execute all resetters and ensure it's only executed once, since it is a single
+ // instance of the same resetter.
+ ResettersForTesting.executeResetters();
+ assertEquals(1, ResetsToNullAndIncrementsWithOneShotResetter.resetCount);
+
+ // Execute the resetters again, and verify it does not invoke the same resetter again.
+ ResettersForTesting.executeResetters();
+ assertEquals(1, ResetsToNullAndIncrementsWithOneShotResetter.resetCount);
+ }
+}
diff --git a/base/android/proguard/chromium_code.flags b/base/android/proguard/chromium_code.flags
index b8ee9ea..5f5bb55 100644
--- a/base/android/proguard/chromium_code.flags
+++ b/base/android/proguard/chromium_code.flags
@@ -13,8 +13,12 @@
@androidx.annotation.Keep <methods>;
}
-# Allow unused native methods to be removed, but prevent renaming on those that are kept.
--keepclasseswithmembernames,includedescriptorclasses,allowaccessmodification class ** {
+# Even unused methods kept due to explicit jni registration:
+# https://crbug.com/688465.
+-keepclasseswithmembers,includedescriptorclasses,allowaccessmodification class !org.chromium.base.library_loader.**,** {
+ native <methods>;
+}
+-keepclasseswithmembernames,includedescriptorclasses,allowaccessmodification class org.chromium.base.library_loader.** {
native <methods>;
}
diff --git a/base/containers/span.h b/base/containers/span.h
index 65b392b..ce3e017 100644
--- a/base/containers/span.h
+++ b/base/containers/span.h
@@ -6,6 +6,7 @@
#define BASE_CONTAINERS_SPAN_H_
#include <stddef.h>
+#include <stdint.h>
#include <array>
#include <iterator>
diff --git a/base/message_loop/fd_watch_controller_posix_unittest.cc b/base/message_loop/fd_watch_controller_posix_unittest.cc
index 766f392..f937ca9 100644
--- a/base/message_loop/fd_watch_controller_posix_unittest.cc
+++ b/base/message_loop/fd_watch_controller_posix_unittest.cc
@@ -131,10 +131,10 @@
TEST_F(FdWatchControllerPosixTest, FileDescriptorWatcherOutlivesMessageLoop) {
// Simulate a MessageLoop that dies before an FileDescriptorWatcher.
// This could happen when people use the Singleton pattern or atexit.
+ TestHandler handler;
// Arrange for watcher to live longer than message loop.
MessagePumpForIO::FdWatchController watcher(FROM_HERE);
- TestHandler handler;
{
test::TaskEnvironment env(test::TaskEnvironment::MainThreadType::IO);
diff --git a/base/message_loop/message_pump_glib.cc b/base/message_loop/message_pump_glib.cc
index 7c046dc..dfc8a95 100644
--- a/base/message_loop/message_pump_glib.cc
+++ b/base/message_loop/message_pump_glib.cc
@@ -103,6 +103,153 @@
// we run DoWork. That part will also run in the other event pumps.
// - We also run DoWork, and possibly DoIdleWork, in the main loop,
// around event handling.
+//
+// ---------------------------------------------------------------------------
+//
+// An overview on the way that we track work items:
+//
+// ScopedDoWorkItems are used by this pump to track native work. They are
+// stored by value in |state_| and are set/cleared as the pump runs. Their
+// setting and clearing is done in the functions
+// {Set,Clear,EnsureSet,EnsureCleared}ScopedWorkItem. Control flow in GLib is
+// quite non-obvious because chrome is not notified when a nested loop is
+// entered/exited. To detect nested loops, MessagePumpGlib uses
+// |state_->do_work_depth| which is incremented when DoWork is entered, and a
+// GLib library function, g_main_depth(), which indicates the current number of
+// Dispatch() calls on the stack. To react to them, two separate
+// ScopedDoWorkItems are used (a standard one used for all native work, and a
+// second one used exclusively for forcing nesting when there is a native loop
+// spinning). Note that `ThreadController` flags all nesting as
+// `Phase::kNested` so separating native and application work while nested isn't
+// supported nor a goal.
+//
+// It should also be noted that a second GSource has been added to GLib,
+// referred to as the "observer" source. It is used because in the case where
+// native work occurs on wakeup that is higher priority than Chrome (all of
+// GTK), chrome won't even get notified that the pump is awake.
+//
+// There are several cases to consider wrt. nesting level and order. In
+// order, we have:
+// A. [root] -> MessagePump::Run() -> native event -> g_main_context_iteration
+// B. [root] -> MessagePump::Run() -> DoWork -> g_main_context_iteration
+// C. [root] -> native -> DoWork -> MessagePump -> [...]
+// The second two cases are identical for our purposes, and the last one turns
+// out to be handled without any extra headache.
+//
+// Consider nesting case A, where native work is called from
+// |g_main_context_iteration()| from the pump, and that native work spins up a
+// loop. For our purposes, this is a nested loop, because control is not
+// returned to the pump once one iteration of the pump is complete. In this
+// case, the pump needs to enter nesting without DoWork being involved at
+// all. This is accomplished using |MessagePumpGlib::NestIfRequired()|, which is
+// called during the Prepare() phase of GLib. As the pump records state on entry
+// and exit from GLib using |OnEntryToGlib| and |OnExitFromGlib|, we can compare
+// |g_main_depth| at |HandlePrepare| with the one before we entered
+// |g_main_context_iteration|. If it is higher, there is a native loop being
+// spun, and |RegisterNesting| is called, forcing nesting by initializing two
+// work items at once. These are destroyed after the exit from
+// |g_main_context_iteration| using |OnExitFromGlib|.
+//
+// Then, considering nesting case B, |state_->do_work_depth| is incremented
+// during any Chrome work, to allow the pump to detect re-entrancy during a
+// chrome work item. This is required because `g_main_depth` is not incremented
+// in any `DoWork` call not occuring during `Dispatch()` (i.e. during
+// `MessagePumpGlib::Run()`). In this case, a nested loop is recorded, and the
+// pump sets-and-clears scoped work items during Prepare, Check, and Dispatch. A
+// work item can never be active when control flow returns to GLib (i.e. on
+// return) during a nested loop, because the nested loop could exit at any
+// point. This is fine because TimeKeeper is only concerned with the fact that a
+// nested loop is in progress, as opposed to the various phases of the nested
+// loop.
+//
+// Finally, consider nesting case C, where a native loop is spinning
+// entirely outside of Chrome, such as inside a signal handler, the pump might
+// create and destroy DoWorkItems during Prepare() and Check(), but these work
+// items will always get cleared during Dispatch(), before the pump enters a
+// DoWork(), leading to the pump showing non-nested native work without the
+// thread controller being active, the correct situation (which won't occur
+// outside of startup or shutdown). Once Dispatch() is called, the pump's
+// nesting tracking works correctly, as state_->do_work_depth is increased, and
+// upon re-entrancy we detect the nested loop, which is correct, as this is the
+// only point at which the loop actually becomes "nested".
+//
+// -----------------------------------------------------------------------------
+//
+// As an overview of the steps taken by MessagePumpGLib to ensure that nested
+// loops are detected adequately during each phase of the GLib loop:
+//
+// 0: Before entering GLib:
+// 0.1: Record state about current state of GLib (g_main_depth()) for
+// case 1.1.2.
+//
+// 1: Prepare.
+// 1.1: Detection of nested loops
+
+// 1.1.1: If |state_->do_work_depth| > 0, we are in nesting case B detailed
+// above. A work item must be newly created during this function to
+// trigger nesting, and is destroyed to ensure proper destruction order
+// in the case where GLib quits after Prepare().
+//
+// 1.1.2: Otherwise, check if we are in nesting case A above. If yes, trigger
+// nesting using ScopedDoWorkItems. The nesting will be cleared at exit
+// from GLib.
+//
+// This check occurs only in |HandleObserverPrepare|, not in
+// |HandlePrepare|.
+//
+// A third party is running a glib message loop. Since Chrome work is
+// registered with GLib at |G_PRIORITY_DEFAULT_IDLE|, a relatively low
+// priority, sources of default-or-higher priority will be Dispatch()ed
+// first. Since only one source is Dispatched per loop iteration,
+// |HandlePrepare| can get called several times in a row in the case that
+// there are any other events in the queue. A ScopedDoWorkItem is created
+// and destroyed to record this. That work item triggers nesting.
+//
+// 1.2: Other considerations
+// 1.2.1: Sleep occurs between Prepare() and Check(). If Chrome will pass a
+// nonzero poll time to GLib, the inner ScopedDoWorkItem is cleared and
+// BeforeWait() is called. In nesting case A, the nesting work item will
+// not be cleared. A nested loop will typically not block.
+//
+// Since Prepare() is called before Check() in all cases, the bulk of
+// nesting detection is done in Prepare().
+//
+// 2: Check.
+// 2.1: Detection of nested loops:
+// 2.1.1: In nesting case B, |ClearScopedWorkItem()| on exit. A third party is
+// running a glib message loop. It is possible that at any point the
+// nested message loop will quit. In this case, we don't want to leave a
+// nested DoWorkItem on the stack.
+//
+// 2.2: Other considerations
+// 2.2.1: A ScopedDoWorkItem may be created (if it was not already present) at
+// the entry to Check() to record a wakeup in the case that the pump
+// slept. It is important to note that this occurs both in
+// |HandleObserverCheck| and |HandleCheck| to ensure that at every point
+// as the pump enters the Dispatch phase it is awake. In the case it is
+// already awake, this is a very cheap operation.
+//
+// 3: Dispatch
+// 3.1 Detection of nested loops
+// 3.1.1: |state_->do_work_depth| is incremented on entry and decremented on
+// exit. This is used to detect nesting case B.
+//
+// 3.1.2: Nested loops can be quit at any point, and so ScopedDoWorkItems can't
+// be left on the stack for the same reasons as in 1.1.1/2.1.1.
+//
+// 3.2 Other considerations
+// 3.2.1: Since DoWork creates its own work items, ScopedDoWorkItems are not
+// used as this would trigger nesting in all cases.
+//
+// 4: Post GLib
+// 4.1: Detection of nested loops
+// 4.1.1: |state_->do_work_depth| is also increased during the DoWork in Run()
+// as nesting in that case [calling glib from third party code] needs to
+// clear all work items after return to avoid improper destruction order.
+//
+// 4.2: Other considerations:
+// 4.2.1: DoWork uses its own work item, so no ScopedDoWorkItems are active in
+// this case.
struct WorkSource : public GSource {
raw_ptr<MessagePumpGlib> pump;
@@ -130,8 +277,28 @@
}
// I wish these could be const, but g_source_new wants non-const.
-GSourceFuncs WorkSourceFuncs = {WorkSourcePrepare, WorkSourceCheck,
- WorkSourceDispatch, nullptr};
+GSourceFuncs g_work_source_funcs = {WorkSourcePrepare, WorkSourceCheck,
+ WorkSourceDispatch, nullptr};
+
+struct ObserverSource : public GSource {
+ raw_ptr<MessagePumpGlib> pump;
+};
+
+gboolean ObserverPrepare(GSource* gsource, gint* timeout_ms) {
+ auto* source = static_cast<ObserverSource*>(gsource);
+ source->pump->HandleObserverPrepare();
+ *timeout_ms = -1;
+ // We always want to poll.
+ return FALSE;
+}
+
+gboolean ObserverCheck(GSource* gsource) {
+ auto* source = static_cast<ObserverSource*>(gsource);
+ return source->pump->HandleObserverCheck();
+}
+
+GSourceFuncs g_observer_funcs = {ObserverPrepare, ObserverCheck, nullptr,
+ nullptr};
struct FdWatchSource : public GSource {
raw_ptr<MessagePumpGlib> pump;
@@ -171,6 +338,25 @@
// Used to flag that the current Run() invocation should return ASAP.
bool should_quit = false;
+ // Keeps track of the number of calls to DoWork() on the stack for the current
+ // Run() invocation. Used to detect reentrancy from DoWork in order to make
+ // decisions about tracking nested work.
+ int do_work_depth = 0;
+
+ // Value of g_main_depth() captured before the call to
+ // g_main_context_iteration() in Run(). nullopt if Run() is not calling
+ // g_main_context_iteration(). Used to track whether the pump has forced a
+ // nested state due to a native pump.
+ absl::optional<int> g_depth_on_iteration;
+
+ // Used to keep track of the native event work items processed by the message
+ // pump.
+ Delegate::ScopedDoWorkItem scoped_do_work_item;
+
+ // Used to force the pump into a nested state when a native runloop was
+ // dispatched from main.
+ Delegate::ScopedDoWorkItem native_loop_do_work_item;
+
// The information of the next task available at this run-level. Stored in
// RunState because different set of tasks can be accessible at various
// run-levels (e.g. non-nestable tasks).
@@ -199,8 +385,13 @@
wakeup_gpollfd_->fd = wakeup_pipe_read_;
wakeup_gpollfd_->events = G_IO_IN;
+ observer_source_ = std::unique_ptr<GSource, GSourceDeleter>(
+ g_source_new(&g_observer_funcs, sizeof(ObserverSource)));
+ static_cast<ObserverSource*>(observer_source_.get())->pump = this;
+ g_source_attach(observer_source_.get(), context_);
+
work_source_ = std::unique_ptr<GSource, GSourceDeleter>(
- g_source_new(&WorkSourceFuncs, sizeof(WorkSource)));
+ g_source_new(&g_work_source_funcs, sizeof(WorkSource)));
static_cast<WorkSource*>(work_source_.get())->pump = this;
g_source_add_poll(work_source_.get(), wakeup_gpollfd_.get());
g_source_set_priority(work_source_.get(), kPriorityWork);
@@ -327,19 +518,74 @@
return controller->Attach(this);
}
+void MessagePumpGlib::HandleObserverPrepare() {
+ // |state_| may be null during tests.
+ if (!state_) {
+ return;
+ }
+
+ if (state_->do_work_depth > 0) {
+ // Contingency 1.1.1 detailed above
+ SetScopedWorkItem();
+ ClearScopedWorkItem();
+ } else {
+ // Contingency 1.1.2 detailed above
+ NestIfRequired();
+ }
+
+ return;
+}
+
+bool MessagePumpGlib::HandleObserverCheck() {
+ // |state_| may be null in tests.
+ if (!state_) {
+ return FALSE;
+ }
+
+ // Make sure we record the fact that we're awake. Chrome won't get Check()ed
+ // if a higher priority work item returns TRUE from Check().
+ EnsureSetScopedWorkItem();
+ if (state_->do_work_depth > 0) {
+ // Contingency 2.1.1
+ ClearScopedWorkItem();
+ }
+
+ // The observer never needs to run anything.
+ return FALSE;
+}
+
// Return the timeout we want passed to poll.
int MessagePumpGlib::HandlePrepare() {
// |state_| may be null during tests.
if (!state_)
return 0;
- return GetTimeIntervalMilliseconds(state_->next_work_info.delayed_run_time);
+ const int next_wakeup_millis =
+ GetTimeIntervalMilliseconds(state_->next_work_info.delayed_run_time);
+ if (next_wakeup_millis != 0) {
+ // When this is called, it is not possible to know for sure if a
+ // ScopedWorkItem is on the stack, because HandleObserverCheck may have set
+ // it during an iteration of the pump where a high priority native work item
+ // executed.
+ EnsureClearedScopedWorkItem();
+ state_->delegate->BeforeWait();
+ }
+
+ return next_wakeup_millis;
}
bool MessagePumpGlib::HandleCheck() {
if (!state_) // state_ may be null during tests.
return false;
+ // Ensure pump is awake.
+ EnsureSetScopedWorkItem();
+
+ if (state_->do_work_depth > 0) {
+ // Contingency 2.1.1
+ ClearScopedWorkItem();
+ }
+
// We usually have a single message on the wakeup pipe, since we are only
// signaled when the queue went from empty to non-empty, but there can be
// two messages if a task posted a task, hence we read at most two bytes.
@@ -371,7 +617,18 @@
}
void MessagePumpGlib::HandleDispatch() {
+ // Contingency 3.2.1
+ EnsureClearedScopedWorkItem();
+
+ // Contingency 3.1.1
+ ++state_->do_work_depth;
state_->next_work_info = state_->delegate->DoWork();
+ --state_->do_work_depth;
+
+ if (state_ && state_->do_work_depth > 0) {
+ // Contingency 3.1.2
+ EnsureClearedScopedWorkItem();
+ }
}
void MessagePumpGlib::Run(Delegate* delegate) {
@@ -391,14 +648,28 @@
// callbacks. This is so we only quit our own loops, and we don't quit
// nested loops run by others. TODO(deanm): Is this what we want?
for (;;) {
+ // ScopedWorkItem to account for any native work until the runloop starts
+ // running chrome work.
+ SetScopedWorkItem();
+
// Don't block if we think we have more work to do.
bool block = !more_work_is_plausible;
+ OnEntryToGlib();
more_work_is_plausible = g_main_context_iteration(context_, block);
+ OnExitFromGlib();
+
if (state_->should_quit)
break;
+ // Contingency 4.2.1
+ EnsureClearedScopedWorkItem();
+
+ // Contingency 4.1.1
+ ++state_->do_work_depth;
state_->next_work_info = state_->delegate->DoWork();
+ --state_->do_work_depth;
+
more_work_is_plausible |= state_->next_work_info.is_immediate();
if (state_->should_quit)
break;
@@ -471,4 +742,128 @@
return state_->should_quit;
}
+void MessagePumpGlib::SetScopedWorkItem() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ // If there exists a ScopedDoWorkItem in the current RunState, it cannot be
+ // overwritten.
+ CHECK(state_->scoped_do_work_item.IsNull());
+
+ // In the case that we're more than two work items deep, don't bother tracking
+ // individual native events anymore. Note that this won't cause out-of-order
+ // end work items, because the work item is cleared before entering the second
+ // DoWork().
+ if (state_->do_work_depth < 2) {
+ state_->scoped_do_work_item = state_->delegate->BeginWorkItem();
+ }
+}
+
+void MessagePumpGlib::ClearScopedWorkItem() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+
+ CHECK(!state_->scoped_do_work_item.IsNull());
+ // See identical check in SetScopedWorkItem
+ if (state_->do_work_depth < 2) {
+ state_->scoped_do_work_item = Delegate::ScopedDoWorkItem();
+ }
+}
+
+void MessagePumpGlib::EnsureSetScopedWorkItem() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ if (state_->scoped_do_work_item.IsNull()) {
+ SetScopedWorkItem();
+ }
+}
+
+void MessagePumpGlib::EnsureClearedScopedWorkItem() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ if (!state_->scoped_do_work_item.IsNull()) {
+ ClearScopedWorkItem();
+ }
+}
+
+void MessagePumpGlib::RegisterNested() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ CHECK(state_->native_loop_do_work_item.IsNull());
+
+ // Transfer `scoped_do_work_item` to `native_do_work_item`, and so the
+ // ephemeral `scoped_do_work_item` will be coming in and out of existence on
+ // top of `native_do_work_item`, whose state hasn't been deleted.
+
+ if (state_->scoped_do_work_item.IsNull()) {
+ state_->native_loop_do_work_item = state_->delegate->BeginWorkItem();
+ } else {
+ // This clears state_->scoped_do_work_item.
+ state_->native_loop_do_work_item = std::move(state_->scoped_do_work_item);
+ }
+ SetScopedWorkItem();
+ ClearScopedWorkItem();
+}
+
+void MessagePumpGlib::UnregisterNested() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ CHECK(!state_->native_loop_do_work_item.IsNull());
+
+ EnsureClearedScopedWorkItem();
+ // Nesting exits here.
+ state_->native_loop_do_work_item = Delegate::ScopedDoWorkItem();
+}
+
+void MessagePumpGlib::NestIfRequired() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ if (state_->native_loop_do_work_item.IsNull() &&
+ state_->g_depth_on_iteration.has_value() &&
+ g_main_depth() != state_->g_depth_on_iteration.value()) {
+ RegisterNested();
+ }
+}
+
+void MessagePumpGlib::UnnestIfRequired() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ if (!state_->native_loop_do_work_item.IsNull()) {
+ UnregisterNested();
+ }
+}
+
+void MessagePumpGlib::OnEntryToGlib() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ CHECK(!state_->g_depth_on_iteration.has_value());
+ state_->g_depth_on_iteration.emplace(g_main_depth());
+}
+
+void MessagePumpGlib::OnExitFromGlib() {
+ // |state_| can be null during tests
+ if (!state_) {
+ return;
+ }
+ state_->g_depth_on_iteration.reset();
+ UnnestIfRequired();
+}
+
} // namespace base
diff --git a/base/message_loop/message_pump_glib.h b/base/message_loop/message_pump_glib.h
index 1b0fcce..647f3cc 100644
--- a/base/message_loop/message_pump_glib.h
+++ b/base/message_loop/message_pump_glib.h
@@ -93,6 +93,12 @@
bool HandleCheck();
void HandleDispatch();
+ // Very similar to the above, with the key difference that these functions are
+ // only used to track work items and never indicate work is available, and
+ // poll indefinitely.
+ void HandleObserverPrepare();
+ bool HandleObserverCheck();
+
// Overridden from MessagePump:
void Run(Delegate* delegate) override;
void Quit() override;
@@ -131,6 +137,38 @@
raw_ptr<RunState> state_;
+ // Starts tracking a new work item and stores a `ScopedDoWorkItem` in
+ // `state_`.
+ void SetScopedWorkItem();
+ // Gets rid of the current scoped work item.
+ void ClearScopedWorkItem();
+ // Ensures there's a ScopedDoWorkItem at the current run-level. This can be
+ // useful for contexts where the caller can't tell whether they just woke up
+ // or are continuing from native work.
+ void EnsureSetScopedWorkItem();
+ // Ensures there's no ScopedDoWorkItem at the current run-level. This can be
+ // useful in contexts where the caller knows that a sleep is imminent but
+ // doesn't know if the current context captures ongoing work (back from
+ // native).
+ void EnsureClearedScopedWorkItem();
+
+ // Called before entrance to g_main_context_iteration to record context
+ // related to nesting depth to track native nested loops which would otherwise
+ // be invisible.
+ void OnEntryToGlib();
+ // Cleans up state set in OnEntryToGlib.
+ void OnExitFromGlib();
+ // Forces the pump into a nested state by creating two work items back to
+ // back.
+ void RegisterNested();
+ // Removes all of the pump's ScopedDoWorkItems to remove the state of nesting
+ // which was forced onto the pump.
+ void UnregisterNested();
+ // Nest if pump is not already marked as nested.
+ void NestIfRequired();
+ // Remove the nesting if the pump is nested.
+ void UnnestIfRequired();
+
std::unique_ptr<GMainContext, GMainContextDeleter> owned_context_;
// This is a GLib structure that we can add event sources to. On the main
// thread, we use the default GLib context, which is the one to which all GTK
@@ -141,6 +179,10 @@
// the message pump is destroyed.
std::unique_ptr<GSource, GSourceDeleter> work_source_;
+ // The observer source. It is shared by all calls to Run and destroyed when
+ // the message pump is destroyed.
+ std::unique_ptr<GSource, GSourceDeleter> observer_source_;
+
// We use a wakeup pipe to make sure we'll get out of the glib polling phase
// when another thread has scheduled us to do some work. There is a glib
// mechanism g_main_context_wakeup, but this won't guarantee that our event's
diff --git a/base/message_loop/message_pump_glib_unittest.cc b/base/message_loop/message_pump_glib_unittest.cc
index b202b40..7b5406e 100644
--- a/base/message_loop/message_pump_glib_unittest.cc
+++ b/base/message_loop/message_pump_glib_unittest.cc
@@ -6,6 +6,7 @@
#include <glib.h>
#include <math.h>
+#include "build/build_config.h"
#include <algorithm>
#include <vector>
@@ -27,6 +28,7 @@
#include "base/task/single_thread_task_executor.h"
#include "base/task/single_thread_task_runner.h"
#include "base/test/task_environment.h"
+#include "base/test/trace_event_analyzer.h"
#include "base/threading/thread.h"
#include "testing/gtest/include/gtest/gtest.h"
@@ -540,6 +542,75 @@
run_loop.Run();
}
+namespace {
+
+class NestedEventAnalyzer {
+ public:
+ NestedEventAnalyzer() {
+ trace_analyzer::Start(TRACE_DISABLED_BY_DEFAULT("base"));
+ }
+
+ size_t CountEvents() {
+ std::unique_ptr<trace_analyzer::TraceAnalyzer> analyzer =
+ trace_analyzer::Stop();
+ trace_analyzer::TraceEventVector events;
+ return analyzer->FindEvents(trace_analyzer::Query::EventName() ==
+ trace_analyzer::Query::String("Nested"),
+ &events);
+ }
+};
+
+} // namespace
+
+// TODO(crbug.com/1434860): Re-enable this test
+#if BUILDFLAG(IS_LINUX) && defined(MEMORY_SANITIZER)
+#define MAYBE_TestNativeNestedLoopWithoutDoWork \
+ DISABLED_TestNativeNestedLoopWithoutDoWork
+#else
+#define MAYBE_TestNativeNestedLoopWithoutDoWork \
+ TestNativeNestedLoopWithoutDoWork
+#endif
+TEST_F(MessagePumpGLibTest, MAYBE_TestNativeNestedLoopWithoutDoWork) {
+ // Tests that nesting is triggered correctly if a message loop is run
+ // from a native event (gtk event) outside of a work item (not in a posted
+ // task).
+
+ RunLoop run_loop;
+ NestedEventAnalyzer analyzer;
+
+ base::CurrentThread::Get()->EnableMessagePumpTimeKeeperMetrics(
+ "GlibMainLoopTest");
+
+ scoped_refptr<GLibLoopRunner> runner = base::MakeRefCounted<GLibLoopRunner>();
+ injector()->AddEvent(
+ 0,
+ BindOnce(
+ [](EventInjector* injector, scoped_refptr<GLibLoopRunner> runner,
+ OnceClosure done) {
+ CurrentThread::ScopedAllowApplicationTasksInNativeNestedLoop allow;
+ runner->RunLoop();
+ },
+ Unretained(injector()), runner, run_loop.QuitClosure()));
+
+ injector()->AddDummyEvent(0);
+ injector()->AddDummyEvent(0);
+ injector()->AddDummyEvent(0);
+
+ SingleThreadTaskRunner::GetCurrentDefault()->PostDelayedTask(
+ FROM_HERE, BindOnce(&GLibLoopRunner::Quit, runner), Milliseconds(40));
+
+ SingleThreadTaskRunner::GetCurrentDefault()->PostDelayedTask(
+ FROM_HERE, run_loop.QuitClosure(), Milliseconds(40));
+
+ run_loop.Run();
+
+ // It would be expected that there be one single event, but it seems like this
+ // is counting the Begin/End of the Nested trace event. Each of the two events
+ // found are of duration 0 with distinct timestamps. It has also been
+ // confirmed that nesting occurs only once.
+ CHECK_EQ(analyzer.CountEvents(), 2ul);
+}
+
// Tests for WatchFileDescriptor API
class MessagePumpGLibFdWatchTest : public testing::Test {
protected:
diff --git a/base/message_loop/message_pump_libevent.h b/base/message_loop/message_pump_libevent.h
index 4017d93..0389de9 100644
--- a/base/message_loop/message_pump_libevent.h
+++ b/base/message_loop/message_pump_libevent.h
@@ -162,7 +162,10 @@
// State used only with libevent
std::unique_ptr<event> event_;
- raw_ptr<MessagePumpLibevent> libevent_pump_ = nullptr;
+
+ // Tests (e.g. FdWatchControllerPosixTest) deliberately make this dangle.
+ raw_ptr<MessagePumpLibevent, DisableDanglingPtrDetection> libevent_pump_ =
+ nullptr;
// State used only with epoll
WeakPtr<MessagePumpEpoll> epoll_pump_;
diff --git a/base/message_loop/message_pump_unittest.cc b/base/message_loop/message_pump_unittest.cc
index 9e19703..b11167b 100644
--- a/base/message_loop/message_pump_unittest.cc
+++ b/base/message_loop/message_pump_unittest.cc
@@ -139,6 +139,13 @@
MessagePumpTest() : message_pump_(MessagePump::Create(GetParam())) {}
protected:
+#if defined(USE_GLIB)
+ // Because of a GLIB implementation quirk, the pump doesn't do the same things
+ // between each DoWork. In this case, it won't set/clear a ScopedDoWorkItem
+ // because we run a chrome work item in the runloop outside of GLIB's control,
+ // so we oscillate between setting and not setting PreDoWorkExpectations.
+ std::map<MessagePump::Delegate*, int> do_work_counts;
+#endif
void AddPreDoWorkExpectations(
testing::StrictMock<MockMessagePumpDelegate>& delegate) {
#if BUILDFLAG(IS_WIN)
@@ -155,14 +162,35 @@
EXPECT_CALL(delegate, MockOnEndWorkItem).Times(AtMost(1));
}
#endif // BUILDFLAG(IS_WIN)
+#if defined(USE_GLIB)
+ do_work_counts.try_emplace(&delegate, 0);
+ if (GetParam() == MessagePumpType::UI) {
+ if (++do_work_counts[&delegate] % 2) {
+ // The GLib MessagePump will do native work before chrome work on
+ // startup.
+ EXPECT_CALL(delegate, MockOnBeginWorkItem);
+ EXPECT_CALL(delegate, MockOnEndWorkItem);
+ }
+ }
+#endif // defined(USE_GLIB)
}
void AddPostDoWorkExpectations(
testing::StrictMock<MockMessagePumpDelegate>& delegate) {
+#if BUILDFLAG(IS_POSIX) && !BUILDFLAG(IS_NACL)
// MessagePumpLibEvent checks for native notifications once after processing
// a DoWork() but only instantiates a ScopedDoWorkItem that triggers
// MessagePumpLibevent::OnLibeventNotification() which this test does not
// so there are no post-work expectations at the moment.
+#endif
+#if defined(USE_GLIB)
+ if (GetParam() == MessagePumpType::UI) {
+ // The GLib MessagePump can create and destroy work items between DoWorks
+ // depending on internal state.
+ EXPECT_CALL(delegate, MockOnBeginWorkItem).Times(AtMost(1));
+ EXPECT_CALL(delegate, MockOnEndWorkItem).Times(AtMost(1));
+ }
+#endif // defined(USE_GLIB)
}
std::unique_ptr<MessagePump> message_pump_;
@@ -182,6 +210,16 @@
message_pump_->Quit();
return MessagePump::Delegate::NextWorkInfo{TimeTicks::Max()};
}));
+
+ // MessagePumpGlib uses a work item between a HandleDispatch() call and
+ // passing control back to the chrome loop, which handles the Quit() despite
+ // us not necessarily doing any native work during that time.
+#if defined(USE_GLIB)
+ if (GetParam() == MessagePumpType::UI) {
+ AddPostDoWorkExpectations(delegate);
+ }
+#endif
+
EXPECT_CALL(delegate, DoIdleWork()).Times(0);
message_pump_->ScheduleWork();
@@ -215,10 +253,14 @@
return MessagePump::Delegate::NextWorkInfo{TimeTicks::Max()};
}));
- // PostDoWorkExpectations for the first DoWork.
+ // The `nested_delegate` will quit first.
+ AddPostDoWorkExpectations(nested_delegate);
+
+ // Return a delayed task with |yield_to_native| set, and exit.
AddPostDoWorkExpectations(delegate);
AddPreDoWorkExpectations(delegate);
+
EXPECT_CALL(delegate, DoWork).WillOnce(Invoke([this] {
message_pump_->Quit();
return MessagePump::Delegate::NextWorkInfo{TimeTicks::Max()};
@@ -245,8 +287,8 @@
}));
AddPostDoWorkExpectations(delegate);
- // Return a delayed task with |yield_to_native| set, and exit.
AddPreDoWorkExpectations(delegate);
+ // Return a delayed task with |yield_to_native| set, and exit.
EXPECT_CALL(delegate, DoWork).WillOnce(Invoke([this] {
message_pump_->Quit();
auto now = TimeTicks::Now();
@@ -357,9 +399,13 @@
message_pump_->Quit();
return MessagePump::Delegate::NextWorkInfo{TimeTicks::Max()};
}));
+
+ AddPostDoWorkExpectations(delegate);
+
#if BUILDFLAG(IS_IOS)
EXPECT_CALL(delegate, DoIdleWork).Times(AnyNumber());
#endif
+
message_pump_->Run(&delegate);
}
@@ -383,6 +429,11 @@
return MessagePump::Delegate::NextWorkInfo{TimeTicks::Max()};
}));
+ // We quit `nested_delegate` before `delegate`
+ AddPostDoWorkExpectations(nested_delegate);
+
+ AddPostDoWorkExpectations(delegate);
+
#if BUILDFLAG(IS_IOS)
EXPECT_CALL(nested_delegate, DoIdleWork).Times(AnyNumber());
EXPECT_CALL(delegate, DoIdleWork).Times(AnyNumber());
diff --git a/base/profiler/stack_sampler.cc b/base/profiler/stack_sampler.cc
index 65d3eb9..b1659b1 100644
--- a/base/profiler/stack_sampler.cc
+++ b/base/profiler/stack_sampler.cc
@@ -150,8 +150,15 @@
if ((++stack_size_histogram_sampling_counter_ %
kUMAHistogramDownsampleAmount) == 0) {
// Record the size of the stack to tune kLargeStackSize.
- UmaHistogramMemoryKB("Memory.StackSamplingProfiler.StackSampleSize",
- saturated_cast<int>(stack_size / kBytesPerKilobyte));
+ // UmaHistogramMemoryKB has a min of 1000, which isn't useful for our
+ // purposes, so call UmaHistogramCustomCounts directly.
+ // Min is 4KB, since that's the normal pagesize and setting kLargeStackSize
+ // smaller than that would be pointless. Max is 8MB since that's the
+ // current ChromeOS stack size; we shouldn't be able to get a number
+ // larger than that.
+ UmaHistogramCustomCounts(
+ "Memory.StackSamplingProfiler.StackSampleSize2",
+ saturated_cast<int>(stack_size / kBytesPerKilobyte), 4, 8 * 1024, 50);
}
// We expect to very rarely see stacks larger than kLargeStackSize. If we see
diff --git a/base/profiler/stack_sampler.h b/base/profiler/stack_sampler.h
index 9622453..d6154ce 100644
--- a/base/profiler/stack_sampler.h
+++ b/base/profiler/stack_sampler.h
@@ -94,7 +94,7 @@
StackSamplerTestDelegate* test_delegate = nullptr);
#if BUILDFLAG(IS_CHROMEOS)
- // How often to record the "Memory.StackSamplingProfiler.StackSampleSize" UMA
+ // How often to record the "Memory.StackSamplingProfiler.StackSampleSize2" UMA
// histogram. Specifically, only 1 in kUMAHistogramDownsampleAmount calls to
// RecordStackFrames will add a sample to the histogram. RecordStackFrames is
// called many times a second. We don't need multiple samples per second to
@@ -130,7 +130,7 @@
const raw_ptr<StackSamplerTestDelegate> test_delegate_;
#if BUILDFLAG(IS_CHROMEOS)
- // Counter for "Memory.StackSamplingProfiler.StackSampleSize" UMA histogram.
+ // Counter for "Memory.StackSamplingProfiler.StackSampleSize2" UMA histogram.
// See comments above kUMAHistogramDownsampleAmount. Unsigned so that overflow
// isn't undefined behavior.
uint32_t stack_size_histogram_sampling_counter_ = 0;
diff --git a/base/profiler/stack_sampler_unittest.cc b/base/profiler/stack_sampler_unittest.cc
index 41d593f..c5b9b5f 100644
--- a/base/profiler/stack_sampler_unittest.cc
+++ b/base/profiler/stack_sampler_unittest.cc
@@ -319,16 +319,16 @@
PlatformThread::CurrentId());
// Should have no new samples in the
- // Memory.StackSamplingProfiler.StackSampleSize histogram.
+ // Memory.StackSamplingProfiler.StackSampleSize2 histogram.
histogram_tester.ExpectUniqueSample(
- "Memory.StackSamplingProfiler.StackSampleSize", kExpectedSizeKB, 0);
+ "Memory.StackSamplingProfiler.StackSampleSize2", kExpectedSizeKB, 0);
}
stack_sampler->RecordStackFrames(stack_buffer.get(), &profile_builder,
PlatformThread::CurrentId());
histogram_tester.ExpectUniqueSample(
- "Memory.StackSamplingProfiler.StackSampleSize", kExpectedSizeKB, 1);
+ "Memory.StackSamplingProfiler.StackSampleSize2", kExpectedSizeKB, 1);
}
#endif // #if BUILDFLAG(IS_CHROMEOS)
diff --git a/base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java b/base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java
index 7f4d192..208b7b6 100644
--- a/base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java
+++ b/base/test/android/javatests/src/org/chromium/base/test/BaseJUnit4ClassRunner.java
@@ -237,7 +237,7 @@
@CallSuper
protected List<TestRule> getDefaultTestRules() {
return Arrays.asList(new BaseJUnit4TestRule(), new MockitoErrorHandler(),
- new UnitTestLifetimeAssertRule());
+ new UnitTestLifetimeAssertRule(), new ResettersForTestingTestRule());
}
/**
diff --git a/base/test/android/javatests/src/org/chromium/base/test/ResettersForTestingTestRule.java b/base/test/android/javatests/src/org/chromium/base/test/ResettersForTestingTestRule.java
new file mode 100644
index 0000000..7797306
--- /dev/null
+++ b/base/test/android/javatests/src/org/chromium/base/test/ResettersForTestingTestRule.java
@@ -0,0 +1,32 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.base.test;
+
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.Statement;
+
+import org.chromium.base.ResettersForTesting;
+
+/**
+ * Ensures that all resetters are cleaned up after a test. The resetters are registered through
+ * {@link ResettersForTesting#register(Runnable)} and are typically used whenever we have code that
+ * has <code>public static void setFooForTesting(...)</code> constructs.
+ */
+class ResettersForTestingTestRule implements TestRule {
+ @Override
+ public Statement apply(Statement base, Description description) {
+ return new Statement() {
+ @Override
+ public void evaluate() throws Throwable {
+ try {
+ base.evaluate();
+ } finally {
+ ResettersForTesting.executeResetters();
+ }
+ }
+ };
+ }
+}
diff --git a/base/test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java b/base/test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java
index a0d0608..defe3ae 100644
--- a/base/test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java
+++ b/base/test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java
@@ -18,6 +18,7 @@
import org.chromium.base.Flag;
import org.chromium.base.LifetimeAssert;
import org.chromium.base.PathUtils;
+import org.chromium.base.ResettersForTesting;
import org.chromium.base.ThreadUtils;
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.library_loader.LibraryProcessType;
@@ -68,6 +69,7 @@
} finally {
CommandLineFlags.tearDownMethod();
CommandLineFlags.tearDownClass();
+ ResettersForTesting.executeResetters();
ApplicationStatus.destroyForJUnitTests();
ContextUtils.clearApplicationContextForTests();
PathUtils.resetForTesting();
diff --git a/components/policy/core/common/async_policy_loader.cc b/components/policy/core/common/async_policy_loader.cc
index f22b9ad..4720c11 100644
--- a/components/policy/core/common/async_policy_loader.cc
+++ b/components/policy/core/common/async_policy_loader.cc
@@ -67,8 +67,7 @@
// `management_service_` must be called on the main thread.
// base::Unretained is okay here since `management_service_` is an instance of
// PlatformManagementService which is a singleton that outlives this class.
- if (!platform_management_trustworthiness_.has_value() &&
- management_service_) {
+ if (NeedManagementBitBeforeLoad()) {
DCHECK_EQ(management_service_, PlatformManagementService::GetInstance());
ui_thread_task_runner_->PostTaskAndReplyWithResult(
FROM_HERE,
@@ -125,6 +124,11 @@
Reload(force);
}
+bool AsyncPolicyLoader::NeedManagementBitBeforeLoad() {
+ return !platform_management_trustworthiness_.has_value() &&
+ management_service_;
+}
+
PolicyBundle AsyncPolicyLoader::InitialLoad(
const scoped_refptr<SchemaMap>& schema_map) {
// This is the first load, early during startup. Use this to record the
diff --git a/components/policy/core/common/async_policy_loader.h b/components/policy/core/common/async_policy_loader.h
index 7ee411f..c77212b 100644
--- a/components/policy/core/common/async_policy_loader.h
+++ b/components/policy/core/common/async_policy_loader.h
@@ -51,7 +51,9 @@
virtual ~AsyncPolicyLoader();
// Gets a SequencedTaskRunner backed by the background thread.
- base::SequencedTaskRunner* task_runner() const { return task_runner_.get(); }
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner() const {
+ return task_runner_;
+ }
// Returns the currently configured policies. Load() is always invoked on
// the background thread, except for the initial Load() at startup which is
@@ -81,7 +83,7 @@
// When |periodic_updates_| is true a reload is posted periodically, if it
// hasn't been triggered recently. This makes sure the policies are reloaded
// if the update events aren't triggered.
- void Reload(bool force);
+ virtual void Reload(bool force);
// Returns `true` and only if the platform is not managed by a trusted source.
bool ShouldFilterSensitivePolicies();
@@ -97,6 +99,11 @@
reload_interval_ = reload_interval;
}
+ protected:
+ // Return true if we need to asynchronously get
+ //`platform_management_trustworthiness_` bit before reloading policies.
+ bool NeedManagementBitBeforeLoad();
+
private:
// Allow AsyncPolicyProvider to call Init().
friend class AsyncPolicyProvider;
diff --git a/components/policy/core/common/policy_logger.cc b/components/policy/core/common/policy_logger.cc
index 416c7a3..1b45e8b 100644
--- a/components/policy/core/common/policy_logger.cc
+++ b/components/policy/core/common/policy_logger.cc
@@ -179,7 +179,7 @@
DCHECK_CALLED_ON_VALID_SEQUENCE(logs_list_sequence_checker_);
logs_.emplace_back(std::move(new_log));
- if (!is_log_deletion_scheduled_) {
+ if (!is_log_deletion_scheduled_ && is_log_deletion_enabled_) {
ScheduleOldLogsDeletion();
}
}
@@ -225,6 +225,10 @@
#endif // BUILDFLAG(IS_ANDROID)
}
+void PolicyLogger::EnableLogDeletion() {
+ is_log_deletion_enabled_ = true;
+}
+
size_t PolicyLogger::GetPolicyLogsSizeForTesting() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(logs_list_sequence_checker_);
return logs_.size();
@@ -234,6 +238,7 @@
DCHECK_CALLED_ON_VALID_SEQUENCE(logs_list_sequence_checker_);
logs_.erase(logs_.begin(), logs_.end());
is_log_deletion_scheduled_ = false;
+ is_log_deletion_enabled_ = false;
}
} // namespace policy
diff --git a/components/policy/core/common/policy_logger.h b/components/policy/core/common/policy_logger.h
index 9171b9a..2886fda 100644
--- a/components/policy/core/common/policy_logger.h
+++ b/components/policy/core/common/policy_logger.h
@@ -161,6 +161,9 @@
// Checks if browser is running on Android.
bool IsPolicyLoggingEnabled() const;
+ // Sets `is_log_deletion_enabled_` to allow scheduling old log deletion.
+ void EnableLogDeletion();
+
// Returns the logs size for testing purposes.
size_t GetPolicyLogsSizeForTesting() const;
@@ -181,6 +184,13 @@
// flag.
void ScheduleOldLogsDeletion();
+ // Log deletion scheduling fails in unit tests when there is no task
+ // environment (See crbug.com/1434241). To avoid having a task environment in
+ // every existing and new unit test that calls a function with logs, this flag
+ // is disabled in unit tests, and enabled everywhere else early in the policy
+ // stack initialization from `BrowserPolicyConnector::Init`.
+ bool is_log_deletion_enabled_{false};
+
bool is_log_deletion_scheduled_{false};
std::vector<Log> logs_ GUARDED_BY_CONTEXT(logs_list_sequence_checker_);
diff --git a/components/policy/core/common/policy_logger_unittest.cc b/components/policy/core/common/policy_logger_unittest.cc
index ff1c75f..a6ffbac 100644
--- a/components/policy/core/common/policy_logger_unittest.cc
+++ b/components/policy/core/common/policy_logger_unittest.cc
@@ -72,6 +72,7 @@
// Checks that the deletion of expired logs works as expected.
TEST_F(PolicyLoggerTest, DeleteOldLogs) {
PolicyLogger* policy_logger = policy::PolicyLogger::GetInstance();
+ policy_logger->EnableLogDeletion();
size_t logs_size_before_adding = policy_logger->GetPolicyLogsSizeForTesting();
AddLogs("First log at t=0.", policy_logger);
diff --git a/components/policy/core/common/scoped_critical_policy_section.cc b/components/policy/core/common/scoped_critical_policy_section.cc
new file mode 100644
index 0000000..99a5af1
--- /dev/null
+++ b/components/policy/core/common/scoped_critical_policy_section.cc
@@ -0,0 +1,119 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/policy/core/common/scoped_critical_policy_section.h"
+
+#include <userenv.h>
+#include <windows.h>
+
+#include "base/functional/bind.h"
+#include "base/logging.h"
+#include "base/task/bind_post_task.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/time/time.h"
+#include "components/policy/core/common/policy_types.h"
+
+namespace policy {
+
+namespace {
+
+void EnterSection(
+ ScopedCriticalPolicySection::OnSectionEnteredCallback callback) {
+ ScopedCriticalPolicySection::Handles handles;
+ // We need both user and machine handles. Based on MSFT doc, user handle must
+ // be acquired first to prevent dead lock.
+ // https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-entercriticalpolicysection
+ //
+ // If we failed to aquire lock or the API is timeout, we will read the policy
+ // regardless, as we used to have.
+ handles.user_handle = ::EnterCriticalPolicySection(false);
+ if (!handles.user_handle) {
+ PLOG(WARNING) << "Failed to enter user critical policy section.";
+ }
+ handles.machine_handle = ::EnterCriticalPolicySection(true);
+ if (!handles.machine_handle) {
+ PLOG(WARNING) << "Failed to enter machine critical policy section.";
+ }
+ std::move(callback).Run(handles);
+}
+
+} // namespace
+
+// static
+void ScopedCriticalPolicySection::Enter(
+ base::OnceClosure callback,
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
+ DCHECK(task_runner->RunsTasksInCurrentSequence());
+ EnterWithEnterSectionCallback(std::move(callback), EnterSectionCallback(),
+ task_runner);
+}
+
+// static
+void ScopedCriticalPolicySection::EnterWithEnterSectionCallback(
+ base::OnceClosure callback,
+ EnterSectionCallback enter_section_callback,
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
+ auto scoped_section =
+ std::make_unique<ScopedCriticalPolicySection>(task_runner);
+
+ scoped_section->enter_section_callback_ =
+ enter_section_callback ? std::move(enter_section_callback)
+ : base::BindOnce(&EnterSection);
+ scoped_section->Init(base::BindOnce(
+ [](std::unique_ptr<ScopedCriticalPolicySection> scoped_section,
+ base::OnceClosure callback) { std::move(callback).Run(); },
+ std::move(scoped_section), std::move(callback)));
+}
+
+ScopedCriticalPolicySection::ScopedCriticalPolicySection(
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+ : task_runner_(task_runner) {}
+
+ScopedCriticalPolicySection::~ScopedCriticalPolicySection() {
+ if (machine_handle_) {
+ ::LeaveCriticalPolicySection(machine_handle_);
+ }
+
+ if (user_handle_) {
+ ::LeaveCriticalPolicySection(user_handle_);
+ }
+}
+
+void ScopedCriticalPolicySection::Init(base::OnceClosure callback) {
+ DCHECK(!callback_);
+ callback_ = std::move(callback);
+
+ if (enter_section_callback_) {
+ // Call ::EnterCriticalPolicySection in a different thread as the API could
+ // take minutes to return.
+ // Using `PostTask` instead of `PostTaskAndReplyWithResult` allows unit test
+ // mimic blocking function easily.
+ auto on_section_entered = base::BindPostTask(
+ task_runner_,
+ base::BindOnce(&ScopedCriticalPolicySection::OnSectionEntered,
+ weak_factory_.GetWeakPtr()));
+ base::ThreadPool::PostTask(
+ FROM_HERE, {base::MayBlock()},
+ base::BindOnce(std::move(enter_section_callback_),
+ std::move(on_section_entered)));
+ }
+
+ // Based on UMA data, 15 seconds timeout is enough for 99.9% cases.
+ task_runner_->PostDelayedTask(
+ FROM_HERE,
+ base::BindOnce(&ScopedCriticalPolicySection::OnSectionEntered,
+ weak_factory_.GetWeakPtr(), Handles()),
+ base::Seconds(15));
+}
+
+void ScopedCriticalPolicySection::OnSectionEntered(Handles handles) {
+ DCHECK(task_runner_->RunsTasksInCurrentSequence());
+ machine_handle_ = handles.machine_handle;
+ user_handle_ = handles.user_handle;
+ std::move(callback_).Run();
+}
+
+} // namespace policy
diff --git a/components/policy/core/common/scoped_critical_policy_section.h b/components/policy/core/common/scoped_critical_policy_section.h
new file mode 100644
index 0000000..cc3d246
--- /dev/null
+++ b/components/policy/core/common/scoped_critical_policy_section.h
@@ -0,0 +1,72 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_POLICY_CORE_COMMON_SCOPED_CRITICAL_POLICY_SECTION_H_
+#define COMPONENTS_POLICY_CORE_COMMON_SCOPED_CRITICAL_POLICY_SECTION_H_
+
+#include "base/functional/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "components/policy/policy_export.h"
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace policy {
+
+// Scoped class for ::EnterCriticalPolicySection API. The class make sure we try
+// to acquire the section before reading the policy values. It will leave the
+// section in the end and self destory.
+class POLICY_EXPORT ScopedCriticalPolicySection {
+ public:
+ struct Handles {
+ HANDLE machine_handle;
+ HANDLE user_handle;
+ };
+
+ using OnSectionEnteredCallback = base::OnceCallback<void(Handles)>;
+ using EnterSectionCallback =
+ base::OnceCallback<void(OnSectionEnteredCallback)>;
+
+ // Create and own `ScopedCriticalPolicySection` instance. And destory itself
+ // after `callback` being invoked.
+ // This must be called on the background thread. When loading policy on the
+ // main thread, we can't wait for the API as everything must be returned
+ // synchronously.
+ static void Enter(
+ base::OnceClosure callback,
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+
+ // Same but with custome function to enter critical policy section`. Only
+ // used for testing purposes.
+ static void EnterWithEnterSectionCallback(
+ base::OnceClosure callback,
+ EnterSectionCallback enter_section_callback,
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+
+ ScopedCriticalPolicySection(
+ const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+ ScopedCriticalPolicySection(const ScopedCriticalPolicySection&) = delete;
+ ScopedCriticalPolicySection& operator=(const ScopedCriticalPolicySection&) =
+ delete;
+ ~ScopedCriticalPolicySection();
+
+ void Init(base::OnceClosure callback);
+
+ private:
+ void OnSectionEntered(Handles handles);
+
+ HANDLE machine_handle_ = nullptr;
+ HANDLE user_handle_ = nullptr;
+ const scoped_refptr<base::SequencedTaskRunner> task_runner_;
+ base::OnceClosure callback_;
+
+ EnterSectionCallback enter_section_callback_;
+
+ base::WeakPtrFactory<ScopedCriticalPolicySection> weak_factory_{this};
+};
+
+} // namespace policy
+
+#endif // COMPONENTS_POLICY_CORE_COMMON_SCOPED_CRITICAL_POLICY_SECTION_H_
diff --git a/components/policy/core/common/scoped_critical_policy_section_unittest.cc b/components/policy/core/common/scoped_critical_policy_section_unittest.cc
new file mode 100644
index 0000000..07d1718
--- /dev/null
+++ b/components/policy/core/common/scoped_critical_policy_section_unittest.cc
@@ -0,0 +1,72 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/policy/core/common/scoped_critical_policy_section.h"
+
+#include "base/functional/bind.h"
+#include "base/functional/callback_helpers.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
+#include "base/test/mock_callback.h"
+#include "base/test/task_environment.h"
+#include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace policy {
+
+class ScopedCriticalPolicySectionTest : public ::testing::Test {
+ public:
+ ScopedCriticalPolicySectionTest()
+ : task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME),
+ task_runner_(base::ThreadPool::CreateSequencedTaskRunner({})) {}
+
+ ~ScopedCriticalPolicySectionTest() override = default;
+ base::SequencedTaskRunner* task_runner() { return task_runner_.get(); }
+
+ base::test::TaskEnvironment* task_environment() { return &task_environment_; }
+
+ private:
+ base::test::TaskEnvironment task_environment_;
+ scoped_refptr<base::SequencedTaskRunner> task_runner_;
+};
+
+TEST_F(ScopedCriticalPolicySectionTest, Normal) {
+ ::testing::StrictMock<base::MockOnceClosure> mock_callback;
+ EXPECT_CALL(mock_callback, Run()).Times(1);
+ ScopedCriticalPolicySection::EnterWithEnterSectionCallback(
+ mock_callback.Get(),
+ base::BindOnce(
+ [](ScopedCriticalPolicySection::OnSectionEnteredCallback callback) {
+ std::move(callback).Run(ScopedCriticalPolicySection::Handles());
+ }),
+ task_runner());
+ task_environment()->RunUntilIdle();
+}
+
+TEST_F(ScopedCriticalPolicySectionTest, Timeout) {
+ auto fake_enter_section_function = base::BindOnce(
+ [](ScopedCriticalPolicySection::OnSectionEnteredCallback callback) {
+ base::ThreadPool::PostDelayedTask(
+ FROM_HERE,
+ base::BindOnce(std::move(callback),
+ ScopedCriticalPolicySection::Handles()),
+ base::Seconds(30));
+ });
+
+ ::testing::StrictMock<base::MockOnceClosure> mock_callback;
+ EXPECT_CALL(mock_callback, Run()).Times(0);
+ ScopedCriticalPolicySection::EnterWithEnterSectionCallback(
+ mock_callback.Get(), std::move(fake_enter_section_function),
+ task_runner());
+ task_environment()->RunUntilIdle();
+ ::testing::Mock::VerifyAndClearExpectations(&mock_callback);
+
+ EXPECT_CALL(mock_callback, Run()).Times(1);
+ task_environment()->FastForwardBy(base::Seconds(15));
+}
+
+} // namespace policy
