Opened 6 months ago

Closed 4 months ago

Last modified 2 months ago

#22618 closed defect (fixed)

Downloading pdf file via file:/// is stalling if the external helper dialog is enabled

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-7.0-issues, tbb-regression, tbb-e10s, TorBrowserTeam201708R, tbb-backported
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

See the scenario in #22610. 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.

Child Tickets

Change History (15)

comment:1 Changed 6 months ago by gk

Keywords: tbb-e10s added

comment:2 Changed 6 months ago by mcs

The root cause is almost certainly shared with #22471.

comment:3 Changed 6 months ago by mcs

Actually, maybe this is different. What do you mean by "via file:///?" Isn't the URL an http:// or https:// one?

Last edited 6 months ago by mcs (previous) (diff)

comment:4 in reply to:  3 Changed 6 months ago by gk

Replying to mcs:

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 #22610, 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.

comment:5 Changed 6 months ago by mcs

I missed the fact that it had to be a local file (sorry!)
The cause is likely similar to that of #22471; 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 #21766 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.

comment:6 in reply to:  5 Changed 6 months ago by gk

Replying to mcs:

I missed the fact that it had to be a local file (sorry!)
The cause is likely similar to that of #22471; 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 #21766 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.

comment:7 Changed 5 months ago by mcs

Keywords: TorBrowserTeam201707R added
Status: newneeds_review

Kathy and I looked at getting rid of the extra dialog but decided against it for three reasons:

  1. 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.
  2. It would be difficult to fit meaningful warning text into Mozilla's existing protocol and "download or save" prompts.
  3. 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 #21766 (our testing confirms that this ticket – #22618 – is fixed as well as #22471, #22472, and #22610). 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.

Our Torbutton changes are here (one patch):
https://gitweb.torproject.org/user/brade/torbutton.git/log/?h=bug21766-01
Note that we reopened #22434 since we now understand what that ticket was about.

comment:8 Changed 5 months ago by gk

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201707R removed

Moving review tickets to August.

comment:9 Changed 5 months ago by arthuredelstein

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?

comment:10 in reply to:  9 ; Changed 5 months ago by mcs

Replying to arthuredelstein:

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?

Yes. Temporary files (.part) are created during the download and they are placed in the configured downloads directory. See the nsExternalAppHandler::SetUpTempFile() function here: https://dxr.mozilla.org/mozilla-esr52/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1400

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.

comment:11 in reply to:  10 Changed 4 months ago by gk

Replying to mcs:

Replying to arthuredelstein:

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?

Yes. Temporary files (.part) are created during the download and they are placed in the configured downloads directory. See the nsExternalAppHandler::SetUpTempFile() function here: https://dxr.mozilla.org/mozilla-esr52/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1400

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.

comment:12 Changed 4 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201708R removed
Status: needs_reviewneeds_revision

Alright, I think I am done with testing and code review. Very nice work! Here are some nits which should be fixed:

in the torbutton patch:

1) not sure what "may be opened by another or application" means to be honest.

in the tor browser patch:

2) nsExternalLoadURIHandler(nsIInterfaceRequestor * aWindowContext, -> nsExternalLoadURIHandler(nsIInterfaceRequestor *aWindowContext,

3) // if we are not supposed to ask <- s/if/If/ (I know it was just copied and pasted, but...)

4)

+  // Break our reference cycle with the download warning dialog (set up in
+  // OnStartRequest)

missing "." at the end of the sentence (twice)

5) 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)

comment:13 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201708 removed
Status: needs_revisionneeds_review

Here is a fixup patch for torbutton:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug21766-fixup-01

And here is one for the browser:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug21766-fixup-01

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.

comment:14 in reply to:  13 Changed 4 months ago by gk

Keywords: tbb-backport added
Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

Here is a fixup patch for torbutton:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug21766-fixup-01

And here is one for the browser:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug21766-fixup-01

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 af0ca6a886c7a9149602a6588cc4d44874f2fee9 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 9ecbfca98dc5488527495e3bfde0fabf75a8529a). Closing this and a bunch of related tickets, yay! + marking this as a potential backport candidate although it's a big patch.

comment:15 Changed 2 months ago by gk

Keywords: tbb-backported added; tbb-backport removed

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 d47e339ca3509f387ee1e4dcc931c6c92c732e98) that fixed #19273 and then cherry-picked the two from tor-browser-52.4.0esr-7.5-1 (bad2f0be0d196bf51fb69d8e26147b3df5733a13 and 7c3bb9697882b07c1ad4a496b0ce2e8ec2593969).

Note: See TracTickets for help on using tickets.