nsExternalAppHelperService uses global PB service to decide when to remove temporary files

RESOLVED FIXED in mozilla16

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

({dev-doc-needed})

Trunk
mozilla16
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

3.69 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.05 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.06 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.28 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.82 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.74 KB, patch
Details | Diff | Splinter Review
3.51 KB, patch
mossop
: review+
Details | Diff | Splinter Review
The global PB service is going away. nsExternalAppHelperService needs some way of determining which files are temporary for a PB window instance, and deal with them somehow.
We can use the notification in bug 725210 to expunge all current temporary private files in nsExternalAppHelperService::Observe. With regards to nsExternalAppHelper::OpenWithApplication, we can probably store the private status of the related nsIRequest before calling OpenWithApplication. That leaves nsExternalHelperAppService::DeleteTemporaryFileOnExit, which is called in these locations: http://mxr.mozilla.org/mozilla-central/ident?i=DeleteTemporaryFileOnExit

Many of those have ways of accessing a source of privacy information, as far as I can tell, so we can probably pass in a flag to DeleteTemporaryFileOnExit.
Depends on: 725210, 722859
Depends on: 748635
This API is intended to be used by the callers when they want to delete a temporary file created in private browsing mode as soon as possible.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #618145 - Flags: review?(bzbarsky)
Comment on attachment 618145 [details] [diff] [review]
Part 1: Add the deleteTemporaryPrivateFileWhenPossible API

r=me
Attachment #618145 - Flags: review?(bzbarsky) → review+
Comment on attachment 618148 [details] [diff] [review]
Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API

r=me
Attachment #618148 - Flags: review?(bzbarsky) → review+
Comment on attachment 618154 [details] [diff] [review]
Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication

>+        ctx->GetUsePrivateBrowsing(&inPrivateBrowsing);

  inPrivateBrowsing = ctx->UsePrivateBrowsing();

please.

r=me
Attachment #618154 - Flags: review?(bzbarsky) → review+
Attachment 618157 [details] [diff] is the last patch for this bug.  :-)
Comment on attachment 618155 [details] [diff] [review]
Part 6: Delete the temporary private files when the last private browsing window is closed

r=me
Attachment #618155 - Flags: review?(bzbarsky) → review+
Comment on attachment 618157 [details] [diff] [review]
Part 7: Remove the usage of the global private browsing in the external app helper service since it is no longer needed

r=me
Attachment #618157 - Flags: review?(bzbarsky) → review+
Keywords: dev-doc-needed
Comment on attachment 618161 [details] [diff] [review]
Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication

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

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +2235,5 @@
> +    NS_ASSERTION(mRequest, "This should never be called with a null request");
> +    nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
> +    if (channel) {
> +      nsCOMPtr<nsILoadContext> ctx;
> +      NS_QueryNotificationCallbacks(channel, ctx);

This won't work in multiprocess mode. Could you file a followup to make this use the machinery in bug 722845 when it lands?
However, let me be the first to say \o/
Comment on attachment 618147 [details] [diff] [review]
Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API

This just moves the problematic checking of the global PB state to a different place, right? Is there another bug on making the download manager support per-window private browsing?
Attachment #618147 - Flags: review?(dolske) → review+
Comment on attachment 618152 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API

>diff --git a/toolkit/components/viewsource/content/viewSourceUtils.js b/toolkit/components/viewsource/content/viewSourceUtils.js

>+          if (aDocument) {

Are there cases where aDocument can be null that we need to worry about?

>+          webNavigation.QueryInterface(Components.interfaces.nsILoadContext);

This seems to be the viewSource window's docShell, so I don't think using it's PB state is relevant.
Attachment #618152 - Flags: review?(gavin.sharp) → review-
(In reply to Josh Matthews [:jdm] from comment #17)
> Comment on attachment 618161 [details] [diff] [review]
> Part 5: Use the channel's private browsing flag to determine how to handle
> the temporary file in nsExternalAppHandler::OpenWithApplication
> 
> Review of attachment 618161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: uriloader/exthandler/nsExternalHelperAppService.cpp
> @@ +2235,5 @@
> > +    NS_ASSERTION(mRequest, "This should never be called with a null request");
> > +    nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
> > +    if (channel) {
> > +      nsCOMPtr<nsILoadContext> ctx;
> > +      NS_QueryNotificationCallbacks(channel, ctx);
> 
> This won't work in multiprocess mode. Could you file a followup to make this
> use the machinery in bug 722845 when it lands?

Filed bug 748890.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19)
> Comment on attachment 618147 [details] [diff] [review]
> Part 2: Make the download manager aware of the
> deleteTemporaryPrivateFileWhenPossible API
> 
> This just moves the problematic checking of the global PB state to a
> different place, right? Is there another bug on making the download manager
> support per-window private browsing?

Yeah, that's bug 722859.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #20)
> Comment on attachment 618152 [details] [diff] [review]
> Part 4: Make the view source window aware of the
> deleteTemporaryPrivateFileWhenPossible API
> 
> >diff --git a/toolkit/components/viewsource/content/viewSourceUtils.js b/toolkit/components/viewsource/content/viewSourceUtils.js
> 
> >+          if (aDocument) {
> 
> Are there cases where aDocument can be null that we need to worry about?

No, I just imitated the existing null check.
This version of the patch uses the docshell of the correct document.
Attachment #618152 - Attachment is obsolete: true
Attachment #618399 - Flags: review?(gavin.sharp)
Gavin: review ping!
Blocks: 748890
Comment on attachment 618399 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API

Forwarding the request to Mossop.  I'm not sure who should review this, and Gavin implied on IRC that it's not him.
Attachment #618399 - Flags: review?(gavin.sharp) → review?(dtownsend+bugmail)
Comment on attachment 618399 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API

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

::: toolkit/components/viewsource/content/viewSourceUtils.js
@@ +330,5 @@
> +                         .QueryInterface(Components.interfaces.nsILoadContext)
> +                         .usePrivateBrowsing;
> +
> +          let helperService = Components.classes["@mozilla.org/uriloader/external-helper-app-service;1"]
> +                              .getService(Components.interfaces.nsPIExternalAppLauncher);

Line up the .'s here
Attachment #618399 - Flags: review?(dtownsend+bugmail) → review+
Oops, resolved too soon!  :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 618161 [details] [diff] [review]
Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication

>+    // See whether the channel has been opened in private browsing mode
>+    bool inPrivateBrowsing = false;
>+    NS_ASSERTION(mRequest, "This should never be called with a null request");
Actually this happens whenever you turn off Always Ask for a particular download type...
(In reply to comment #34)
> Comment on attachment 618161 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=618161
> Part 5: Use the channel's private browsing flag to determine how to handle the
> temporary file in nsExternalAppHandler::OpenWithApplication
> 
> >+    // See whether the channel has been opened in private browsing mode
> >+    bool inPrivateBrowsing = false;
> >+    NS_ASSERTION(mRequest, "This should never be called with a null request");
> Actually this happens whenever you turn off Always Ask for a particular
> download type...

Can you please file a bug about that?
You need to log in before you can comment on or make changes to this bug.