1463527 - Implement a mechanism to prevent multiple downloads

Status: NEW

(Firefox :: File Handling, defect, P3)

Description

[Sagar Bharadwaj]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36

Comment 1

[Sagar Bharadwaj]

Attachment: Diff with current changes

Does not entirely 'block' the download. Only implements a map to count downloads associated with a URL

Comment 2

[Sagar Bharadwaj]

Comment on attachment 8979722 [details] [diff] [review]
Diff with current changes

>diff --git a/toolkit/components/downloads/DownloadCore.jsm b/toolkit/components/downloads/DownloadCore.jsm
>--- a/toolkit/components/downloads/DownloadCore.jsm
>+++ b/toolkit/components/downloads/DownloadCore.jsm
>@@ -427,6 +427,10 @@ this.Download.prototype = {
>           throw new DownloadError({ becauseBlockedByRuntimePermissions: true });
>         }
> 
>+        if (DownloadIntegration.checkMultipleDownloads(this)) {
>+          throw new DownloadError({ message: "Multiple downloads are blocked" });
>+        }
>+
>         // We should check if we have been canceled in the meantime, after all
>         // the previous asynchronous operations have been executed and just
>         // before we call the "execute" method of the saver.
>diff --git a/toolkit/components/downloads/DownloadIntegration.jsm b/toolkit/components/downloads/DownloadIntegration.jsm
>--- a/toolkit/components/downloads/DownloadIntegration.jsm
>+++ b/toolkit/components/downloads/DownloadIntegration.jsm
>@@ -375,6 +375,30 @@ var DownloadIntegration = {
>                                       RuntimePermissions.WRITE_EXTERNAL_STORAGE));
>   },
> 
>+  checkMultipleDownloads: function D_checkMultipleDownloads(aDownload) {
>+
>+    let sourceURL = aDownload.source.url;
>+
>+    if (typeof D_checkMultipleDownloads.downloadCountMap == 'undefined'){
>+      D_checkMultipleDownloads.downloadCountMap = new Map();
>+    } 
>+
>+    if (!D_checkMultipleDownloads.downloadCountMap.has(sourceURL)) {
>+      D_checkMultipleDownloads.downloadCountMap.set(sourceURL, 1);
>+    } else {
>+      let currentDownloadCount = D_checkMultipleDownloads
>+                            .downloadCountMap.get(sourceURL);
>+      D_checkMultipleDownloads.downloadCountMap
>+                            .set(sourceURL, currentDownloadCount + 1);
>+    }
>+
>+    if(D_checkMultipleDownloads.downloadCountMap.get(sourceURL) > 1){
>+      return true;
>+    } else {
>+      return false;
>+    }
>+  },
>+
>   /**
>    * Checks to determine whether to block downloads because they might be
>    * malware, based on application reputation checks.
>@@ -584,7 +608,7 @@ var DownloadIntegration = {
>    */
>   async launchDownload(aDownload) {
>     let file = new FileUtils.File(aDownload.target.path);
>-
>+    
>     // In case of a double extension, like ".tar.gz", we only
>     // consider the last one, because the MIME service cannot
>     // handle multiple extensions.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8979722 [details] [diff] [review]
Diff with current changes

Review of attachment 8979722 [details] [diff] [review]:
-----------------------------------------------------------------

Decent start, but this makes me wonder where in the download flow we actually want to plug in.

::: toolkit/components/downloads/DownloadIntegration.jsm
@@ +378,5 @@
> +  checkMultipleDownloads: function D_checkMultipleDownloads(aDownload) {
> +
> +    let sourceURL = aDownload.source.url;
> +
> +    if (typeof D_checkMultipleDownloads.downloadCountMap == 'undefined'){

Let's not store this map as a property of the function. Instead, it should probably live as a property of the DownloadIntegration object - initialize it to a new Map() there.

@@ +380,5 @@
> +    let sourceURL = aDownload.source.url;
> +
> +    if (typeof D_checkMultipleDownloads.downloadCountMap == 'undefined'){
> +      D_checkMultipleDownloads.downloadCountMap = new Map();
> +    } 

nit: trailing whitespace. protip: You should be able to configure your editor to automatically trim trailing whitespace when saving files.

@@ +608,4 @@
>     */
>    async launchDownload(aDownload) {
>      let file = new FileUtils.File(aDownload.target.path);
> +    

nit: please remove this extra indent.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Paolo,

Sagar is working on the download spam protection project, and for his first task I thought it would be a good idea to implement a map of origin -> downloads initiated from that origin. As a first attempt we put this code in DownloadIntegration.jsm, which is probably ok for now, but trying to use it to "block" the download from DownloadCore.jsm near where we check for runtime permissions revealed that this is too late - at this point the "Save file" dialog has already been shown.

Could you chime in here and point us to where you think would be a more appropriate place to "block" the download from? From a bit of searchfox-ing, I couldn't determine where exactly the Save File dialog is spawned from.

Thanks!

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Forgot to set needinfo? on comment 4.

Comment 6

[:Paolo Amadini]

Hi! Yeah, Downloads.jsm is actually involved too late in the process. In the future we'd like to refactor how this works, but for the moment there is an entire separate download flow for downloads started during document loading. This flow goes through nsExternalHelperAppService.cpp and nsHelperAppDlg.js first, and only later transitions to Downloads.jsm through the nsITransfer interface and the DownloadLegacy object.

I think the best opportunity for showing a permission notification would be in nsHelperAppDlg.js, which is the module that handles the file name selection asyncrhonously:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js

Note that the download is already running in the background at this point, which is unavoidable, but it can be canceled if the user doesn't grant the permission. I don't know if we can get a reference to the original tab where the download started, if not we can update the interfaces to make sure this information is passed to the dialog.

If I remember correctly the origin might not be the only key to be used here, and we may need a mechanism more similar to temporary permissions, whose metadata is associated to a browser tab. We also may not need a download count, but a flag to indicate whether one download was already started, and this may be tab-specific rather than global. In fact, I'd suggest working on a proof-of-concept of the end-to-end workflow (and you can stub things like the user-initiated flag) before thinking about modularization.

Hope this helps!

Comment 7

[Sagar Bharadwaj]

Attachment: Diff after review comments

All comments have been addressed.

Comment 8

[Sagar Bharadwaj]

Paolo,

Thanks. Sorry for the late response but I somehow failed to notice your comment :(

So I'm guessing you'd rather use the browser element or the tab that started a download as the download's 'origin' rather than the URL (download.source.url). 

I was thinking of situations where we'll need to provide users protection against sites that intentionally bombard the user with multiple downloads. After a user has blocked multiple downloads from a site, opening the same site on a different tab must not bother the user with the permission prompt again. True?

Also, shouldn't the meta data of temporary permission be associated with the URL and not the browser tab keeping the above situation in mind? 

But in cases where the user wants to block multiple downloads on one tab but allow on others for the same site, he/she may have to go through the arduous process of changing permissions. 

Thoughts?

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Sagar Bharadwaj from comment #8)
> So I'm guessing you'd rather use the browser element or the tab that started
> a download as the download's 'origin' rather than the URL
> (download.source.url). 

You'd still need to reset the count upon origin change.

> I was thinking of situations where we'll need to provide users protection
> against sites that intentionally bombard the user with multiple downloads.
> After a user has blocked multiple downloads from a site, opening the same
> site on a different tab must not bother the user with the permission prompt
> again. True?

This isn't obvious. First of all, why on earth would a sane user repeatedly navigate to a page that spawns multiple *unwanted* downloads every time? Do you think a site with legitimate content that the user is interested in, and more importantly one that the user would repeatedly come back to, would ever do this? Doesn't seem like a real scenario to me.

Anyway, we could use a checkbox to "always block for this site", but do we want it checked by default? Maybe have it unchecked by default the first time, and checked by default the second time it shows up? We can debate this at leisure when the time comes, it's not important immediately. I do understand that you're saying that it's important to keep track of the origin if we want to do this, though.

> Also, shouldn't the meta data of temporary permission be associated with the
> URL and not the browser tab keeping the above situation in mind? 

Let's worry about this once we figure out the blocking code path.

> But in cases where the user wants to block multiple downloads on one tab but
> allow on others for the same site, he/she may have to go through the arduous
> process of changing permissions. 
> 
> Thoughts?

Again, this doesn't seem like a real scenario to me.

Comment 10

[Sagar Bharadwaj]

Nihanth,

Having a checkbox like you suggested solves everything. But, you are right about the scenario not being common and it wouldn't justify investing time on it. 

So I guess the 'origin' should be the tab that triggered the download and we should explore if we have access to that.

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to :Paolo Amadini from comment #6)
> If I remember correctly the origin might not be the only key to be used
> here, and we may need a mechanism more similar to temporary permissions,
> whose metadata is associated to a browser tab. We also may not need a
> download count, but a flag to indicate whether one download was already
> started, and this may be tab-specific rather than global.

I think we can't make it exclusively tab-specific, I think we need to track the origin as well, or now that I think about it, maybe it should be URL-specific. Think about this situation: there are multiple download links on a page (for example, Firefox Beta and Nightly). The user clicks Beta. It takes them to a landing page that initiates the download ("automated download" #1). The user then goes back, and clicks Nightly. Again, it takes them to a landing page that initiates the download ("automated download" #2). Feels wrong to show a permission prompt in this scenario.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Sagar, not sure if you made progress, but in case you are stuck, I did some digging and here is a hint:

nsHelperAppDlg.js implements the interface for showing the file saving dialog. This interface is called nsIHelperAppLauncherDialog [1]. You can use searchfox to figure out how this interface is used [2]. This should lead you into nsExternalHelperAppService.cpp, and from there you should be able to trace the code path to the point where the dialog is actually spawned. Once you arrive at this line, you should be able to figure out from the surrounding code how to cancel the download, etc. Basically I'd guess this would be the place in code from which the download counting/blocking code path would start.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Forgot to add links to comment 12.

[1] https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#124
[2] https://searchfox.org/mozilla-central/search?q=nsIHelperAppLauncherDialog

Comment 14

[Sagar Bharadwaj]

Nihanth,

Thanks for this :) I've been having some health issues since a couple of days. Will look into this.

Comment 15

[Sagar Bharadwaj]

Attachment: Plugging in nsHelperAppDialog.js

The checking code added in nsHelperAppDlg.js

However, I'm not really sure if that's the place we should be checking for multiple downloads. 

The helper app dialog is only opened if the preferred action for a mime type of the response is unknown. Looks like it is not opened in case the user has set the preference for a mime type under Applications in about:preferences. This can also be set by checking the remember choice checkbox in helper app dialog.

So shouldn't multiple downloads be checked before even the mime type preferences are read. Looks that code is around here : 

https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1683

The dialog is invoked here : 

https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1737

However, logging here doesn't print anything though (used MOZ_LOG and set environment variable). 

Should we continue modifying nsHelperAppDlg.js (thereby respecting user prefs for required action for a file type and always doing that regardless of how many downloads are started) or modify nsExternalHelperAppService.cpp or some other file?

Comment 16

[Sagar Bharadwaj]

Also, should we create a new bug for poc?

Comment 17

[:Paolo Amadini]

(In reply to Sagar Bharadwaj from comment #15)
> The helper app dialog is only opened if the preferred action for a mime type
> of the response is unknown. Looks like it is not opened in case the user has
> set the preference for a mime type under Applications in about:preferences.
> This can also be set by checking the remember choice checkbox in helper app
> dialog.
> 
> So shouldn't multiple downloads be checked before even the mime type
> preferences are read. Looks that code is around here : 

Ah, good catch, the protection from multiple downloads should definitely apply also to cases where we wouldn't show the handler choice dialog.

Comment 18

[Sagar Bharadwaj]

Also, the 'blue flash' that appears when a download is triggered is still shown. Maybe we can start our flow there? But I'm not aware of how I can find that.

Comment 19

[Sagar Bharadwaj]

The flash is also shown only on the tab that triggered a download. So looks like we do have reference to the tab (and the corresponding browser element maybe) that triggered the download. Which is the place where such 'flashes' or animations on the tab are handled?

Comment 20

[Sagar Bharadwaj]

Okay, so looks like it's here : 

https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#4524


But I think that happens only when the download completes and not when its triggered. (https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#4514)

Comment 21

[Sagar Bharadwaj]

Clearing flag.

Comment 22

[:Paolo Amadini]

Thanks, I didn't know which code controlled the "bursting" state. For our purposes, we can ignore this, since it is related to the network-level load of the page, which may happen regardless of whether the download will be blocked later in the flow. I don't think we have an easy way of knowing if the download will be blocked at this point in the flow.

Comment 23

[Sagar Bharadwaj]

Okay,

So here is the download flow that we should be concerned about : 

1. Starts in nsURILoader.cpp, nsDocumentOpenInfo::OnStartRequest : https://searchfox.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#219

2. Through : https://searchfox.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#313 to https://searchfox.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#385 (nsDocumentOpenInfo::DispatchContent)

3. Checking for if Content-Disposiotion is set to attachment : https://searchfox.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#423.

I think we should put out permissions checking code somewhere here. The temp file has not been created here and the download hasn't yet started. So this means download spam does not hog user's bandwidth.

4. Through : https://searchfox.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#619 to https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#696 (nsExternalHelperAppService::DoContent)

5. Through : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#704 to https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#618 (nsExternalHelperAppService::DoContentContentProcessHelper)

6. Through : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#692 to https://searchfox.org/mozilla-central/source/uriloader/exthandler/ExternalHelperAppChild.h#33

7. And then ExternalHelperAppChild::OnStartRequest : https://searchfox.org/mozilla-central/source/uriloader/exthandler/ExternalHelperAppChild.cpp#77

8. Through : https://searchfox.org/mozilla-central/source/uriloader/exthandler/ExternalHelperAppChild.cpp#79 to https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1560 (nsExternalAppHandler::OnStartRequest)

9. And then nsExternalHelperAppService::DoContent() and then back to nsExternalHelperAppService::OnStartRequest()

10. Decision on whether to invoke the dialog or not starts here : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1683

By now, the temp file has been set up and transfer has begun in the background. We can also put permissions checking code here and cancel the download based on results. This deletes the temp file and stops the transfer.

If dialog need not be invoked, SaveToDisk is directly called : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1777

11. Dialog invoked here : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1737 

12. Dialog show function is here : https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#132 (nsUnknownContentTypeDialog.show())

We should put permissions checking code here if we should always respect user's preferences for a particular mime type irrespective of how many downloads are triggered

13. Through : https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#148, a timer is started

14. notify function here : https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#148

15. Through : https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#537 to https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#155 (reallyShow() function)

16. After dialog, SaveToDisk function is invoked : https://searchfox.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#544 (Temp file to target file)

17. SaveToDisk : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2283

18. Through : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2305 to https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2310 ( nsExternalAppHandler::ContinueSave)

19. Create transfer invoked (where actual transfer starts) : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2351

20. Create Transfer : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2122
Through : https://searchfox.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2165, the flow finally enters the download module.

After this download.start (https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadCore.jsm#310) is eventually called. It's too late to check now, rightly said.

I'll create a patch with the download count checking code and plug it at the code point mentioned in Point 3.

Comment 24

[:Paolo Amadini]

(In reply to Sagar Bharadwaj from comment #23)
> I think we should put out permissions checking code somewhere here. The temp
> file has not been created here and the download hasn't yet started. So this
> means download spam does not hog user's bandwidth.

Any operation that may wait for a significant time has to be done after setting up the temporary ".part" file. This is because servers may expect the transfer of the actual data to start soon, and will timeout otherwise, making the download fail if the permission request is not answered within a few seconds.

Also, you'll find that the network handling code expects this part of the process to be synchronous. There are ways to suspend the network request until the operation is completed, which would allow the use of an asynchronous function at this stage, but this is a major refactoring that has various dependencies and would likely take months.

> By now, the temp file has been set up and transfer has begun in the
> background. We can also put permissions checking code here and cancel the
> download based on results. This deletes the temp file and stops the transfer.

Yes, this looks like the correct place for the check. Make sure the API is asynchronous, which traditionally means it will invoke a callback when done. We may even have support for using Promise easily in the C++/JavaScript boundary nowadays, but I haven't worked with it. Nihanth may be able to suggest a way here.

Thanks for documenting the full download flow from page load, it's quite helpful, as I think any documentation we used to have would be now outdated.

Comment 25

[Sagar Bharadwaj]

(In reply to :Paolo Amadini from comment #24)

> Any operation that may wait for a significant time has to be done after
> setting up the temporary ".part" file. This is because servers may expect
> the transfer of the actual data to start soon, and will timeout otherwise,
> making the download fail if the permission request is not answered within a
> few seconds.
> 
> Also, you'll find that the network handling code expects this part of the
> process to be synchronous. There are ways to suspend the network request
> until the operation is completed, which would allow the use of an
> asynchronous function at this stage, but this is a major refactoring that
> has various dependencies and would likely take months.

Thanks for this :). Didn't think of this at all!

> Yes, this looks like the correct place for the check. Make sure the API is
> asynchronous, which traditionally means it will invoke a callback when done.
> We may even have support for using Promise easily in the C++/JavaScript
> boundary nowadays, but I haven't worked with it. Nihanth may be able to
> suggest a way here.

Okay. I'll look into that.

Comment 26

[(inactive) Nihanth Subramanya [:nhnt11]]

Hey Sagar, did you make any progress on the API? If you need some inspiration on how to design the API for requesting the permission, here's a quick idea: why not copy what the unknown file type handler dialog does? Make a new file that is equivalent to nsHelperAppDialog.js, along with the relevant interface etc. Of course, the show() function would ask for permission instead of asking of asking what to do with the file.

I'm suggesting this because nsHelperAppDialog does something very similar to what we want. It's a JS frontend for a user action, and provides a way to call it from C++.

This new interface/component would do all the logic like counting/tracking downloads and requesting user permission.

The complication I think is that we need to call this API, wait for it to do its thing, and then if the user granted permission, still go ahead into the nsHelperAppDialog flow. There are a few ways I can think of to do this. Here's one:

- We can invoke the permission dialog at the same place we currently invoke nsHelperAppDialog (replace nsHelperAppDialog with our new one, basically). 
- nsHelperAppDialog calls mLauncher.saveToDisk to continue the flow. We can make mLauncher have another function, and move the nsHelperAppDialog call into this new function, and call it from our permission frontend if the user agrees to grant permission.

some crude pseudocode to illustrate what I'm saying:

OnStartRequest(...) {
  ...
  nsDownloadSpamPermissionHandler->show(...); // instead of mDialog->show();
}

HandleMimeTypeAndSaveToDisk(...) {
  mDialog->show(...);
}

This might require some refactoring withing nsExternalHelperAppService.cpp, especially with how things are named so that it's clear that this file is doing more than just handling external app launching. That's not too important for the proof of concept though.

Comment 27

[(inactive) Nihanth Subramanya [:nhnt11]]

Great job documenting that flow by the way, it's pretty helpful. Small suggestion: I'd recommend using permalinks in searchfox in the future to ensure the links always point to the line you want if the file is changed by future commits.

Comment 28

[Sagar Bharadwaj]

(In reply to Nihanth Subramanya [:nhnt11] from comment #26)
> Hey Sagar, did you make any progress on the API? If you need some
> inspiration on how to design the API for requesting the permission, here's a
> quick idea: why not copy what the unknown file type handler dialog does?
> Make a new file that is equivalent to nsHelperAppDialog.js, along with the
> relevant interface etc. Of course, the show() function would ask for
> permission instead of asking of asking what to do with the file.
> 
Hey Nihanth, had some hardware problems so couldn't respond. All good now. Thanks for the lead. Will proceed from here :)

(In reply to Nihanth Subramanya [:nhnt11] from comment #27)
> Small suggestion: I'd recommend using permalinks in searchfox in the future to
> ensure the links always point to the line you want if the file is changed by
> future commits.

Yeah will do this

Comment 29

[Sagar Bharadwaj]

Hey Nihanth,

In order to clone what nsHelperAppDlg.js does, we'll need to create a new XPCOM interface to enable interfacing between c++ and js. So should I create a new idl (probably nsIMultipleDownloads.idl) and implement it in js. I can't imagine the interface having anything other than 'checkMultipleDownloads' function. (This function should be called from c++ and the function will return asynchronously the result of user's permission, if I'm not wrong). 

So should we go ahead and create a new interface and install it for this? Or is there some alternative to calling js functions from c++? I tried going through JSAPI, but still looks like a lot of trouble. 

Is there an XPCOM interface that is written to handle user permissions? If yes, we can implement this without having to write our own interface

The permissions module that I think we'll be eventually using : https://searchfox.org/mozilla-central/source/browser/modules/SitePermissions.jsm. However I couldn't find any XPCOM interface's implementation that uses this module. Mostly just calls from within js

Comment 30

[Sagar Bharadwaj]

Attachment: POC without permissions

This is the POC without permissions. I've been working on permissions, thought I'd upload this for review meanwhile.

Comment 31

[Sagar Bharadwaj]

Attachment: DiffWithPermissions.diff

Permissions using SitePermissions.jsm. Does not yet conform to the UI spec on the meta bug however. Uses confirmCheck from nsIPromptService. Scope of permission is current session.

Comment 32

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8990586 [details] [diff] [review]
DiffWithPermissions.diff

Review of attachment 8990586 [details] [diff] [review]:
-----------------------------------------------------------------

Good work with the xpcom stuff! As we discussed on IRC, let's get rid of the "remember" checkbox and make sure we don't overlap prompts.

Comment 33

[Sagar Bharadwaj]

Attachment: Patch with permission prompt attached to the download triggering browser

Patch with permission prompt attached to the download triggering browser. Downloads are now queued up when the prompt is being shown. Queued up downloads are allowed or blocked after user takes action

Comment 34

[Sagar Bharadwaj]

(In reply to Sagar Bharadwaj from comment #33)
> Created attachment 8993513 [details] [diff] [review]
> Patch with permission prompt attached to the download triggering browser
> 
> Patch with permission prompt attached to the download triggering browser.
> Downloads are now queued up when the prompt is being shown. Queued up
> downloads are allowed or blocked after user takes action

Paolo, please have a look.

Also, I was trying things to differentiate between user initiated and automatic downloads. No success as of now. 

I tried EventStateManager::IsHandlingUserInput() : It always returns false. 
https://searchfox.org/mozilla-central/rev/799c3f49233257bd5e899c4107f65ea394b7ae96/dom/events/EventStateManager.h#269
(Documentation wise, that should work)

nsContentUtils::GetUserIsInteracting() : Always returns true
https://searchfox.org/mozilla-central/rev/10e6320abc7fb0479032f0e95e9d61e4be194b9e/dom/base/nsContentUtils.h#3181

nsIDocument::UserHasInteracted()
https://searchfox.org/mozilla-central/rev/799c3f49233257bd5e899c4107f65ea394b7ae96/dom/base/nsIDocument.h#3409
I could not get access to the nsIDocument corresponding to an nsIRequest (or other information we have)

Do we have some other way to determine this?

Btw, I'm checking the condition in this context : https://searchfox.org/mozilla-central/rev/799c3f49233257bd5e899c4107f65ea394b7ae96/uriloader/exthandler/nsExternalHelperAppService.cpp#1675

Comment 35

[(inactive) Nihanth Subramanya [:nhnt11]]

Nice work! This seems to work well. The UX will need to be tweaked, but I think we are now at a point where we should start doing real code review and start landing this in pieces. Let's split this large patch into small, piece-wise commits. Just as an example:

commit #1: Add interfaces to allow frontend to choose to allow or block a download
commit #2: Basic implementation of interfaces (simply allow all downloads, or something)
commit #3: Maintain downloads-per-URL count, associated with browser tabs
commit #4: Implement UI prompt


Again, this is just an example. Splitting this kind of patch into smaller commits is important for easy code review. Also, for the final patch, we should land this feature behind a pref: i.e., check the value of a pref "browser.download.promptForAutomatedDownloads" and simply always allow the download if it's false. The default value of the pref should be false and we can change this default when the feature is ready to ship in Nightly.

Comment 36

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Added interface to allow invoking multiple downloads check mechanism from backend

Added interface to allow invoking multiple downloads check mechanism from backend

Comment 37

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Basic Implementation of nsIMultipleDownloadsCheck currently allowing all downloads

Basic Implementation of nsIMultipleDownloadsCheck currently allowing all downloads

Comment 38

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Added pref to enable or disable the multiple downloads check feature

Added pref to enable or disable the multiple downloads check feature

Comment 39

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Download per url count maintained. Second download blocked

Download per url count maintained. Second download blocked

Comment 40

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Implements permission prompt. Downloads triggered while displaying prompt blocked

Implements permission prompt. Downloads triggered while displaying prompt blocked

Comment 41

[Sagar Bharadwaj]

Attachment: Bug 1463527 - Queueing up downloads triggered while the prompt is displayed

Queueing up downloads triggered while the prompt is displayed

Comment 42

[:Paolo Amadini]

Hi Sagar, thank you for splitting the patch, this definitely helps with the review process.

Do you have tryserver access? If not, the next step would be to set it up, so you can run tests in our automation infrastructure. Once you have a tryserver build of the patch series, you can post a link here. This will make sure the code does not cause issues on other platforms, and it will make it easier for me to apply the patch series locally.

I noticed some formatting and indentation issues that ESLint would have caught. If you have run it, it's possible that this folder is excluded. In this case, we can update the configuration to exclude individual files but not the entire folder, so newly added files are linted.

I don't have an answer right now on how to detect downloads started by the user, there is bug 1306351 on file to figure this out. This may require passing additional flags to the network load operation. Probably Boris Zbarsky would have suggestions, but while the feature is behind a preference we can still do the other parts of the project, and only enable the feature once everything is ready.

Comment 43

[Sagar Bharadwaj]

(In reply to :Paolo Amadini from comment #42)

> Do you have tryserver access? If not, the next step would be to set it up,
> so you can run tests in our automation infrastructure. 

I do not have try server access. I have filed a bug under "Repository Account Requests" for the same. (https://bugzilla.mozilla.org/show_bug.cgi?id=1479846).

> I noticed some formatting and indentation issues that ESLint would have
> caught. 

I did not run eslint. I'll amend my commits after eslint

> I don't have an answer right now on how to detect downloads started by the
> user, there is bug 1306351 on file to figure this out. This may require
> passing additional flags to the network load operation. Probably Boris
> Zbarsky would have suggestions, but while the feature is behind a preference
> we can still do the other parts of the project, and only enable the feature
> once everything is ready.

Sure I'll work on other parts of the project (UI/UX tweaks etc) while simultaneously working on user vs automatic downloads as well :)

Comment 44

[Sagar Bharadwaj]

Paolo and Nihanth,

Which jobs should I schedule on try server? 
I'm using `./mach try -b do -p all -u all -t none`. Is this fine?

Comment 45

[Sagar Bharadwaj]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68371f00ec89ef65730f1a69ceb5880feb1a2c51

The test results for the last commit. Many failures :(

Comment 46

[Sagar Bharadwaj]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08c28f74021782a2726327e3faf35cf11f44b20

Test results.

Comment 47

[:Paolo Amadini]

Looks like the reviews are still ongoing, let me know when this is ready.

Comment 50

[BugBot [:suhaib / :marco/ :calixte]]

The following field has been copied from duplicate bugs:

Field	Value	Source
Status	NEW	bug 1454370 and bug 1685526

For more information, please visit auto_nag documentation.