Opened 2 months ago

Last modified 2 weeks ago

#22618 needs_revision defect

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, TorBrowserTeam201708
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 (12)

comment:1 Changed 2 months ago by gk

Keywords: tbb-e10s added

comment:2 Changed 2 months ago by mcs

The root cause is almost certainly shared with #22471.

comment:3 Changed 2 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 2 months ago by mcs (previous) (diff)

comment:4 in reply to:  3 Changed 2 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 2 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 2 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 weeks 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 3 weeks ago by gk

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201707R removed

Moving review tickets to August.

comment:9 Changed 3 weeks 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 3 weeks 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 3 weeks 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 2 weeks 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)

Note: See TracTickets for help on using tickets.