net/http: correct use of byte slice in js syscall #26350
Conversation
googlebot
added
the
cla: yes
|
Message from Gerrit User 13620: Patch Set 1: Code-Review-2 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 12446: Uploaded patch set 3: New patch set was added with same tree, parent, and commit message as Patch Set 2. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 2: (1 comment) Changed to defer the release of the array. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5065: Patch Set 3: So apparently we have no test coverage for any of this? Can we fix that and make this code actually used on the builders? Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 3:
Indeed, there are no tests for this right now. I'd love to write some tests but I've no idea where to start really. Node does not have native support for the Fetch API AFAIK so we can't just run it in that interpreter. Any ideas? Richard? Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 13620: Patch Set 3: Code-Review+1 (1 comment)
The package https://www.npmjs.com/package/node-fetch would help with that. I'm using it a lot, it is good. We would need to somehow preinstall it on the builders. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
syscall/js does not allow []byte to be used in direct inputs to its JavaScript manipulation methods since bafe466. Unfortunately, this use of a byte slice was missed, so any uses of the WASM Roundtripper with a body will panic. This ensures the byte slice is appropriately converted before being passed to syscall. Fixes #26349
|
Message from Gerrit User 12446: Uploaded patch set 5: New patch set was added with same tree, parent, and commit message as Patch Set 4. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 3: (1 comment)
If we can configure the runners to install this package I can start writing some tests for it, assuming the runners will be patched by the time I've written them. Brad, is there something I can do to configure the runners with this package? Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5137: Patch Set 5: Thanks Johan for the quick fix! Could we perhaps add a test here to ensure that we never regress? Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5065: Patch Set 5: I left a comment at #26051 (comment) on the bug about setting up a browser-based builder. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 5:
So should we hold off this fix until such a builder is available or should we merge this and add tests as soon as possible? I'd argue that we need to get thisin before the next beta release since I expect wasm users will face this bug pretty quickly. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5137: Patch Set 5: Run-TryBot+1
Johan, roger that. I think in the meantime we can merge this change but I'll open up an issue for when browser-based builders are available and tag this CL as well as Brad's ping. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5976: Patch Set 5: TryBots beginning. Status page: https://farmer.golang.org/try?commit=9342bb38 Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5137: Patch Set 5:
Filed an issue at #26358 for us to add such tests. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5976: Patch Set 5: TryBot-Result-1 1 of 19 TryBots failed: Consult https://build.golang.org/ to see whether they are new failures. Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 5:
Looks like a flaky test failure, could we have a run please? Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 13620: Patch Set 5: Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 5065: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
Message from Gerrit User 16585: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
syscall/js does not allow []byte to be used in direct inputs to its JavaScript manipulation methods since bafe466. Unfortunately, this use of a byte slice was missed, so any uses of the WASM Roundtripper with a body will panic. This ensures the byte slice is appropriately converted before being passed to syscall. Fixes #26349 Change-Id: I83847645d71ce310c1eee3decddbac990fae166b GitHub-Last-Rev: 3914bda GitHub-Pull-Request: #26350 Reviewed-on: https://go-review.googlesource.com/123537 Run-TryBot: Emmanuel Odeke <[email protected]> Reviewed-by: Richard Musiol <[email protected]> Reviewed-by: Brad Fitzpatrick <[email protected]>
|
Message from Gerrit User 5065: Patch Set 5: Code-Review+2 (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/123537. |
|
This PR is being closed because golang.org/cl/123537 has been merged. |
syscall/js does not allow []byte to be used in direct inputs to
its JavaScript manipulation methods since
bafe466.
Unfortunately, this use of a byte slice was missed, so any
uses of the WASM Roundtripper with a body will panic.
This ensures the byte slice is appropriately converted
before being passed to syscall.
Fixes #26349