gh-145306: gate --browser on export success

Author: pablogsal

Lib/profiling/sampling/binary_collector.py

@@ -94,6 +94,7 @@ def export(self, filename=None):
             filename: Ignored (binary files are written incrementally)
         """
         self._writer.finalize()
+        return True
 
     @property
     def total_samples(self):

Lib/profiling/sampling/cli.py

@@ -660,10 +660,14 @@ def _handle_output(collector, args, pid, mode):
             filename = os.path.join(args.outfile, _generate_output_filename(args.format, pid))
         else:
             filename = args.outfile or _generate_output_filename(args.format, pid)
-        collector.export(filename)
+        export_ok = collector.export(filename)
 
         # Auto-open browser for HTML output if --browser flag is set
-        if args.format in ('flamegraph', 'heatmap') and getattr(args, 'browser', False):
+        if (
+            export_ok
+            and args.format in ('flamegraph', 'heatmap')
+            and getattr(args, 'browser', False)
+        ):
             _open_in_browser(filename)
 
 
@@ -1203,10 +1207,14 @@ def progress_callback(current, total):
                 collector.print_stats(sort_mode, limit, not args.no_summary, PROFILING_MODE_WALL)
         else:
             filename = args.outfile or _generate_output_filename(args.format, os.getpid())
-            collector.export(filename)
+            export_ok = collector.export(filename)
 
             # Auto-open browser for HTML output if --browser flag is set
-            if args.format in ('flamegraph', 'heatmap') and getattr(args, 'browser', False):
+            if (
+                export_ok
+                and args.format in ('flamegraph', 'heatmap')
+                and getattr(args, 'browser', False)
+            ):
                 _open_in_browser(filename)
 
         print(f"Replayed {count} samples")

Lib/profiling/sampling/collector.py

@@ -81,7 +81,11 @@ def collect_failed_sample(self):
 
     @abstractmethod
     def export(self, filename):
-        """Export collected data to a file."""
+        """Export collected data.
+
+        Returns:
+            bool: True if output was generated, False if there was no data to export.
+        """
 
     @staticmethod
     def _filter_internal_frames(frames):

Lib/profiling/sampling/gecko_collector.py

@@ -701,6 +701,7 @@ def spin():
         print(
             f"Open in Firefox Profiler: https://profiler.firefox.com/"
         )
+        return True
 
     def _build_marker_schema(self):
         """Build marker schema definitions for Firefox Profiler."""

Lib/profiling/sampling/heatmap_collector.py

@@ -714,7 +714,7 @@ def export(self, output_path):
         """
         if not self.file_samples:
             print("Warning: No heatmap data to export")
-            return
+            return False
 
         try:
             output_dir = self._prepare_output_directory(output_path)
@@ -728,6 +728,7 @@ def export(self, output_path):
             self._generate_index_html(output_dir / 'index.html', file_stats)
 
             self._print_export_summary(output_dir, file_stats)
+            return True
 
         except Exception as e:
             print(f"Error: Failed to export heatmap: {e}")

Lib/profiling/sampling/pstats_collector.py

@@ -61,6 +61,7 @@ def collect(self, stack_frames, timestamps_us=None):
     def export(self, filename):
         self.create_stats()
         self._dump_stats(filename)
+        return True
 
     def _dump_stats(self, file):
         stats_with_marker = dict(self.stats)

Lib/profiling/sampling/stack_collector.py

@@ -61,6 +61,7 @@ def export(self, filename):
             for stack, count in lines:
                 f.write(f"{stack} {count}\n")
         print(f"Collapsed stack output written to {filename}")
+        return True
 
 
 class FlamegraphCollector(StackTraceCollector):
@@ -153,14 +154,15 @@ def export(self, filename):
             print(
                 "Warning: No functions found in profiling data. Check if sampling captured any data."
             )
-            return
+            return False
 
         html_content = self._create_flamegraph_html(flamegraph_data)
 
         with open(filename, "w", encoding="utf-8") as f:
             f.write(html_content)
 
         print(f"Flamegraph saved to: {filename}")
+        return True
 
     @staticmethod
     @functools.lru_cache(maxsize=None)

Lib/test/test_profiling/test_sampling_profiler/test_cli.py

@@ -4,6 +4,7 @@
 import subprocess
 import sys
 import unittest
+from types import SimpleNamespace
 from unittest import mock
 
 try:
@@ -15,7 +16,7 @@
 
 from test.support import is_emscripten, requires_remote_subprocess_debugging
 
-from profiling.sampling.cli import main
+from profiling.sampling.cli import _handle_output, _handle_replay, main
 from profiling.sampling.errors import SamplingScriptNotFoundError, SamplingModuleNotFoundError, SamplingUnknownProcessError
 
 class TestSampleProfilerCLI(unittest.TestCase):
@@ -700,6 +701,106 @@ def test_async_aware_incompatible_with_all_threads(self):
         self.assertIn("--all-threads", error_msg)
         self.assertIn("incompatible with --async-aware", error_msg)
 
+    def test_handle_output_browser_not_opened_when_export_fails(self):
+        collector = mock.MagicMock()
+        collector.export.return_value = False
+        args = SimpleNamespace(
+            format="flamegraph",
+            outfile="profile.html",
+            browser=True,
+        )
+
+        with (
+            mock.patch("profiling.sampling.cli.os.path.isdir", return_value=False),
+            mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
+        ):
+            _handle_output(collector, args, pid=12345, mode=0)
+
+        collector.export.assert_called_once_with("profile.html")
+        mock_open.assert_not_called()
+
+    def test_handle_output_browser_opened_when_export_succeeds(self):
+        collector = mock.MagicMock()
+        collector.export.return_value = True
+        args = SimpleNamespace(
+            format="flamegraph",
+            outfile="profile.html",
+            browser=True,
+        )
+
+        with (
+            mock.patch("profiling.sampling.cli.os.path.isdir", return_value=False),
+            mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
+        ):
+            _handle_output(collector, args, pid=12345, mode=0)
+
+        collector.export.assert_called_once_with("profile.html")
+        mock_open.assert_called_once_with("profile.html")
+
+    def test_replay_browser_not_opened_when_export_fails(self):
+        args = SimpleNamespace(
+            input_file="profile.bin",
+            format="flamegraph",
+            outfile="profile.html",
+            browser=True,
+            sort=None,
+            limit=None,
+            no_summary=False,
+        )
+        collector = mock.MagicMock()
+        collector.export.return_value = False
+
+        with (
+            mock.patch("profiling.sampling.cli.os.path.exists", return_value=True),
+            mock.patch("profiling.sampling.cli.BinaryReader") as mock_reader_cls,
+            mock.patch("profiling.sampling.cli._create_collector", return_value=collector),
+            mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
+        ):
+            reader = mock_reader_cls.return_value.__enter__.return_value
+            reader.get_info.return_value = {
+                "sample_count": 1,
+                "sample_interval_us": 1000,
+                "compression_type": 0,
+            }
+            reader.replay_samples.return_value = 1
+
+            _handle_replay(args)
+
+        collector.export.assert_called_once_with("profile.html")
+        mock_open.assert_not_called()
+
+    def test_replay_browser_opened_when_export_succeeds(self):
+        args = SimpleNamespace(
+            input_file="profile.bin",
+            format="flamegraph",
+            outfile="profile.html",
+            browser=True,
+            sort=None,
+            limit=None,
+            no_summary=False,
+        )
+        collector = mock.MagicMock()
+        collector.export.return_value = True
+
+        with (
+            mock.patch("profiling.sampling.cli.os.path.exists", return_value=True),
+            mock.patch("profiling.sampling.cli.BinaryReader") as mock_reader_cls,
+            mock.patch("profiling.sampling.cli._create_collector", return_value=collector),
+            mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
+        ):
+            reader = mock_reader_cls.return_value.__enter__.return_value
+            reader.get_info.return_value = {
+                "sample_count": 1,
+                "sample_interval_us": 1000,
+                "compression_type": 0,
+            }
+            reader.replay_samples.return_value = 1
+
+            _handle_replay(args)
+
+        collector.export.assert_called_once_with("profile.html")
+        mock_open.assert_called_once_with("profile.html")
+
     @unittest.skipIf(is_emscripten, "subprocess not available")
     def test_run_nonexistent_script_exits_cleanly(self):
         """Test that running a non-existent script exits with a clean error."""