fix: session.getBlobData never resolves with blob sizes > 65536 #35277

frank-pian · 2022-08-09T03:17:27Z

Description of Change

Fixes #34398
When the blob data is greater than 65536 in length, remaining_size_ != 0.
My understanding of the Electron code is superficial, but it is feasible after testing.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is 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 session.getBlobData never resolves with blob sizes > 65536

…tron#34398)

welcome · 2022-08-09T03:17:29Z

💖 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.

frank-pian · 2022-08-09T03:19:16Z

@VerteDinde It seems to work

codebytere

@frank-pian please fill out the PR body and describe your changes in enough detail for them to be appropriately reviewed. Release notes are also required. Can this be tested? If yes, we should add a test to prevent regressions.

frank-pian · 2022-08-10T10:44:54Z

@frank-pian please fill out the PR body and describe your changes in enough detail for them to be appropriately reviewed. Release notes are also required. Can this be tested? If yes, we should add a test to prevent regressions.

OK, I'll try to complete the information.
This PR submission process is confusing to me. I hope you can help me complete the test. I just want to fix this bug.
Thanks!

zcbenz

I'm good with this change since it is a trivial one and matches the behavior of Chromium's code:
https://source.chromium.org/chromium/chromium/src/+/main:services/network/data_pipe_element_reader.cc;drc=aeedf4cd0d323c89781ecbe6662c899cdfa8aa81;l=112?q=OnHandleReadable&ss=chromium

But a test is still preferred since there is already a test case written in https://gist.github.com/quantatic/b3b9955084b145c77987e4dbc6c81078.

frank-pian · 2022-08-15T07:44:47Z

@zcbenz
Yes, I have verified the code using this case and it works.

jkleinsc

@frank-pian can you add a test here https://github.com/electron/electron/blob/main/spec-main/api-session-spec.ts#L485 using the gist https://gist.github.com/quantatic/b3b9955084b145c77987e4dbc6c81078? You can copy what was done for the existing session.getBlobData test and just modify to test this fix.

microwugui · 2022-08-22T03:02:00Z

@frank-pian can you add a test here https://github.com/electron/electron/blob/main/spec-main/api-session-spec.ts#L485 using the gist https://gist.github.com/quantatic/b3b9955084b145c77987e4dbc6c81078? You can copy what was done for the existing session.getBlobData test and just modify to test this fix.

I think it's not frank's business
Why not create a new PR by yourself to fix this issue and provide the test cases/docs you are supposed to provide?

frank-pian · 2022-08-22T06:17:49Z

@jkleinsc ok, i have added test case.

zcbenz

Can you rebase to fix the merge conflict?

frank-pian · 2022-08-23T01:53:21Z

Can you rebase to fix the merge conflict?

I merged main branch. Are there any other mistakes?

zcbenz · 2022-08-25T07:05:03Z

/cc @codebytere @jkleinsc

RangerMauve · 2022-09-02T23:39:45Z

This would be very useful to have, running into some issues with uploading files to protocol handlers. 😅

welcome · 2022-09-07T21:47:09Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2022-09-07T21:47:10Z

Release Notes Persisted

Fix session.getBlobData never resolves with blob sizes > 65536

* fix: session.getBlobData never resolves with blob sizes > 65536 (#34398) * Add unit test case for session.getBlobData Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

trop · 2022-09-07T21:47:53Z

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

trop · 2022-09-07T21:47:59Z

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

trop · 2022-09-07T21:48:02Z

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

…36 (electron#35277)" This reverts commit e3efa16.

fix: session.getBlobData never resolves with blob sizes > 65536 (#35277) * fix: session.getBlobData never resolves with blob sizes > 65536 (#34398) * Add unit test case for session.getBlobData Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: Frank Pian <bianyongfang@vip.qq.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

…tron#35277) * fix: session.getBlobData never resolves with blob sizes > 65536 (electron#34398) * Add unit test case for session.getBlobData Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

fix: session.getBlobData never resolves with blob sizes > 65536 (elec…

3d011df

…tron#34398)

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 9, 2022

codebytere changed the title ~~fix: session.getBlobData never resolves with blob sizes > 65536 (#34398)~~ fix: session.getBlobData never resolves with blob sizes > 65536 Aug 9, 2022

codebytere requested changes Aug 9, 2022

View reviewed changes

frank-pian requested a review from codebytere August 10, 2022 10:29

zcbenz added semver/patch backwards-compatible bug fixes target/19-x-y target/21-x-y PR should also be added to the "21-x-y" branch. labels Aug 15, 2022

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 15, 2022

zcbenz approved these changes Aug 15, 2022

View reviewed changes

jkleinsc requested changes Aug 15, 2022

View reviewed changes

Add unit test case for session.getBlobData

dc07695

frank-pian requested review from jkleinsc and removed request for codebytere August 22, 2022 06:18

zcbenz reviewed Aug 23, 2022

View reviewed changes

Merge branch 'main' into my-branch

c9f25e7

codebytere approved these changes Aug 29, 2022

View reviewed changes

Merge branch 'main' into my-branch

b42f44f

jkleinsc approved these changes Sep 7, 2022

View reviewed changes

jkleinsc merged commit e3efa16 into electron:main Sep 7, 2022

trop bot mentioned this pull request Sep 7, 2022

fix: session.getBlobData never resolves with blob sizes > 65536 #35600

Merged

This was referenced Sep 7, 2022

fix: session.getBlobData never resolves with blob sizes > 65536 #35601

Merged

fix: session.getBlobData never resolves with blob sizes > 65536 #35602

Merged

trop bot added in-flight/19-x-y and removed target/19-x-y labels Sep 7, 2022

trop bot added the in-flight/20-x-y label Sep 7, 2022

trop bot added in-flight/21-x-y and removed target/20-x-y target/21-x-y PR should also be added to the "21-x-y" branch. labels Sep 7, 2022

Pink021 added a commit to Pink021/electron that referenced this pull request Sep 8, 2022

Revert "fix: session.getBlobData never resolves with blob sizes > 655…

7659a69

…36 (electron#35277)" This reverts commit e3efa16.

Pink021 mentioned this pull request Sep 8, 2022

Revert "fix: session.getBlobData never resolves with blob sizes > 65536" #35606

Closed

trop bot added merged/19-x-y and removed in-flight/19-x-y labels Sep 8, 2022

trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Sep 8, 2022

trop bot added the merged/20-x-y label Sep 8, 2022

trop bot removed the in-flight/20-x-y label Sep 8, 2022

commit-lint bot mentioned this pull request Dec 13, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: session.getBlobData never resolves with blob sizes > 65536 #35277

fix: session.getBlobData never resolves with blob sizes > 65536 #35277

frank-pian commented Aug 9, 2022 •

edited

welcome bot commented Aug 9, 2022

frank-pian commented Aug 9, 2022

codebytere left a comment

frank-pian commented Aug 10, 2022

zcbenz left a comment

frank-pian commented Aug 15, 2022

jkleinsc left a comment

microwugui commented Aug 22, 2022 •

edited

frank-pian commented Aug 22, 2022

zcbenz left a comment

frank-pian commented Aug 23, 2022

zcbenz commented Aug 25, 2022

RangerMauve commented Sep 2, 2022

welcome bot commented Sep 7, 2022

release-clerk bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

fix: session.getBlobData never resolves with blob sizes > 65536 #35277

fix: session.getBlobData never resolves with blob sizes > 65536 #35277

Conversation

frank-pian commented Aug 9, 2022 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Aug 9, 2022

frank-pian commented Aug 9, 2022

codebytere left a comment

Choose a reason for hiding this comment

frank-pian commented Aug 10, 2022

zcbenz left a comment

Choose a reason for hiding this comment

frank-pian commented Aug 15, 2022

jkleinsc left a comment

Choose a reason for hiding this comment

microwugui commented Aug 22, 2022 • edited

frank-pian commented Aug 22, 2022

zcbenz left a comment

Choose a reason for hiding this comment

frank-pian commented Aug 23, 2022

zcbenz commented Aug 25, 2022

RangerMauve commented Sep 2, 2022

welcome bot commented Sep 7, 2022

release-clerk bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

trop bot commented Sep 7, 2022

frank-pian commented Aug 9, 2022 •

edited

microwugui commented Aug 22, 2022 •

edited