How to proceed with patch for MDNS fix #9630
Comments
|
@RangerMauve Could you share the patch here first so we could take a look to see how to minimize the patching? I suppose yours are different from the 2017 one. |
|
Hey! Here's the git diff for my patch. It's a little different, but it's not a lot of code overall, I tried to follow the style conventions of the files I modified. diff --git a/extensions/browser/api/socket/udp_socket.cc b/extensions/browser/api/socket/udp_socket.cc
index 0f129a64cbb9..46819d3165b8 100644
--- a/extensions/browser/api/socket/udp_socket.cc
+++ b/extensions/browser/api/socket/udp_socket.cc
@@ -74,6 +74,7 @@ void UDPSocket::Bind(const std::string& address,
std::move(callback).Run(net::ERR_INVALID_ARGUMENT);
return;
}
+
socket_->Bind(ip_end_point, std::move(socket_options_),
base::BindOnce(&UDPSocket::OnBindCompleted,
base::Unretained(this), std::move(callback)));
@@ -381,6 +382,13 @@ void UDPSocket::SetBroadcast(bool enabled,
socket_->SetBroadcast(enabled, std::move(callback));
}
+int UDPSocket::SetAllowAddressReuse(bool allowed) {
+ if (!socket_options_)
+ return net::ERR_SOCKET_IS_CONNECTED;
+ socket_options_->allow_address_reuse = allowed;
+ return net::OK;
+}
+
const std::vector<std::string>& UDPSocket::GetJoinedGroups() const {
return multicast_groups_;
}
diff --git a/extensions/browser/api/socket/udp_socket.h b/extensions/browser/api/socket/udp_socket.h
index bc655989882c..433c3a56943a 100644
--- a/extensions/browser/api/socket/udp_socket.h
+++ b/extensions/browser/api/socket/udp_socket.h
@@ -61,6 +61,9 @@ class UDPSocket : public Socket, public network::mojom::UDPSocketListener {
// Sets broadcast to |enabled|. Can only be called after a successful Bind().
void SetBroadcast(bool enabled, net::CompletionOnceCallback callback);
+ // Address reuse options must be set before Bind()/Connect() is called.
+ int SetAllowAddressReuse(bool allowed);
+
const std::vector<std::string>& GetJoinedGroups() const;
protected:
diff --git a/extensions/browser/api/sockets_udp/sockets_udp_api.cc b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
index aedfe02ea34f..b8db6124bcd4 100644
--- a/extensions/browser/api/sockets_udp/sockets_udp_api.cc
+++ b/extensions/browser/api/sockets_udp/sockets_udp_api.cc
@@ -566,5 +566,30 @@ void SocketsUdpSetBroadcastFunction::OnCompleted(int net_result) {
AsyncWorkCompleted();
}
+SocketsUdpSetAllowAddressReuseFunction::
+ SocketsUdpSetAllowAddressReuseFunction() {}
+
+SocketsUdpSetAllowAddressReuseFunction::
+ ~SocketsUdpSetAllowAddressReuseFunction() {}
+
+bool SocketsUdpSetAllowAddressReuseFunction::Prepare() {
+ params_ = api::sockets_udp::SetAllowAddressReuse::Params::Create(*args_);
+ EXTENSION_FUNCTION_VALIDATE(params_.get());
+ return true;
+}
+
+void SocketsUdpSetAllowAddressReuseFunction::Work() {
+ ResumableUDPSocket* socket = GetUdpSocket(params_->socket_id);
+ if (!socket) {
+ error_ = kSocketNotFoundError;
+ return;
+ }
+
+ int net_result = socket->SetAllowAddressReuse(params_->allowed);
+ if (net_result != net::OK)
+ error_ = net::ErrorToString(net_result);
+ results_ = sockets_udp::SetAllowAddressReuse::Results::Create(net_result);
+}
+
} // namespace api
} // namespace extensions
diff --git a/extensions/browser/api/sockets_udp/sockets_udp_api.h b/extensions/browser/api/sockets_udp/sockets_udp_api.h
index ad9b06f28cde..a10eb22e358e 100644
--- a/extensions/browser/api/sockets_udp/sockets_udp_api.h
+++ b/extensions/browser/api/sockets_udp/sockets_udp_api.h
@@ -250,6 +250,25 @@ class SocketsUdpSetMulticastTimeToLiveFunction
std::unique_ptr<sockets_udp::SetMulticastTimeToLive::Params> params_;
};
+class SocketsUdpSetAllowAddressReuseFunction
+ : public UDPSocketAsyncApiFunction {
+ public:
+ DECLARE_EXTENSION_FUNCTION("sockets.udp.setAllowAddressReuse",
+ SOCKETS_UDP_SETALLOWADDRESSREUSE)
+
+ SocketsUdpSetAllowAddressReuseFunction();
+
+ protected:
+ ~SocketsUdpSetAllowAddressReuseFunction() override;
+
+ // AsyncApiFunction
+ bool Prepare() override;
+ void Work() override;
+
+ private:
+ std::unique_ptr<sockets_udp::SetAllowAddressReuse::Params> params_;
+};
+
class SocketsUdpSetMulticastLoopbackModeFunction
: public UDPSocketAsyncApiFunction {
public:
diff --git a/extensions/browser/extension_function_histogram_value.h b/extensions/browser/extension_function_histogram_value.h
index ffd2d40c50ab..1e64f88fcd61 100644
--- a/extensions/browser/extension_function_histogram_value.h
+++ b/extensions/browser/extension_function_histogram_value.h
@@ -1500,6 +1500,7 @@ enum HistogramValue {
PRINTING_CANCELJOB = 1437,
WEBCAMPRIVATE_RESTORE_CAMERA_PRESET = 1449,
WEBCAMPRIVATE_SET_CAMERA_PRESET = 1450,
+ SOCKETS_UDP_SETALLOWADDRESSREUSE = 1451,
// Last entry: Add new entries above, then run:
// python tools/metrics/histograms/update_extension_histograms.py
ENUM_BOUNDARY = 1440
diff --git a/extensions/common/api/sockets_udp.idl b/extensions/common/api/sockets_udp.idl
index 784a31d25d77..696756dd5db2 100644
--- a/extensions/common/api/sockets_udp.idl
+++ b/extensions/common/api/sockets_udp.idl
@@ -124,6 +124,11 @@ namespace sockets.udp {
// A negative value indicates an error.
callback SetMulticastLoopbackModeCallback = void (long result);
+ // Callback from the <code>setAllowAddressReuse</code> method.
+ // |result| : The result code returned from the underlying network call.
+ // A negative value indicates an error.
+ callback SetAllowAddressReuseCallback = void (long result);
+
// Callback from the <code>getJoinedGroupsCallback</code> method.
// |groups| : Array of groups the socket joined.
callback GetJoinedGroupsCallback = void (DOMString[] groups);
@@ -290,6 +295,15 @@ namespace sockets.udp {
boolean enabled,
SetMulticastLoopbackModeCallback callback);
+ // Sets whether the socket should allow address reuse.
+ // |socketId| : The socket ID.
+ // |allowed| : Indicate whether to allow or disallow address reuse.
+ // |callback| : Called when the configuration operation completes.
+ static void setAllowAddressReuse(
+ long socketId,
+ boolean allowed,
+ SetAllowAddressReuseCallback callback);
+
// Gets the multicast group addresses the socket is currently joined to.
// |socketId| : The socket ID.
// |callback| : Called with an array of strings of the result.
This will also require running |
|
Hey! Just wanted to follow up and see if there was anything else I could do for this. |
|
For That being said, it seems like a good candidate to try to push to upstream so these patches could go away. The old patch seems to be almost got accepted by chromium but not due to fail on mac, which I suppose we don't have that issue anymore, right? |
|
Awesome. I don't have a mac handy right now but I'll ask around to see if anyone in my city is willing to let me use theirs. I'll report after I've test it and see about cleaning up the patch from there. 😁 |
|
Question: Is there a way for me to cross-compile to make a Mac build from my Linux machine? I've got my hands on a Mac I could use but I don't think it has space. If not I'll try to look for other macs with more space on them. :P |
|
Trying to run |
|
Gave up trying to source a laptop in meatspace. Gonna try compiling a binary on my machine and giving instructions to someone virtually. 😁 |
|
I tried building a release version for mac, the |
|
Ah, just found the create_dist command. Might be a good to have that somewhere a bit more visible in the Wiki. |
|
Is cross compiling from linux even possible? It seems target_os=mac was wrong and I needed to use target_os=darwin. Not sure if that'll work, and it looks like this'll need another three hours of compilation. 😅 |
|
Hey @yrliou I've made a branch of brave-core with my patches. https://github.com/RangerMauve/brave-core/tree/udp-fix Would it be possible to trigger the regular brave CI to compile a DMG using them so I can send it out for tests? |
|
@RangerMauve a test PR is created to run CI, it failed because we just merged chromium 83 into master, please rebase on master and update your patch file if needed and push to the branch again, thanks. |
|
Will do, thank you! I've also got my hands on a macbook so I can test this out once the DMG is built. 😁 |
|
@yrliou I just pushed the rebased version to my fork. Would you be able to update the branch for the CI when you have time? 😁 |
|
@RangerMauve Failed with below, was your local build successful after rebasing? You might need to run |
|
Ah dang. I'll check it out Thank you! |
|
Histagram was needed for adding the new JS method to the API. Though I could probably move the flag into the constructor instead? |
|
IIUC the patch from 2017 used arg: @RangerMauve would doing something similar (instead of a new API method) work for us in the context of local discovery in Brave? |
|
@lidel Yeah, I was initially departing from that because of the way the multicast APIs were configuring the settings, but I think that adding it inside |
|
K, reworked the API to have |
|
@yrliou Would you be able to get my latest changes into the CI when you have a chance? Thank you so much for your patience with all this BTW. 💜 |
|
@RangerMauve np, pushed. |
|
Tested on Mac, getting the error |
|
Looking into where the flag is being used in the networking code. It looks like on Linux it's using the |
|
Gonna see how Node.js handles this case since they've got it working somehow. |
|
Here's how libuv handles it: https://github.com/nodejs/node/blob/3abb52fdb683c9c9ade1b2c7d16d0f640bbaacfd/deps/uv/src/unix/udp.c#L487 Here's how chromium handles it: https://chromium.googlesource.com/chromium/src/net/+/refs/heads/master/socket/socket_options.cc#33 Still doing some research to actually understand what's going on, but it seems I can do a check for |
|
K, I've added another patch to my branch. It appears to be compiling fine on linux still. 😅 @yrliou would you mind building another dmg when you have a chance? 😁 |
|
@RangerMauve np, pushed. |
|
Weeeird, it doesn't seem to have helped even though it's pretty much the same code as libuv. 🙃 Just pushed out another patch which uses the exact same variable as the libuv code, maybe that'll help. 🤷 |
|
Hmm, actually, looking at the code for the multicast-dns module, it could be that we need to bind to a specific network interface for things to work. https://github.com/mafintosh/multicast-dns/blob/master/index.js#L157 Gonna investigate why it's there tomorrow. No need to do another build in the meantime. 😁 |
|
Seems I'll need to make use of the network interface API to get this working properly on Mac. https://developer.chrome.com/apps/system_network Are there any blockers for adding it to the safelist in the ipfs-companion extension? |
bbondy
added
priority/P3
|
I believe this can be closed @bbondy Google decided to deprecate Chrome Apps APIs, so we had to deprioritize js-ipfs+chromesockets in favor of more future-proof go-ipfs (see #10220) Still, thank you again for all the research that went into figuring this out @RangerMauve @yrliou 👍 |
|
ok thanks. |
Description
I'm currently working on fixing the MDNS peer discovery issue in ipfs-companion.
In order to fix this I discovered that we needed to change Chrome's UDP socket API to have a flag to enable address reuse which is the default in Node.js's UDP API.
To that end, I've added a new api called
chrome.sockets.udp.setAllowAddressReusewhich is inspired by this chromium patch from 2017.I've got it working and it's fixing the issue, but I'm unsure as to how to proceed with submitting the patch to Brave.
@lidel linked me to this guide and I was thinking of using the preprocessor method in order to add the lines I needed in
brave-core/chromium_src/, but I'm not sure if that's the best way forward.Some guidance on what the best approach would be to get this patch accepted would be very much appreciated.
Steps to Reproduce
Actual result:
There's an error in the console complaining about address reuse
Expected result:
They should connect
Reproduces how often:
n/a
Brave version (brave://version info)
n/a
Version/Channel Information:
n/a
Other Additional Information:
Miscellaneous Information:
The changes mostly affect the
extensions/folder of the SRC, and I'm a bit worried about the change that was needed inextensions/browser/extension_function_histogram_value.hsince it seems values there shouldn't conflict with chromium.The text was updated successfully, but these errors were encountered: