fix: properly stream `uploadData` in `protocol.handle()` #41052

BurningEnlightenment · 2024-01-18T21:18:29Z

Description of Change

Fixes a few annoying bugs which prevented requests intercepted with protocol.handle() from being properly forwarded to net.fetch():
Fixes #39658
Fixes #40754
Fixes #40826

I considered submitting multiple pull requests, but the fixes would've generated conflicts with each other. In essence I've just streamlined the pull() implementation of convertToRequestBody() and added a test case for every fixed bug. I also added an explicit error condition in case an unknown/new chunk type gets pushed into the protocol handler. Otherwise it is very
unobvious and hard to figure out why your request has been mutilated.

If you like, I am willing to fix the chunk type definitions / remove the any casts. However, I'd need some guidance on whether I should introduce local types or add them to the public API.

Additionally I noticed while reviewing the C++ parts that the cb, which is passed to the actual handler function async (preq: ProtocolRequest, cb: any), never accesses the statusText property. Is there a reason for passing statusText in anyways?

cc: @nornagon, @codebytere

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fix various bugs which could prevent forwarding requests intercepted with protocol.handle().

welcome · 2024-01-18T21:18:32Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codebytere · 2024-01-19T08:55:30Z

hey @BurningEnlightenment - thanks for this! We'll try to get to reviewing it soon - @nornagon is out until the end of the month so we might want to wait until he can take a look as well.

BurningEnlightenment · 2024-01-19T10:02:22Z

No pressure, I'm applying these fixes currently via monkey patching protocol.handle().

Some additional thoughts:

It would probably be nicer to disentangle the stream-flattening from the chunk => stream conversion instead of doing it all in pull().
We should probably forward cancel events to the inner stream, otherwise a file handle might not get closed until GC kicks in.
Make a type: 'bytes' stream and supply an explicit backpressure strategy.
Adding support for BYOBReader could reduce memory pressure in certain scenarios

ckerr · 2024-01-22T23:16:32Z

Additionally I noticed while reviewing the C++ parts that the cb, which is passed to the actual handler function async (preq: ProtocolRequest, cb: any), never accesses the statusText property. Is there a reason for passing statusText in anyways?

@codebytere and @nornagon know the context better than me but it looks like statusText was added in the initial feature commit @ #36674 but AFAICT isn't used or documented. So maybe we should remove it?

nornagon

Thanks, these are good changes (and good tests!)

Regarding the unused statusText property, good catch, that should probably be plumbed through to the underlying HttpResponseHeaders—currently we use net::GetHttpReasonPhrase to derive the status text from the status code instead.

nornagon · 2024-02-02T00:11:13Z

lib/browser/api/protocol.ts

+          // Note that even though `getBlobData()` is a `Session` API, it doesn't
+          // actually use the `Session` context. Its implementation solely relies
+          // on global variables which allows us to implement this feature without
+          // knowledge of the `Session` associated with the current request by
+          // always pulling `Blob` data out of the default `Session`.
+          controller.enqueue(await session.defaultSession.getBlobData(chunk.blobUUID));


I feel like this should be fixed to not be an instance method then 😅 but won't block on it.

I agree with you. I just wanted to sidestep a potential API design review process (and it would've bumped this from semver/patch to at least a semver/minor change).

Btw. if the getBlobData() API gets touched it might also be prudent to rename blobUUID to just blobId as the implementation hasn't used UUIDs for quite some time. Should I submit a follow up PR potentially including some further changes outlined in my previous comments?

codebytere · 2024-02-14T10:15:15Z

@BurningEnlightenment would you mind rebasing this to fix CI? there was an issue with WOA CI that's since been addressed. Beyond that:

Should I submit a follow up PR potentially including some further changes outlined in my previous comments?

Yes, I think your best course of action is to open a follow-up! The scope of this PR is good to merge for now and i'd rather address the issues herein first.

Increase readability by moving the file stream creation logic out of the `uploadData` to request body conversion function.

Refs: electron#39658

Refs: electron#40754

Refs: electron#38929

Refs: electron#40826

BurningEnlightenment · 2024-02-14T16:49:21Z

@codebytere done. LGTM

welcome · 2024-02-16T19:29:32Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2024-02-16T19:29:32Z

Release Notes Persisted

Fix various bugs which could prevent forwarding requests intercepted with protocol.handle().

trop · 2024-02-16T19:30:17Z

I have automatically backported this PR to "28-x-y", please check out #41358

trop · 2024-02-16T19:30:23Z

I have automatically backported this PR to "29-x-y", please check out #41359

This was primarily for getting the latest Electron, but I ran `yarn upgrade-interactive` and upgraded the other non-breaking deps (mostly dev) too. Reason for wanting electron is to try and see if this backport fixes the issue with our streams not getting faithfully written: electron/electron#41052 In some ad-hoc and quick testing, I noticed that the new `writeStream` we've implemented works fine for files up to 128 K, presumably some chunk size, but then begins to diverge. Sounds similar (but not exactly the same) as this issue: electron/electron#39658 Unfortunately, this didn't fix the issue we're facing, so our case is perhaps different.

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 18, 2024

codebytere requested review from nornagon and codebytere January 19, 2024 08:55

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 25, 2024

nornagon approved these changes Feb 2, 2024

View reviewed changes

nornagon added target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. semver/patch backwards-compatible bug fixes component/protocol labels Feb 2, 2024

zcbenz approved these changes Feb 8, 2024

View reviewed changes

zcbenz force-pushed the dev/protocol_handle_refactor branch from 51031a5 to 50817e2 Compare February 8, 2024 01:49

BurningEnlightenment added 6 commits February 14, 2024 17:47

refactor(protocol): extract file stream factory

0707eb5

Increase readability by moving the file stream creation logic out of the `uploadData` to request body conversion function.

fix: properly flatten streams in protocol.handle()

e98785a

Refs: electron#39658

fix: protocol.handle() filter null origin header

03d5b66

Refs: electron#40754

fix: remove obsolete TODO comment

5e9af2c

Refs: electron#38929

fix: forward Blob parts in protocol.handle()

ce67224

Refs: electron#40826

fix: explicitly error out on unknown chunk parts

885ae8c

BurningEnlightenment force-pushed the dev/protocol_handle_refactor branch from 50817e2 to 885ae8c Compare February 14, 2024 16:47

jkleinsc approved these changes Feb 16, 2024

View reviewed changes

jkleinsc merged commit 80906c0 into electron:main Feb 16, 2024
16 checks passed

trop bot mentioned this pull request Feb 16, 2024

fix: properly stream uploadData in protocol.handle() #41358

Closed

trop bot mentioned this pull request Feb 16, 2024

fix: properly stream uploadData in protocol.handle() #41359

Merged

trop bot added in-flight/28-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. labels Feb 16, 2024

trop bot added in-flight/29-x-y merged/29-x-y PR was merged to the "29-x-y" branch. and removed target/29-x-y PR should also be added to the "29-x-y" branch. in-flight/29-x-y labels Feb 16, 2024

trop bot removed the in-flight/28-x-y label Mar 25, 2024

mrchaofan mentioned this pull request Apr 25, 2024

feat: Add support to callback in protocol interceptor handler without argument to perform as if no interceptors. #41929

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: properly stream `uploadData` in `protocol.handle()` #41052

fix: properly stream `uploadData` in `protocol.handle()` #41052

BurningEnlightenment commented Jan 18, 2024

welcome bot commented Jan 18, 2024

codebytere commented Jan 19, 2024

BurningEnlightenment commented Jan 19, 2024

ckerr commented Jan 22, 2024

nornagon left a comment

nornagon Feb 2, 2024

BurningEnlightenment Feb 2, 2024

codebytere commented Feb 14, 2024

BurningEnlightenment commented Feb 14, 2024

welcome bot commented Feb 16, 2024

release-clerk bot commented Feb 16, 2024

trop bot commented Feb 16, 2024

trop bot commented Feb 16, 2024

fix: properly stream uploadData in protocol.handle() #41052

fix: properly stream uploadData in protocol.handle() #41052

Conversation

BurningEnlightenment commented Jan 18, 2024

Description of Change

Checklist

Release Notes

welcome bot commented Jan 18, 2024

codebytere commented Jan 19, 2024

BurningEnlightenment commented Jan 19, 2024

ckerr commented Jan 22, 2024

nornagon left a comment

Choose a reason for hiding this comment

nornagon Feb 2, 2024

Choose a reason for hiding this comment

BurningEnlightenment Feb 2, 2024

Choose a reason for hiding this comment

codebytere commented Feb 14, 2024

BurningEnlightenment commented Feb 14, 2024

welcome bot commented Feb 16, 2024

release-clerk bot commented Feb 16, 2024

trop bot commented Feb 16, 2024

trop bot commented Feb 16, 2024

fix: properly stream `uploadData` in `protocol.handle()` #41052

fix: properly stream `uploadData` in `protocol.handle()` #41052