Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Saving torrent file causes crash #13365

Closed
srirambv opened this issue Mar 2, 2018 · 8 comments
Closed

Saving torrent file causes crash #13365

srirambv opened this issue Mar 2, 2018 · 8 comments

Comments

@srirambv
Copy link
Collaborator

srirambv commented Mar 2, 2018

Description

Saving torrent file causes crash

Steps to Reproduce

  1. Clean install 0.21.654
  2. Visit https://webtorrent.io/torrents/big-buck-bunny.torrent
  3. Click on save torrent, browser crashes

Actual result:
torrentcrash

Expected result:
Shouldn't crash when saving torrent file

Reproduces how often:
100%

Brave Version

about:brave info:

Brave 0.21.654
OS Release 10.0.16299
Muon 5.1.3
V8 6.5.254.26
libchromiumcontent 65.0.3325.88
Node.js 7.9.0
rev bc6272c
Update Channel Beta
OS Platform Microsoft Windows
OS Architecture x64

Reproducible on current live release:
No

Additional Information

Saving an image doesn't cause crash
#13350
cc: @darkdh @bridiver

@srirambv srirambv added crash regression feature/torrent priority/P2 Crashes. Loss of data. Severe memory leak. 0.21.x issue first seen in 0.21.x labels Mar 2, 2018
@srirambv srirambv added this to the 0.21.x w/ Chromium 65 (Beta Channel) milestone Mar 2, 2018
@bsclifton
Copy link
Member

bsclifton commented Mar 2, 2018

@darkdh possibly caused by the problem we had with bittorrent-tracker and it's downstream dependency on bufferutils. Maybe we can try removing the lock file and perform npm install using npm install --no-optional

@darkdh
Copy link
Member

darkdh commented Mar 5, 2018

@bsclifton no, this is because item->GetWebContents() is null
https://github.com/brave/muon/blob/ac452600e5fc77e251155339ea9fb407469a14b6/atom/browser/atom_download_manager_delegate.cc#L139 so it sequentially cause null window and finally crash at window->inspectable_web_contents().
Only reproduced in C65

@darkdh darkdh added the cr65 label Mar 5, 2018
@ltilve
Copy link

ltilve commented Mar 6, 2018

It seems to be reproducible also on Linux and Mac.

@ltilve
Copy link

ltilve commented Mar 7, 2018

When file download was initiated by adding target="_blank" w/o download property, clicking that link was initiating file download from newly created tab instead of current tab that has that link.

When file download was controlled by download manager, newly created web contents was destroyed. However, brave was trying to find browser window's webcontents to show save dialog on it by using destroyed webcontents. But, it was already null, so can't find window pointer. It caused crash.

We created a PR to fix the issue brave/muon#522

It allows downloading from newly created tab crash to be handled too. In this patch, use last active native window if there is no window related with download item.

@ltilve
Copy link

ltilve commented Mar 8, 2018

Updated the PR to move download closing code from browser-laptop to muon. It seems AtomDownloadManagerDelegate is the proper place to do it because item's webcontents is used there and after used, we can close it safely.
#13404
brave/muon#522

@ltilve
Copy link

ltilve commented Mar 13, 2018

Created a new PR brave/muon#529 to amend the merged brave/muon#522. Just with the already applied patch, the tab would be closed whether it is newtab on download or not.

@ltilve
Copy link

ltilve commented Mar 14, 2018

Now that the muon PR is merged (brave/muon#529) the browser-laptop one #13404 could be merged too.

@bsclifton
Copy link
Member

Fixed with brave/muon#529 and #13404

Thanks, @ltilve 😄 👍

# for free to subscribe to this conversation on GitHub. Already have an account? #.