See the scenario in legacy/trac#22610 (moved). If you are not clicking on Cancel but are allowing the loading of the PDF file and you try to save it somewhere then the resulting download is stalling.
Designs
Child items
...
Show closed items
Linked items
0
Link issues together to show that they're related.
Learn more.
Actually, maybe this is different. What do you mean by "via file:///?" Isn't the URL an http:// or https:// one?
Yeah, "Downloading" might be misleading. What I have been doing is the same as in legacy/trac#22610 (moved), loading a pdf file from the hard disk but instead of hitting the "Cancel" button on the external helper app I proceeded and tried to save that file on the hard disk somewhere else. This gets handled as a download in Firefox it seems with the stalling symptoms.
I missed the fact that it had to be a local file (sorry!)
The cause is likely similar to that of legacy/trac#22471 (moved); my guess without looking at the code or debugging this problem is that the file: URL handler does not support the Suspend() / Resume() mechanism we added to work around legacy/trac#21766 (moved) and related problems. It seems like we will need to use a different technique to solve all of the problems, e.g., we could implement our download warning dialog the same way Mozilla implements their download-related prompts. Their prompts are asynchronous (which means the download proceeds while the user thinks about how to respond) but somehow the final download is blocked until the user finishes interacting with the dialog window. Maybe we can use Suspend() when it will work (i.e. for HTTP-based downloads) and skip the Suspend() in other cases.
I missed the fact that it had to be a local file (sorry!)
The cause is likely similar to that of legacy/trac#22471 (moved); my guess without looking at the code or debugging this problem is that the file: URL handler does not support the Suspend() / Resume() mechanism we added to work around legacy/trac#21766 (moved) and related problems. It seems like we will need to use a different technique to solve all of the problems, e.g., we could implement our download warning dialog the same way Mozilla implements their download-related prompts. Their prompts are asynchronous (which means the download proceeds while the user thinks about how to respond) but somehow the final download is blocked until the user finishes interacting with the dialog window. Maybe we can use Suspend() when it will work (i.e. for HTTP-based downloads) and skip the Suspend() in other cases.
FWIW: I am all for getting rid of our extra dialog if we can afford it. But the underlying bug (https://bugzilla.mozilla.org/show_bug.cgi?id=440892), which was the reason for creating this workaround, is still not fixed. But given the problems we have with it we might want to think about getting Mozilla into the boat for solving this problem once and for all. I guess we can discuss this a bit on Monday during the meeting.
Kathy and I looked at getting rid of the extra dialog but decided against it for three reasons:
The warning we currently display is shown for all downloads and external protocol accesses (e.g., mailto: links) and we should preserve that behavior. It appears that the preferences discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=440892 were only effective for external protocols.
It would be difficult to fit meaningful warning text into Mozilla's existing protocol and "download or save" prompts.
We have an existing preference (extensions.torbutton.launch_warning) that we should continue to support.
Instead, we dove in deeper and added support to the core browser code to displaying our existing warning dialog modelessly as is done for Mozilla's dialogs. We had to define some new objects and interfaces to make this work, but Kathy and I believe this is a much better fix for legacy/trac#21766 (moved) (our testing confirms that this ticket – legacy/trac#22618 (moved) – is fixed as well as legacy/trac#22471 (moved), legacy/trac#22472 (moved), and legacy/trac#22610 (moved)). What's the downside? A more complex patch to the core browser code and somewhat tight linkage between the browser code and part of Torbutton. Still, the results are good.
The following branch contains two patches, one to revert the previous patch and one to do things the new way:
https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug21766-04
Most of the changes are to split up existing code so that interaction with the Torbutton component is done asynchronously. We also had to add an nsExternalLoadURIHandler class to provide a request-specific object to hold information about each external protocol request while our download warning prompt is displayed.
I built these patches and tested. The behavior looks good. I also read through the code and overall didn't find any errors. Nice work!
One question I have: it looks as though files are downloaded while the first dialog is displayed. That doesn't necessarily seem to be a problem, but are there temporary files where the file is being stored?
One question I have: it looks as though files are downloaded while the first dialog is displayed. That doesn't necessarily seem to be a problem, but are there temporary files where the file is being stored?
We talked about this a little at one of our Monday meetings and Kathy and I had the impression that it was okay for the download to proceed as long as everything gets cleaned up if the user aborts the download. The difference in behavior between Tor Browser 6.5 and 7.x with this patch is that in 6.5 the Tor download warning prompt effectively paused the download because a modal dialog was presented.
One question I have: it looks as though files are downloaded while the first dialog is displayed. That doesn't necessarily seem to be a problem, but are there temporary files where the file is being stored?
We talked about this a little at one of our Monday meetings and Kathy and I had the impression that it was okay for the download to proceed as long as everything gets cleaned up if the user aborts the download. The difference in behavior between Tor Browser 6.5 and 7.x with this patch is that in 6.5 the Tor download warning prompt effectively paused the download because a modal dialog was presented.
I think that's okay. We could think about a more complicated solution which involves not downloading to disk in PBM but it's fine having it in a different bug.
I am still testing the patch and I have some review notes containing things I'd like to see changed/fixed. I'll post those once I am properly done. However, the patch seems to be important enough to get tested in an alpha as it solves a bunch of annoying regressions. That's why I included it for 7.5a4 even though not all parts of my review process are done yet. FWIW: mcs/brade: Thanks for this great work! It looks mostly good to me.
// if we are not supposed to ask <- s/if/If/ (I know it was just copied and pasted, but...)
+ // Break our reference cycle with the download warning dialog (set up in+ // OnStartRequest)
missing "." at the end of the sentence (twice)
Could you add all the bugs that get fixed by this patch in the commit message of the follow-up bugfix? (and then doing a git commit --squash or something similar so that this information gets preserved during rebasing)
Trac: Status: needs_review to needs_revision Keywords: TorBrowserTeam201708R deleted, TorBrowserTeam201708 added
Kathy and I are not sure how best to handle the commit message. We did a git commit --squash ... but we then wrote the commit message that we think should be preserved when rebasing is done (when the three Bug 19273 commits will be squashed together). Hopefully what we did will work, but please let us know if we should do something different.
Trac: Keywords: TorBrowserTeam201708 deleted, TorBrowserTeam201708R added Status: needs_revision to needs_review
Kathy and I are not sure how best to handle the commit message. We did a git commit --squash ... but we then wrote the commit message that we think should be preserved when rebasing is done (when the three Bug 19273 commits will be squashed together). Hopefully what we did will work, but please let us know if we should do something different.
That's okay. I'll take that into account when rebasing (actually I guess af0ca6a8 should have been a fixup commit and not a revert one, but that should be no issue when rebasing either).
Merge both, the Torbutton patch to master (commit 8e5e391c71d6c5f3ae80c88e45d1e8522f981214) and the tor-browser one to tor-browser-52.3.0esr-7.5-2 (commit 9ecbfca9). Closing this and a bunch of related tickets, yay! + marking this as a potential backport candidate although it's a big patch.
Backported for 7.0.7: Torbutton (commits 0144a6719ccb45ec7727454880c4350e40a418d5 and 61aaf90a94efd737213e31405f77965dd36001cd) for tor-browser I first reverted the old commit on tor-browser-52.4.0esr-7.0-1 (commit d47e339c) that fixed legacy/trac#19273 (moved) and then cherry-picked the two from tor-browser-52.4.0esr-7.5-1 (bad2f0be and 7c3bb969).