Skip to content

docs: clarify CdpHttpRequest owns its logger (upstream #15338 N/A) - #3552

Merged
kblok merged 5 commits into
masterfrom
cursor/implement-upstream-change-15338-a3ef
Aug 14, 2026
Merged

docs: clarify CdpHttpRequest owns its logger (upstream #15338 N/A)#3552
kblok merged 5 commits into
masterfrom
cursor/implement-upstream-change-15338-a3ef

Conversation

@kblok

@kblok kblok commented Aug 13, 2026

Copy link
Copy Markdown
Member

Tracks puppeteer/puppeteer#15338 from puppeteer-core-v25.7.0.

Verdict

No behavioral change. Upstream fixed crashes where request error handlers did (this.frame() as any).logger after the frame was detached (frame() null).

PuppeteerSharp already avoids that pattern:

  • CdpHttpRequest takes ILoggerFactory, stores _logger, and HandleError uses that instance
  • NetworkManager passes the factory when constructing requests
  • BiDi request error paths do not look up a logger via Frame

What changed

  • Documented on CdpHttpRequest that the logger is request-owned so detach-safe error handling is intentional (upstream equivalent already present)

Test plan

  • Full CI green (path-filtered build triggered by the .cs comment change)
Open in Web Open in Cursor 

Upstream PR #15338 fixes crashes where CdpHTTPRequest/BidiHTTPRequest
error handlers accessed `(this.frame() as any).logger` after a frame
detached (null frame). The fix stores a logger on the request and passes
it from NetworkManager/BidiFrame, and widens logger visibility so call
sites no longer need `as any`.

PuppeteerSharp already avoids this bug:
- CdpHttpRequest takes ILoggerFactory in its constructor, stores _logger,
  and HandleError uses that logger — never Frame.Logger.
- NetworkManager already passes _loggerFactory when constructing requests.
- CdpHttpRequestTests already supply a LoggerFactory.
- BidiHttpRequest error paths do not dereference Frame.Logger.
- Locators do not consume Frame.Logger, so the protected→public visibility
  change has no .NET counterpart to port.

No code changes required; this commit records the upstream sync.

Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
@kblok
kblok marked this pull request as ready for review August 13, 2026 22:47
@cursor

cursor Bot commented Aug 13, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

cursoragent and others added 4 commits August 13, 2026 22:52
Document that CdpHttpRequest keeps its own ILogger so error handling
does not depend on a possibly-detached Frame (already the .NET pattern).

Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
Co-authored-by: Darío Kondratiuk <dariokondratiuk@gmail.com>
@cursor cursor Bot changed the title Implement upstream PR #15338 docs: clarify CdpHttpRequest owns its logger (upstream #15338 N/A) Aug 14, 2026
@kblok
kblok merged commit ac898ff into master Aug 14, 2026
17 checks passed
@kblok
kblok deleted the cursor/implement-upstream-change-15338-a3ef branch August 14, 2026 12:26
@kblok

kblok commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

Upstream PR #15338 → PuppeteerSharp

Upstream

  • PR: puppeteer/puppeteer#15338
  • Title: fix: logger calling causing crashes
  • Summary: Upstream crashed when request error handlers accessed (this.frame() as any).logger after the frame was detached (frame() null). Fix stores a logger on the request and passes it from NetworkManager / BidiFrame.

PuppeteerSharp mapping

Upstream PuppeteerSharp
Request-owned logger CdpHttpRequest already takes ILoggerFactory and stores _logger
NetworkManager passes logger Already passes _loggerFactory when constructing requests
Avoid Frame.Logger in error paths HandleError uses request _logger only

Changes

No behavioral change. Documented on CdpHttpRequest that the logger is intentionally request-owned so error handling stays safe after frame detach (upstream equivalent already present).

Verification

  • Build (Chrome/CDP): success
  • CdpHttpRequestTests: passed

@kblok kblok mentioned this pull request Aug 14, 2026
sondresjolyst pushed a commit to sondresjolyst/garge-api that referenced this pull request Aug 24, 2026
Updated [PuppeteerSharp](https://github.com/hardkoded/puppeteer-sharp)
from 25.5.0 to 25.7.0.

<details>
<summary>Release notes</summary>

_Sourced from [PuppeteerSharp's
releases](https://github.com/hardkoded/puppeteer-sharp/releases)._

## 25.7.0

## What's Changed
* fix: roll Firefox to 153.0.4 by @​kblok in
hardkoded/puppeteer-sharp#3554
* docs: clarify CdpHttpRequest owns its logger (upstream #​15338 N/A) by
@​kblok in hardkoded/puppeteer-sharp#3552
* feat(tracing): support BufferSize option in Tracing.StartAsync by
@​kblok in hardkoded/puppeteer-sharp#3551
* refactor: track CDP listeners with DisposableActionsStack by @​kblok
in hardkoded/puppeteer-sharp#3555
* feat: roll Chrome to 152.0.7977.42 by @​kblok in
hardkoded/puppeteer-sharp#3553
* Bump version to 25.7.0 by @​kblok in
hardkoded/puppeteer-sharp#3556


**Full Changelog**:
hardkoded/puppeteer-sharp@v25.6.0...v25.7.0

## 25.6.0

## What's Changed
* Replace hand-rolled retry/timeout plumbing with RxSharp by @​kblok in
hardkoded/puppeteer-sharp#3520
* Roll browsers: Chrome 151.0.7922.77, Firefox 153.0.3 by @​kblok in
hardkoded/puppeteer-sharp#3546
* Propagate ILoggerFactory through BiDi sessions, frames and realms by
@​kblok in hardkoded/puppeteer-sharp#3547
* Default BidiRealm logger to NullLogger by @​kblok in
hardkoded/puppeteer-sharp#3550
* Don't fail per-frame CDP fan-out when an OOP iframe goes away by
@​kblok in hardkoded/puppeteer-sharp#3548


**Full Changelog**:
hardkoded/puppeteer-sharp@v25.5.0...v25.6.0

Commits viewable in [compare
view](hardkoded/puppeteer-sharp@v25.5.0...v25.7.0).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=PuppeteerSharp&package-manager=nuget&previous-version=25.5.0&new-version=25.7.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants