Merge branch 'youtube:main' into no_exception

.github/workflows/unit_test_report.yaml

@@ -9,11 +9,12 @@ on:
     types:
       - completed
 
+# TODO(b/293508740): Report failed workflow runs back to triggering PR.
+
 jobs:
   unit-test-report:
-    permissions: {}
-      # TODO(b/293508740): Report failed workflow runs back to triggering PR.
-      # pull-requests: write
+    permissions:
+      pull-requests: write
     if: ${{ github.event.workflow_run.conclusion == 'success' || github.event.workflow_run.conclusion == 'failure' }}
     runs-on: ubuntu-latest
     name: Upload Unit Test Reports

.pre-commit-config.yaml

@@ -27,13 +27,15 @@ exclude: |
             testing|
             third_party|
             tools/gyp|
-            tools/metrics|
             tools/python|
             url
         )/
         |
         components/update_client/((?!cobalt).)*$
         |
+        # Ignore everything under tools/metrics _except_ Cobalt files. We
+        # need those validated to keep the telemetry/metrics pipeline working.
+        tools/metrics/((?!cobalt\/).)*$|
         .*\.pb\.cc$ |
         .*\.pb\.h$ |
         .*\.patch$ |
@@ -49,6 +51,7 @@ repos:
     -   id: end-of-file-fixer
     -   id: trailing-whitespace
     -   id: mixed-line-ending
+    -   id: check-xml
 
 -   repo: https://cobalt.googlesource.com/codespell
     rev: 67c489d36dd4c52cbb9e4755d90c35c6231842ef  # v2.0.0

cobalt/browser/metrics/cobalt_metrics_log_uploader.cc

@@ -67,8 +67,13 @@ void CobaltMetricsLogUploader::UploadLog(
       base::Base64UrlEncode(cobalt_uma_event.SerializeAsString(),
                             base::Base64UrlEncodePolicy::INCLUDE_PADDING,
                             &base64_encoded_proto);
-      upload_handler_->Run(h5vcc::H5vccMetricType::kH5vccMetricTypeCobaltUma,
-                           base64_encoded_proto);
+      // Check again that the upload handler is still valid. Was seeing race
+      // conditions where it was being destroyed while the proto encoding was
+      // happening above.
+      if (upload_handler_ != nullptr) {
+        upload_handler_->Run(h5vcc::H5vccMetricType::kH5vccMetricTypeCobaltUma,
+                             base64_encoded_proto);
+      }
     }
   }
 

cobalt/browser/metrics/cobalt_metrics_service_client.cc

@@ -58,6 +58,17 @@ void CobaltMetricsServiceClient::SetOnUploadHandler(
   }
 }
 
+void CobaltMetricsServiceClient::RemoveOnUploadHandler(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  // Only remove the upload handler if our current reference matches that which
+  // is passed in. Avoids issues with race conditions with two threads trying to
+  // override the handler.
+  if (upload_handler_ == uploader_callback) {
+    LOG(INFO) << "Upload handler removed.";
+    upload_handler_ = nullptr;
+  }
+}
+
 CobaltMetricsServiceClient::CobaltMetricsServiceClient(
     ::metrics::MetricsStateManager* state_manager, PrefService* local_state)
     : metrics_state_manager_(state_manager) {
@@ -77,7 +88,8 @@ ukm::UkmService* CobaltMetricsServiceClient::GetUkmService() {
 
 void CobaltMetricsServiceClient::SetMetricsClientId(
     const std::string& client_id) {
-  // TODO(b/286066035): What to do with client id here?
+  // ClientId is unnecessary within Cobalt. We expect the web client responsible
+  // for uploading these to have its own concept of device/client identifiers.
 }
 
 // TODO(b/286884542): Audit all stub implementations in this class and reaffirm

cobalt/browser/metrics/cobalt_metrics_service_client.h

@@ -53,6 +53,11 @@ class CobaltMetricsServiceClient : public ::metrics::MetricsServiceClient {
   void SetOnUploadHandler(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  // Remove reference to the passed uploader callback, if it's the current
+  // reference. Otherwise, does nothing.
+  void RemoveOnUploadHandler(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   // Returns the MetricsService instance that this client is associated with.
   // With the exception of testing contexts, the returned instance must be valid
   // for the lifetime of this object (typically, the embedder's client

cobalt/browser/metrics/cobalt_metrics_services_manager.cc

@@ -41,14 +41,40 @@ CobaltMetricsServicesManager* CobaltMetricsServicesManager::GetInstance() {
   return instance_;
 }
 
-void CobaltMetricsServicesManager::DeleteInstance() { delete instance_; }
+void CobaltMetricsServicesManager::DeleteInstance() {
+  delete instance_;
+  instance_ = nullptr;
+}
+
+void CobaltMetricsServicesManager::RemoveOnUploadHandler(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  if (instance_ != nullptr) {
+    instance_->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&CobaltMetricsServicesManager::RemoveOnUploadHandlerInternal,
+                   base::Unretained(instance_), uploader_callback));
+  }
+}
+
+void CobaltMetricsServicesManager::RemoveOnUploadHandlerInternal(
+    const CobaltMetricsUploaderCallback* uploader_callback) {
+  CobaltMetricsServiceClient* client =
+      static_cast<CobaltMetricsServiceClient*>(GetMetricsServiceClient());
+  DCHECK(client);
+  client->RemoveOnUploadHandler(uploader_callback);
+}
 
 void CobaltMetricsServicesManager::SetOnUploadHandler(
     const CobaltMetricsUploaderCallback* uploader_callback) {
-  instance_->task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&CobaltMetricsServicesManager::SetOnUploadHandlerInternal,
-                 base::Unretained(instance_), uploader_callback));
+  // H5vccMetrics calls this on destruction when the WebModule is torn down. On
+  // shutdown, CobaltMetricsServicesManager can be destructed before
+  // H5vccMetrics, so we make sure we have a valid instance here.
+  if (instance_ != nullptr) {
+    instance_->task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&CobaltMetricsServicesManager::SetOnUploadHandlerInternal,
+                   base::Unretained(instance_), uploader_callback));
+  }
 }
 
 void CobaltMetricsServicesManager::SetOnUploadHandlerInternal(

cobalt/browser/metrics/cobalt_metrics_services_manager.h

@@ -59,6 +59,14 @@ class CobaltMetricsServicesManager
   static void SetOnUploadHandler(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  // Attempts to clean up the passed reference to CobaltMetricsUploaderCallback,
+  // IFF it matches the current callback reference in
+  // CobaltMetricsServiceManager. This is to avoid situations where two clients
+  // are competing to override the upload handler and prevent one from
+  // inadvertently clobbering another.
+  static void RemoveOnUploadHandler(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   // Toggles whether metric reporting is enabled via
   // CobaltMetricsServicesManager.
   static void ToggleMetricsEnabled(bool is_enabled);
@@ -71,6 +79,9 @@ class CobaltMetricsServicesManager
   void SetOnUploadHandlerInternal(
       const CobaltMetricsUploaderCallback* uploader_callback);
 
+  void RemoveOnUploadHandlerInternal(
+      const CobaltMetricsUploaderCallback* uploader_callback);
+
   void ToggleMetricsEnabledInternal(bool is_enabled);
 
   void SetUploadIntervalInternal(uint32_t interval_seconds);

cobalt/build/cobalt_configuration.py

@@ -133,8 +133,7 @@ def GetTestTargets(self):
         'media_capture_test',
         'media_session_test',
         'media_stream_test',
-        # TODO(b/292030213): Crashes on evergreen
-        # 'media_test',
+        'media_test',
         'memory_store_test',
         'metrics_test',
         'net_unittests',

cobalt/build/gn.py

@@ -51,6 +51,11 @@ def main(out_directory: str, platform: str, build_type: str,
 
 
 if __name__ == '__main__':
+  current_working_dir = os.getcwd()
+  python_env_var = os.getenv('PYTHONPATH')
+  assert current_working_dir in python_env_var, 'env variable PYTHONPATH \
+  should be set to current repo root directory.'
+
   parser = argparse.ArgumentParser()
 
   builds_directory_group = parser.add_mutually_exclusive_group()

cobalt/demos/readme.md

@@ -6,4 +6,4 @@ These html pages can be executed by first running a basic server
 `python3 -m http.server` in the `cobalt_src` directory and then running
 Cobalt and pointing the `--url` parameter to the file in question. For example:
 
-`out/.../cobalt --url=http://0.0.0.0:8000/cobalt/demos/content/watchdog-demo/index.html`
+`out/.../cobalt --url=http://127.0.0.1:8000/cobalt/demos/content/watchdog-demo/index.html`

cobalt/h5vcc/h5vcc_metrics.cc

@@ -25,6 +25,17 @@
 namespace cobalt {
 namespace h5vcc {
 
+H5vccMetrics::~H5vccMetrics() {
+  if (browser::metrics::CobaltMetricsServicesManager::GetInstance() !=
+          nullptr &&
+      run_event_handler_callback_) {
+    // We need to let the metrics manager know not to call the upload callback
+    // any longer, otherwise it could crash.
+    browser::metrics::CobaltMetricsServicesManager::GetInstance()
+        ->RemoveOnUploadHandler(run_event_handler_callback_.get());
+  }
+}
+
 void H5vccMetrics::OnMetricEvent(
     const h5vcc::MetricEventHandlerWrapper::ScriptValue& event_handler) {
   if (!uploader_callback_) {
@@ -43,16 +54,20 @@ void H5vccMetrics::OnMetricEvent(
 void H5vccMetrics::RunEventHandler(
     const cobalt::h5vcc::H5vccMetricType& metric_type,
     const std::string& serialized_proto) {
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&H5vccMetrics::RunEventHandlerInternal, base::Unretained(this),
-                 metric_type, serialized_proto));
+  if (task_runner_ && task_runner_->HasAtLeastOneRef()) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&H5vccMetrics::RunEventHandlerInternal,
+                   base::Unretained(this), metric_type, serialized_proto));
+  }
 }
 
 void H5vccMetrics::RunEventHandlerInternal(
     const cobalt::h5vcc::H5vccMetricType& metric_type,
     const std::string& serialized_proto) {
-  uploader_callback_->callback.value().Run(metric_type, serialized_proto);
+  if (uploader_callback_ != nullptr && uploader_callback_->HasAtLeastOneRef()) {
+    uploader_callback_->callback.value().Run(metric_type, serialized_proto);
+  }
 }
 
 void H5vccMetrics::Enable() { ToggleMetricsEnabled(true); }

cobalt/h5vcc/h5vcc_metrics.h

@@ -47,6 +47,8 @@ class H5vccMetrics : public script::Wrappable {
       : task_runner_(base::ThreadTaskRunnerHandle::Get()),
         persistent_settings_(persistent_settings) {}
 
+  ~H5vccMetrics();
+
   H5vccMetrics(const H5vccMetrics&) = delete;
   H5vccMetrics& operator=(const H5vccMetrics&) = delete;
 

cobalt/media/BUILD.gn

@@ -43,6 +43,8 @@ component("media") {
     "base/format_support_query_metrics.h",
     "base/interleaved_sinc_resampler.cc",
     "base/interleaved_sinc_resampler.h",
+    "base/metrics_provider.cc",
+    "base/metrics_provider.h",
     "base/playback_statistics.cc",
     "base/playback_statistics.h",
     "base/sbplayer_bridge.cc",

cobalt/media/base/cval_stats_test.cc

@@ -12,16 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#ifdef _WIN32
-#include <Windows.h>
-#else
-#include <unistd.h>
-#endif
+#include "cobalt/media/base/cval_stats.h"
 
 #include <set>
 #include <string>
 
-#include "cobalt/media/base/cval_stats.h"
+#include "starboard/thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 
@@ -33,21 +29,11 @@ const char kCValnameMinimum[] = "Media.SbPlayerCreateTime.Minimum";
 
 const char kPipelineIdentifier[] = "test_pipeline";
 
-constexpr int kSleepTimeMs = 50;
+constexpr SbTime kSleepTime = 50;  // 50 microseconds
 
 namespace cobalt {
 namespace media {
 
-namespace {
-void sleep_ms(int ms) {
-#ifdef _WIN32
-  Sleep(ms);
-#else
-  usleep(ms);
-#endif
-}
-}  // namespace
-
 TEST(MediaCValStatsTest, InitiallyEmpty) {
   base::CValManager* cvm = base::CValManager::GetInstance();
   EXPECT_TRUE(cvm);
@@ -71,7 +57,8 @@ TEST(MediaCValStatsTest, NothingRecorded) {
   CValStats cval_stats_;
 
   cval_stats_.StartTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
-  sleep_ms(kSleepTimeMs);
+  SbThreadSleep(kSleepTime);
+
   cval_stats_.StopTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
 
   base::Optional<std::string> result =
@@ -88,7 +75,7 @@ TEST(MediaCValStatsTest, EnableRecording) {
   cval_stats_.Enable(true);
 
   cval_stats_.StartTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
-  sleep_ms(kSleepTimeMs);
+  SbThreadSleep(kSleepTime);
   cval_stats_.StopTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
 
   base::Optional<std::string> result =
@@ -114,7 +101,7 @@ TEST(MediaCValStatsTest, DontGenerateHistoricalData) {
 
   for (int i = 0; i < kMediaDefaultMaxSamplesBeforeCalculation - 1; i++) {
     cval_stats_.StartTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
-    sleep_ms(kSleepTimeMs);
+    SbThreadSleep(kSleepTime);
     cval_stats_.StopTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
   }
 
@@ -141,7 +128,7 @@ TEST(MediaCValStatsTest, GenerateHistoricalData) {
 
   for (int i = 0; i < kMediaDefaultMaxSamplesBeforeCalculation; i++) {
     cval_stats_.StartTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
-    sleep_ms(kSleepTimeMs);
+    SbThreadSleep(kSleepTime);
     cval_stats_.StopTimer(MediaTiming::SbPlayerCreate, kPipelineIdentifier);
   }