WebTorrent Support #356

yrliou · 2018-08-23T20:39:58Z

Fix brave/brave-browser#289.

What's included in this PR:

WebTorrent support of magnet links

What's not included in this PR:

WebTorrent support of .torrent links (WebTorrent support of .torrent links brave-browser#855)
Support of trackless torrents using bittorrent-dht/client (Enable DHT client in WebTorrent for supporting trackless torrents brave-browser#857)
l10n (Add l10n support for webtorrent component extension brave-browser#858)

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Manual Test:

Open a new tab and visit https://webtorrent.io/free-torrents, click magnet link of Big Buck Bunny should leads to a torrent viewer page without start downloading
Click start download, torrent should start to be downloading
Click any link under save file should be able to download that specific file
Click on poster.jpg, should open a tab and show the image directly
Go back to the torrent downloading page and click on Big Buck Bunny.mp4, should open a tab to stream the media
(Note: It might take a while for the media to be ready to play.)
Close the media streaming tab and go back to the torrent downloading page
Go back to https://webtorrent.io/free-torrents tab and see if it also works by right click the magnet link to open it in a new tab

Automation Test:
npm run test -- brave_browser_tests --filter=BraveContentBrowserClientTest.RewriteMagnetURL*
npm run test-unit

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

yrliou · 2018-08-23T20:42:00Z

patches/net-url_request-url_request_context_builder.cc.patch

+   }
+ #endif  // !BUILDFLAG(DISABLE_FTP_SUPPORT)
+
+  job_factory->SetProtocolHandler("magnet", std::make_unique<MagnetProtocolHandler>());


#353 (review)
This review comment is not yet addressed.

@bridiver Addressed by 3ee9fa7

yrliou · 2018-08-26T19:51:28Z

patches/extensions-browser-api-socket-tcp_socket.cc.patch

+   // TODO(devlin): Should we do this for all callbacks?
+  if (!connect_callback_.is_null()) {
+    base::ResetAndReturn(&connect_callback_)
+        .Run(net::ERR_CONNECTION_CLOSED);


Solve DCHECK erorr at https://cs.chromium.org/chromium/src/extensions/browser/extension_function.cc?sq=package:chromium&g=0&l=504.
Webtorrent hit the DCHECK where SocketsTcpConnectFunction is destructed before any responses are made.
It happens because it is possible for the application to make a request to destroy the socket while previous connect request is still in progress.
This DCHECK is introduced by https://codereview.chromium.org/2017113002, in the same changeset, read_callback is reset and return immediately here.

yrliou · 2018-08-27T16:32:01Z

I'm rebasing on C70, hit a new DCHECK in CreateTCPConnectedSocketCallback, related change: https://chromium.googlesource.com/chromium/src/+/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9

yrliou · 2018-08-27T18:37:35Z

DCHECK for CreateTCPConnectedSocketCallback has been fixed in chromium master, I'll pull it in first. https://chromium.googlesource.com/chromium/src/+/602a66794ef884ce0f961e9c625c649704cfcb83%5E%21/

yrliou · 2018-08-27T19:20:19Z

patches/extensions-browser-api-socket-tcp_socket.cc.patch

+  if (connect_callback_) {
+    std::move(connect_callback_)
+        .Run(net::ERR_CONNECTION_CLOSED);
+  }


Solve DCHECK erorr at https://cs.chromium.org/chromium/src/extensions/browser/extension_function.cc?sq=package:chromium&g=0&l=504.
Webtorrent hit the DCHECK where SocketsTcpConnectFunction is destructed before any responses are made.
It happens because it is possible for the application to make a request to destroy the socket while the connect request is still in progress.
This DCHECK is introduced by https://codereview.chromium.org/2017113002, in the same changeset, read_callback is reset and return immediately here.

what is causing SocketsTcpConnectFunction to be destroyed early?

add for accept_callback_ as well as discussed and please submit to upstream

Addressed in 360746b

yrliou · 2018-08-27T19:21:07Z

patches/services-network-tcp_connected_socket.cc.patch

+
+-TCPConnectedSocket::~TCPConnectedSocket() {}
+TCPConnectedSocket::~TCPConnectedSocket() {
+  if (connect_callback_) {


This is pulling from https://chromium.googlesource.com/chromium/src/+/602a66794ef884ce0f961e9c625c649704cfcb83%5E%21/
This patch should be able to be removed in the next chromium upgrade.

I'm fine with the upstream patch, but odd that this doesn't call Disconnect?

This is for CreateTCPConnectedSocketCallback introduced in C70 but not the extension function itself.

Without this, we'll still hit DCHECK in src/out/Debug/gen/services/network/public/mojom/network_context.mojom.cc,
which checks if CreateTCPConnectedSocketCallback is destructed without respond.

bridiver · 2018-08-27T19:32:27Z

browser/extensions/brave_extension_provider.cc

@@ -126,7 +127,8 @@ bool BraveExtensionProvider::UserMayLoad(const Extension* extension,

 bool BraveExtensionProvider::MustRemainInstalled(const Extension* extension,
                                                 base::string16* error) const {
-  return extension->id() == brave_extension_id;
+  return extension->id() == brave_extension_id ||


should this be here? Do we want it always loaded by default or on user activation?

I would prefer loaded by default for now if that's OK.
And address it in a follow-up issue to make it able to be turned on and off by user at anytime.

AFAIK, if we want to disable a component extension from UI, it should probably go through chrome://extensions.
First, to see component extensions listed, we'll need to pass --show-component-extension-options when run it.
But it seems I can't toggle the on&off switch even if the component extension is shown for our current version.
I would like to address this in a separate issue.
@bridiver wdyt?

btw, it seems it is still loaded by default even when I remove this line.

this doesn't affect default loading, it only affects the ability to uninstall

bridiver

c++ stuff looks good

cezaraugusto

code comments left, mostly nit. No show-stoppers. Unable to test it manually ATM as I'm still compiling

cezaraugusto · 2018-08-29T16:37:14Z

components/brave_webtorrent/extension/background.ts

+
+require('./background/store')
+require('./background/events/tabsEvents')
+require('./background/events/windowsEvents')


nit: don't import './background/events/windowsEvents' do the job as well? also init() could go at the bottom

components/brave_webtorrent/extension/components/torrentFileList.tsx

components/brave_webtorrent/extension/components/torrentStatus.tsx

components/brave_webtorrent/extension/components/torrentFileList.tsx

components/brave_webtorrent/extension/components/torrentStatus.tsx

components/brave_webtorrent/extension/components/torrentViewerHeader.tsx

components/brave_webtorrent/extension/manifest.json

yrliou · 2018-08-30T07:22:47Z

@cezaraugusto Please review 970343f and a0bb5ec.
I'll do a final rebase tomorrow, and will test builds on all platforms again before merging.
Please leave merge to me, thanks!

cezaraugusto

+++++++ for the front-end

yrliou · 2018-08-30T20:05:03Z

@bridiver @cezaraugusto Thanks a lot for the review, I'll squash some commits together and merge this PR after I finish testing it on all platforms.

…onnected. Solve DCHECK erorr in ~UIThreadExtensionFunction where SocketsTcpConnectFunction is destructed before any responses were made.

…e run. This patch is pulling in the solution from https://chromium.googlesource.com/chromium/src/+/602a66794ef884ce0f961e9c625c649704cfcb83%5E%21/ Should be removed when this changeset is pulled in next chromium upgrade.

yrliou · 2018-08-30T21:47:07Z

Seems fine on Mac and Linux, waiting on windows build.

yrliou · 2018-08-31T01:05:42Z

Run manual and browser tests on all platforms, seems fine, merging.

bbondy · 2018-08-31T19:42:59Z

WOOoooo 🎉 and :party_parrot:

yrliou self-assigned this Aug 23, 2018

yrliou requested review from bridiver and bbondy August 23, 2018 20:39

yrliou commented Aug 23, 2018

View reviewed changes

yrliou force-pushed the webtorrent_ext branch 4 times, most recently from b486a2f to a846eae Compare August 26, 2018 19:33

yrliou commented Aug 26, 2018

View reviewed changes

yrliou force-pushed the webtorrent_ext branch from 0448b59 to de5581b Compare August 26, 2018 23:19

yrliou force-pushed the webtorrent_ext branch from de5581b to 096f48a Compare August 27, 2018 19:18

yrliou commented Aug 27, 2018

View reviewed changes

yrliou changed the title ~~[WIP] WebTorrent Support~~ WebTorrent Support Aug 27, 2018

bridiver reviewed Aug 27, 2018

View reviewed changes

yrliou force-pushed the webtorrent_ext branch from 17cd247 to faa47bc Compare August 27, 2018 22:26

yrliou mentioned this pull request Aug 27, 2018

Use webpack to solve browser field issue in webtorrent and bittorrent-tracker brave/brave-browser#856

Closed

yrliou requested a review from cezaraugusto August 28, 2018 04:10

bridiver reviewed Aug 28, 2018

View reviewed changes

cezaraugusto reviewed Aug 29, 2018

View reviewed changes

yrliou force-pushed the webtorrent_ext branch from 930e8d0 to a0bb5ec Compare August 30, 2018 07:19

yrliou force-pushed the webtorrent_ext branch from a0bb5ec to ecb32a6 Compare August 30, 2018 19:33

cezaraugusto previously approved these changes Aug 30, 2018

View reviewed changes

yrliou dismissed cezaraugusto’s stale review via a054df3 August 30, 2018 21:20

yrliou force-pushed the webtorrent_ext branch from ecb32a6 to a054df3 Compare August 30, 2018 21:20

Update package.json

a525d5b

yrliou added 5 commits August 30, 2018 14:20

Implement webtorrent component

4ebe7e4

Run pending connect and accept callbacks immediately when socket disc…

3d47e30

…onnected. Solve DCHECK erorr in ~UIThreadExtensionFunction where SocketsTcpConnectFunction is destructed before any responses were made.

Add basic frontend unit tests

f959d2e

Address frontend review comments and adapt brave-ui upgrade

4267741

yrliou force-pushed the webtorrent_ext branch from a054df3 to 4267741 Compare August 30, 2018 21:21

cezaraugusto approved these changes Aug 30, 2018

View reviewed changes

yrliou merged commit e5e2c89 into master Aug 31, 2018

bsclifton deleted the webtorrent_ext branch September 1, 2018 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebTorrent Support #356

WebTorrent Support #356

yrliou commented Aug 23, 2018 •

edited

Loading

yrliou Aug 23, 2018

yrliou Aug 24, 2018

yrliou Aug 26, 2018 •

edited

Loading

yrliou commented Aug 27, 2018 •

edited

Loading

yrliou commented Aug 27, 2018 •

edited

Loading

yrliou Aug 27, 2018 •

edited

Loading

bridiver Aug 27, 2018

bridiver Aug 27, 2018

yrliou Aug 27, 2018

yrliou Aug 27, 2018 •

edited

Loading

bridiver Aug 27, 2018

yrliou Aug 27, 2018

yrliou Aug 27, 2018

bridiver Aug 27, 2018

yrliou Aug 27, 2018

yrliou Aug 27, 2018

bridiver Aug 28, 2018

bridiver left a comment

cezaraugusto left a comment

cezaraugusto Aug 29, 2018

yrliou commented Aug 30, 2018

cezaraugusto left a comment

yrliou commented Aug 30, 2018 •

edited

Loading

yrliou commented Aug 30, 2018

yrliou commented Aug 31, 2018

bbondy commented Aug 31, 2018

WebTorrent Support #356

WebTorrent Support #356

Conversation

yrliou commented Aug 23, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou Aug 26, 2018 • edited Loading

Choose a reason for hiding this comment

yrliou commented Aug 27, 2018 • edited Loading

yrliou commented Aug 27, 2018 • edited Loading

yrliou Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrliou commented Aug 30, 2018

cezaraugusto left a comment

Choose a reason for hiding this comment

yrliou commented Aug 30, 2018 • edited Loading

yrliou commented Aug 30, 2018

yrliou commented Aug 31, 2018

bbondy commented Aug 31, 2018

yrliou commented Aug 23, 2018 •

edited

Loading

yrliou Aug 26, 2018 •

edited

Loading

yrliou commented Aug 27, 2018 •

edited

Loading

yrliou commented Aug 27, 2018 •

edited

Loading

yrliou Aug 27, 2018 •

edited

Loading

yrliou Aug 27, 2018 •

edited

Loading

yrliou commented Aug 30, 2018 •

edited

Loading