Last Comment Bug 722868 - nsExternalAppHelperService uses global PB service to decide when to remove temporary files
: nsExternalAppHelperService uses global PB service to decide when to remove te...
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: :Ehsan Akhgari
:
:
Mentors:
Depends on: 722859 725210 748635
Blocks: PBnGen 748890
  Show dependency treegraph
 
Reported: 2012-01-31 14:12 PST by Josh Matthews [:jdm]
Modified: 2012-09-11 10:33 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add the deleteTemporaryPrivateFileWhenPossible API (3.69 KB, patch)
2012-04-24 19:58 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API (1.05 KB, patch)
2012-04-24 20:02 PDT, :Ehsan Akhgari
gavin.sharp: review+
Details | Diff | Splinter Review
Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API (1.06 KB, patch)
2012-04-24 20:06 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API (3.29 KB, patch)
2012-04-24 20:15 PDT, :Ehsan Akhgari
gavin.sharp: review-
Details | Diff | Splinter Review
Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication (2.74 KB, patch)
2012-04-24 20:32 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 6: Delete the temporary private files when the last private browsing window is closed (1.28 KB, patch)
2012-04-24 20:36 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 7: Remove the usage of the global private browsing in the external app helper service since it is no longer needed (2.82 KB, patch)
2012-04-24 20:38 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication (2.74 KB, patch)
2012-04-24 20:48 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API (3.51 KB, patch)
2012-04-25 12:49 PDT, :Ehsan Akhgari
dtownsend: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-01-31 14:12:04 PST
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.
Comment 1 Josh Matthews [:jdm] 2012-04-01 10:01:47 PDT
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.
Comment 2 :Ehsan Akhgari 2012-04-24 19:58:45 PDT
Created attachment 618145 [details] [diff] [review]
Part 1: Add the deleteTemporaryPrivateFileWhenPossible API

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.
Comment 3 :Ehsan Akhgari 2012-04-24 20:02:50 PDT
Created attachment 618147 [details] [diff] [review]
Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:04:12 PDT
Comment on attachment 618145 [details] [diff] [review]
Part 1: Add the deleteTemporaryPrivateFileWhenPossible API

r=me
Comment 5 :Ehsan Akhgari 2012-04-24 20:06:10 PDT
Created attachment 618148 [details] [diff] [review]
Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:09:00 PDT
Comment on attachment 618148 [details] [diff] [review]
Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API

r=me
Comment 7 :Ehsan Akhgari 2012-04-24 20:15:13 PDT
Created attachment 618152 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API
Comment 8 :Ehsan Akhgari 2012-04-24 20:32:47 PDT
Created 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
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:35:40 PDT
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
Comment 10 :Ehsan Akhgari 2012-04-24 20:36:19 PDT
Created attachment 618155 [details] [diff] [review]
Part 6: Delete the temporary private files when the last private browsing window is closed
Comment 11 :Ehsan Akhgari 2012-04-24 20:38:38 PDT
Created 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
Comment 12 :Ehsan Akhgari 2012-04-24 20:39:11 PDT
Attachment 618157 [details] [diff] is the last patch for this bug.  :-)
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:42:32 PDT
Comment on attachment 618155 [details] [diff] [review]
Part 6: Delete the temporary private files when the last private browsing window is closed

r=me
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 20:43:02 PDT
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
Comment 16 :Ehsan Akhgari 2012-04-24 20:48:59 PDT
Created 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

Addressed the review comment.
Comment 17 Josh Matthews [:jdm] 2012-04-24 21:08:34 PDT
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?
Comment 18 Josh Matthews [:jdm] 2012-04-24 21:08:54 PDT
However, let me be the first to say \o/
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-25 06:58:11 PDT
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?
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-25 11:17:59 PDT
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.
Comment 21 :Ehsan Akhgari 2012-04-25 12:03:49 PDT
(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.
Comment 22 :Ehsan Akhgari 2012-04-25 12:30:25 PDT
(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.
Comment 23 :Ehsan Akhgari 2012-04-25 12:31:01 PDT
(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.
Comment 24 :Ehsan Akhgari 2012-04-25 12:49:43 PDT
Created attachment 618399 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API

This version of the patch uses the docshell of the correct document.
Comment 26 :Ehsan Akhgari 2012-06-26 18:49:01 PDT
Gavin: review ping!
Comment 27 :Ehsan Akhgari 2012-06-27 10:36:13 PDT
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.
Comment 28 Dave Townsend [:mossop] 2012-06-28 07:36:38 PDT
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
Comment 30 :Ehsan Akhgari 2012-06-28 10:54:01 PDT
Oops, resolved too soon!  :-)
Comment 34 neil@parkwaycc.co.uk 2012-09-11 08:06:43 PDT
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...
Comment 35 :Ehsan Akhgari 2012-09-11 10:33:38 PDT
(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?

Note You need to log in before you can comment on or make changes to this bug.