Magnet link doesn't start torrent download #5981

srirambv · 2016-12-02T13:30:21Z

Did you search for similar issues before submitting this one?
Yes

Describe the issue you encountered:
Magnet link doesn't start torrent download

Expected behavior:
Should load the torrent viewer and start downloading

Platform (Win7, 8, 10? macOS? Linux distro?):
Windows 10 x64
Brave Version:
0.13.0 preview 2
Steps to reproduce:
1. Click on any of the magnet links from https://codepen.io/ferossity/full/qaezaB/
2. Torrent viewer doesn't load and download doesn't start
Screenshot if needed:
Any related issues:
Extension downloading not working for C54 #5945

bbondy · 2016-12-05T14:04:14Z

@dcposch if you still have cycles available it'd be helpful if you could take a look on the chromium54 branch.

dcposch · 2016-12-05T22:35:33Z

@bbondy sure, checking it out

dcposch · 2016-12-07T03:28:50Z

Just reproduced the error:

Uncaught SyntaxError: Identifier 'webtorrentEntryPage' has already been declared at webtorrent.js:11

Here's webtorrent.js on the Chromium54 branch:

  let webtorrentEntryPage = 'gen/webtorrentPage.entry.js'

  var queryString = window.location.search
  var portMatch = queryString && queryString.match(/devServerPort=([0-9]+)/)
  var devServerPort
  if (portMatch) {
    devServerPort = portMatch[1]
  }

  let webtorrentEntryPage = 'gen/webtorrentPage.entry.js'
  // [...]

Looks like this commit duplicates webtorrentEntryPage: eb19c30

It's on chromium54 and a few other branches, but not on master.

dcposch · 2016-12-07T03:52:34Z

@bridiver

bridiver · 2016-12-07T03:53:53Z

looks like a rebase issue. The webtorrent.js file shouldn't exist anymore

bridiver · 2016-12-07T05:05:39Z

had to sort some things out in master and I'll update the changes for 54

dcposch · 2016-12-09T05:37:26Z

@bbondy @bridiver LMK if there's anything you'd like me to do.

Right now, Torrent Viewer doesn't work on the chromium54 branch. I just checked out chromium54 and tried reverting eb19c30, and that fixes the duplicate let statement, but there's a different error because it looks like chrome.ipc is gone? Works on master.

dcposch · 2016-12-09T05:43:59Z

Aha, looks like it's called chrome.ipcRenderer now. Changing to use chrome.ipcRenderer doesn't fix it, though

ayumi · 2016-12-16T19:38:16Z

@dcposch I get a CSP error when opening this magnet link in Brave 0.13.0 pre4:

srirambv · 2016-12-16T22:29:33Z

Doesn't load the Torrent viewer interface on Windows x64 (0.13.0 Preview 4)

@bbondy

partial fix for #5981 links still don't open directly auditors @bbondy

@bbondy

partial fix for #5981 links still don't open directly auditors @bbondy

bridiver · 2016-12-21T05:21:38Z

the urls work with "open in new tab" or ctrl/cmd-click now, but do nothing if you just click on them

srirambv · 2016-12-22T15:05:38Z

Doesn't work even with "open in new tab". Here's the screen grab

jonathansampson · 2016-12-28T18:28:28Z

I see the same thing as @srirambv.

bridiver · 2016-12-28T18:49:19Z

reverting eb19c30 is definitely not the right thing to do. The original code incorrectly bypassed the webpack dev server and the csp issues should be resolved in extensions.js

dcposch · 2016-12-28T18:56:37Z

@bridiver sure, but eb19c30 doesn't actually run at all due to the syntax error (duplicate let).

not sure how that got merged.

running without the webpack dev server means you have to run npm run webpack after changes to the torrent extension--i don't think it affects the production build. i can try to get the Torrent Viewer working with the dev server later today to make future development faster.

what are the remaining CSP issues?

cc @feross

bridiver · 2016-12-28T18:56:57Z

actually eb19c30 was already replaced by other code to handle the dev server and that file doesn't even exist anymore

bridiver · 2016-12-28T19:01:39Z

there are a lot of comments on this thread that are no longer relevant so I'm not sure what the outstanding issues are, but the code from eb19c30 is definitely not part of the problem because that file was removed a while ago and the webpack dev server is handled in filtering.js

dcposch · 2016-12-28T19:01:50Z

i'm building brave now to try out:

clicking a magnet link
pasting a magnet link into the URL bar
checking for CSP errrors

is master the right branch to be on?

bridiver · 2016-12-28T19:07:08Z

yep, everything from chromium54 has been merged into master and app/extensions/torrent/js/webtorrent.js doesn't exist there. I'm pretty sure that change was already superseded by the filtering change before it was even merged into master.

bbondy · 2016-12-28T19:59:57Z

I don't think the webtorrent filtering code is seeing the magnet protocol URLs at all

bridiver · 2016-12-28T20:12:24Z

@bbondy that is correct in some cases, but not others and I'm not sure why. Cmd-click seems to work, but normal links don't. Maybe they aren't registering as a navigation?

…

On Dec 28, 2016, at 1:00 PM, Brian R. Bondy ***@***.***> wrote: I don't think the webtorrent filtering code is seeing the magnet protocol URLs at all — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

srirambv · 2017-01-04T17:29:42Z

Seeing this error on 0.13.0 Preview 7

bridiver · 2017-01-04T18:37:01Z

why is the page trying to connect to example.com? The only reference I see to that is in a comment

srirambv · 2017-01-04T18:54:52Z

feross · 2017-01-11T05:44:07Z

Looks like example.com is coming from stream-http's capability detection code. It doesn't actually make an http request. Just calls xhr.open() without xhr.send(). But it looks like that still violates the CSP.

@jhiesey - is there any way to remove example.com from that code, since it could potentially violate the webpage's CSP?

For: brave/browser-laptop#5981

feross · 2017-01-11T07:31:45Z

Working on a PR to stream-http. Can someone tell me the proper branch to test this on? @bridiver: is master still the right place to test?

bsclifton · 2017-01-11T07:41:57Z

@feross master is looking pretty good and is what you should branch from! 😄 if you haven't recently, please remove node_modules and re-npm install

You can tag me as a reviewer (I'd love to try this functionality out; I haven't tried it yet actually)

feross · 2017-01-11T07:56:48Z

Sounds good! 👍

Right now, it looks to me like the torrent viewer code isn't even being triggered when clicking on a magnet link. This must have broken at some point and no one noticed. 😐

bridiver · 2017-01-11T07:57:58Z

@feross that is a known issue in 54

feross · 2017-01-11T07:58:26Z

PR to stream-http here: jhiesey/stream-http#65

Haven't confirmed this fixes the CSP issue since I haven't gotten the torrent viewer to run on master yet.

@bridiver Is there an issue for that?

bridiver · 2017-01-11T07:59:09Z

@feross isn't this the issue?

feross · 2017-01-11T08:33:15Z

@bridiver It looks like updateWebview in frame.js never gets called when a magnet link is clicked.

That used to get called in the past, and in there we detect it's a magnet: url and load the torrent viewer. Any ideas what's preventing that from getting called for magnet: links now?

bridiver · 2017-01-11T08:37:03Z

the redirect is handled here https://github.com/brave/browser-laptop/blob/master/app/browser/webtorrent.js#L37 but that never gets hit because the navigation is completely ignored and I'm not sure why yet

feross · 2017-01-11T09:04:19Z

Unlike normal web requests, the magnet link clicks are getting lost here:

browser-laptop/js/components/frame.js

Line 767 in b466a44

if (e.isMainFrame && !e.isErrorPage && !e.isFrameSrcDoc) {

because e.isMainFrame is false for some reason. Any ideas?

feross · 2017-01-13T00:28:21Z

Okay, the CSP bug fix was just merged and released in stream-http. There should be no more requests to example.com. Now, the remaining issue is to fix the breakage that happened when the chromium54 branch was merged.

@bridiver @bbondy Any ideas why e.isMainFrame is false for clicks on magnet: links but not for normal links. That is causing our handler not to run. See my previous comment.

It was erroring with: ``` _stream_writable.js var asyncWrite = !process.browser && ['v0.10', 'v0.9.'].indexOf(process.version.slice(0, 5)) > -1 ? setImmediate : ``` because our webpack config sets process to false and the process object we provide doesn't have a version property until now. processNextTick; Related to brave/browser-laptop#5981

bbondy · 2017-01-16T02:02:04Z

I'll take a look next, I was trying today but this was blocking me, fixed now:
brave/muon@a1c7592

bbondy · 2017-01-16T02:11:55Z

@feross by the way the torrent file downloads ok, but when I try to play it, in this case an MP3 from here:
magnet:?xt=urn:btih:6a9759bffd5c0af65319979fb7832189f4f3c35d&dn=sintel.mp4&tr=udp%3A%2F%2Fexodus.desync.com%3A6969&tr=udp%3A%2F%2Ftracker.coppersurfer.tk%3A6969&tr=udp%3A%2F%2Ftracker.internetwarriors.net%3A1337&tr=udp%3A%2F%2Ftracker.leechers-paradise.org%3A6969&tr=udp%3A%2F%2Ftracker.openbittorrent.com%3A80&tr=wss%3A%2F%2Ftracker.btorrent.xyz&tr=wss%3A%2F%2Ftracker.webtorrent.io&ws=https%3A%2F%2Fwebtorrent.io%2Ftorrents%2Fsintel-1024-surround.mp4

It errors with this:

bbondy · 2017-01-18T01:55:11Z

I got this working even for links inside pages, it'll be in the next release. I ended up removing the filtering code and doing it through the same API as navigator.registerProtocolHandler uses.
Beyond that I had to adjust media-src to accept anything from localhost.

It'll be available in the next preview build (0.13.0-preview10)

QA
Click on any of the magnet links from https://codepen.io/ferossity/full/qaezaB/
Torrent viewer should be loaded
Download should start

@darkdh

Auditors: @darkdh Fix #5981

srirambv added chromium54 feature/extensions labels Dec 2, 2016

srirambv added this to the 0.13.0 milestone Dec 2, 2016

bbondy added the release/not-blocking label Dec 13, 2016

bridiver self-assigned this Dec 20, 2016

bridiver added a commit that referenced this issue Dec 20, 2016

fix webtorrent csp

dcc0fa3

partial fix for #5981 links still don't open directly auditors @bbondy

bridiver removed their assignment Dec 20, 2016

cezaraugusto pushed a commit that referenced this issue Dec 20, 2016

fix webtorrent csp

3945984

partial fix for #5981 links still don't open directly auditors @bbondy

feross added a commit to jhiesey/stream-http that referenced this issue Jan 11, 2017

Avoid xhr request to example.com if fetch is supported

43761f2

For: brave/browser-laptop#5981

feross mentioned this issue Jan 11, 2017

Avoid xhr request to example.com if fetch is supported jhiesey/stream-http#65

Merged

This was referenced Jan 17, 2017

Change download icon to save after torrent is downloaded #5815

Closed

Feature: WebTorrent Integration #3256

Closed

bbondy closed this as completed in brave/muon@8ca989b Jan 18, 2017

bbondy added a commit that referenced this issue Jan 18, 2017

Fix magnet link handling

4564618

Auditors: @darkdh Fix #5981

luixxiul added QA/test-plan-specified feature/torrent labels Jan 18, 2017

srirambv added the QA/checked-Win64 label Jan 20, 2017

luixxiul added QA/checked-Win32 release-notes/exclude and removed feature/extensions labels Jan 23, 2017

Magnet link doesn't start torrent download #5981

Magnet link doesn't start torrent download #5981

Comments

srirambv commented Dec 2, 2016

bbondy commented Dec 5, 2016

dcposch commented Dec 5, 2016

dcposch commented Dec 7, 2016 • edited Loading

dcposch commented Dec 7, 2016

bridiver commented Dec 7, 2016

bridiver commented Dec 7, 2016

dcposch commented Dec 9, 2016 • edited Loading

dcposch commented Dec 9, 2016 • edited Loading

ayumi commented Dec 16, 2016

srirambv commented Dec 16, 2016

bridiver commented Dec 21, 2016 • edited Loading

srirambv commented Dec 22, 2016

jonathansampson commented Dec 28, 2016

bridiver commented Dec 28, 2016

dcposch commented Dec 28, 2016 • edited Loading

bridiver commented Dec 28, 2016

bridiver commented Dec 28, 2016

dcposch commented Dec 28, 2016

bridiver commented Dec 28, 2016 • edited Loading

bbondy commented Dec 28, 2016

bridiver commented Dec 28, 2016 via email

srirambv commented Jan 4, 2017

bridiver commented Jan 4, 2017

srirambv commented Jan 4, 2017

feross commented Jan 11, 2017 • edited Loading

feross commented Jan 11, 2017 • edited Loading

bsclifton commented Jan 11, 2017

feross commented Jan 11, 2017

bridiver commented Jan 11, 2017

feross commented Jan 11, 2017

bridiver commented Jan 11, 2017

feross commented Jan 11, 2017

bridiver commented Jan 11, 2017

feross commented Jan 11, 2017

feross commented Jan 13, 2017

bbondy commented Jan 16, 2017

bbondy commented Jan 16, 2017

bbondy commented Jan 18, 2017 • edited by luixxiul Loading

dcposch commented Dec 7, 2016 •

edited

Loading

dcposch commented Dec 9, 2016 •

edited

Loading

dcposch commented Dec 9, 2016 •

edited

Loading

bridiver commented Dec 21, 2016 •

edited

Loading

dcposch commented Dec 28, 2016 •

edited

Loading

bridiver commented Dec 28, 2016 •

edited

Loading

feross commented Jan 11, 2017 •

edited

Loading

feross commented Jan 11, 2017 •

edited

Loading

bbondy commented Jan 18, 2017 •

edited by luixxiul

Loading