Re: [PATCH v7 31/59] perf gecko: Port gecko to use python module

sashiko-bot
2026-04-25T23:06:54Z

Sashiko AI review found 4 potential issue(s):
- [Medium] Unchecked return value from `self.session.process_events()` masks processing errors.
- [Medium] Generated profiler URL is not printed, breaking usage on headless environments.
- [Medium] Local HTTP server uses a predictable filename, allowing local information disclosure.
- [Low] Unreachable `except AttributeError:` block in `process_event`.
--

commit 280d1922588c70d042653eee3d46f2293162d4af
Author: Ian Rogers <irogers@google.com>

perf gecko: Port gecko to use python module

This commit ports the gecko script to use the perf Python module directly.
It replaces perf script callbacks with direct event processing to avoid
creating intermediate dictionaries and improve execution speed.

> diff --git a/tools/perf/python/gecko.py b/tools/perf/python/gecko.py
> new file mode 100755
> index 000000000000..01e9052f225e
> --- /dev/null
> +++ b/tools/perf/python/gecko.py

[ ... ]

> + def process_event(self, sample) -> None:

[ ... ]

> + else:
> + # Fallback if no callchain is present
> + try:
> + # If the perf module exposes symbol/dso directly on sample
> + # when callchain is missing, we use them.
> + symbol = getattr(sample, 'symbol', '[unknown]') or '[unknown]'
> + dso = getattr(sample, 'dso', '[unknown]') or '[unknown]'
> + stack.append(f"{symbol} (in {dso})")
> + except AttributeError:
> + stack.append("[unknown] (in [unknown])")

Because getattr is called with a default value like '[unknown]', it returns
that default instead of raising an AttributeError when the attribute is
missing. Does this make the try/except construct unnecessary and the except
block unreachable code?

[ ... ]

> + def run(self) -> None:
> + """Run the conversion process."""
> + input_file = self.args.input or "perf.data"
> + if not os.path.exists(input_file):
> + print(f"Error: {input_file} not found.", file=sys.stderr)
> + sys.exit(1)
> +
> + try:
> + self.session = perf.session(perf.data(input_file), sample=self.process_event)
> + except Exception as e:
> + print(f"Error opening session: {e}", file=sys.stderr)
> + sys.exit(1)
> +
> + self.session.process_events()

Is the return value of process_events() intentionally ignored?

If the underlying perf_session__process_events C function fails due to a
corrupted perf.data file or a parsing error, will this script silently
continue and generate an incomplete profile without notifying the user?

[ ... ]

> + def _write_and_launch(self, profile: Dict) -> None:
> + """Write the profile to a file and launch the Firefox profiler."""
> + print("Starting Firefox Profiler on your default browser...")
> +
> + with tempfile.TemporaryDirectory() as tmp_dir_name:
> + filename = os.path.join(tmp_dir_name, 'gecko_profile.json')

Serving a predictable filename like gecko_profile.json on localhost could
allow another local user to quickly port scan and download the profile data,
which contains process names and memory trace data.

Could this use a randomly generated token in the filename to prevent local
data leaks?

> +
> + with open(filename, 'w', encoding='utf-8') as f:
> + json.dump(profile, f, indent=2)

[ ... ]

> + # Open the browser
> + safe_string = urllib.parse.quote_plus(f'http://127.0.0.1:{port}/gecko_profile.json')
> + url = f'https://profiler.firefox.com/from-url/{safe_string}'
> + webbrowser.open(url)

In headless environments or remote SSH sessions, webbrowser.open(url) fails
and returns False.

Because this return value is ignored and the target url is never printed to
standard output, how can users on headless servers discover the dynamically
assigned port and correct url to access their profile?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260425224951.174663-1-irogers@google.com?part=31