Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla16

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

({dev-doc-needed})

Trunk
mozilla16
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 2 obsolete attachments)

3.69 KB, patch
Details | Diff | Splinter Review
1.05 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.06 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
Details | Diff | Splinter Review
2.82 KB, patch
Details | Diff | Splinter Review
2.74 KB, patch
Details | Diff | Splinter Review
3.51 KB, patch
mossop
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

5 years ago
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
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #618145 - Flags: review?(bzbarsky)
Created attachment 618147 [details] [diff] [review]
Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API
Attachment #618147 - Flags: review?(dolske)

Comment 4

5 years ago
Comment on attachment 618145 [details] [diff] [review]
Part 1: Add the deleteTemporaryPrivateFileWhenPossible API

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

Comment 6

5 years ago
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+
Created attachment 618152 [details] [diff] [review]
Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API
Attachment #618152 - Flags: review?(gavin.sharp)
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
Attachment #618154 - Flags: review?(bzbarsky)

Comment 9

5 years ago
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+
Created attachment 618155 [details] [diff] [review]
Part 6: Delete the temporary private files when the last private browsing window is closed
Attachment #618155 - Flags: review?(bzbarsky)
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
Attachment #618157 - Flags: review?(bzbarsky)
Attachment 618157 [details] [diff] is the last patch for this bug.  :-)

Comment 13

5 years ago
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 14

5 years ago
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+
https://tbpl.mozilla.org/?tree=Try&rev=e7ecbb548a0d
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.
Attachment #618154 - Attachment is obsolete: true
Keywords: dev-doc-needed
(Reporter)

Comment 17

5 years ago
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?
(Reporter)

Comment 18

5 years ago
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.
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.
Attachment #618152 - Attachment is obsolete: true
Attachment #618399 - Flags: review?(gavin.sharp)
https://tbpl.mozilla.org/?tree=Try&rev=08a534d81e17
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa83d71fe7ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/717f908c990a
https://hg.mozilla.org/integration/mozilla-inbound/rev/86762d8c4de9
https://hg.mozilla.org/integration/mozilla-inbound/rev/66dfa9606626
https://hg.mozilla.org/integration/mozilla-inbound/rev/bde34cebdcdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ceeeaf0519f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6175c97f365
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Oops, resolved too soon!  :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sorry, backed out for:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=a6175c97f365

eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=13076404&tree=Mozilla-Inbound#error0

https://hg.mozilla.org/integration/mozilla-inbound/rev/b912941bed45
Target Milestone: mozilla16 → ---
nsILocalFile is dead!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1b57174183
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd64403cf172
https://hg.mozilla.org/integration/mozilla-inbound/rev/de044548f718
https://hg.mozilla.org/integration/mozilla-inbound/rev/660e21737556
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cec29fdbcf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bdfc3bda7be
https://hg.mozilla.org/integration/mozilla-inbound/rev/481df5e313c5
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/ff1b57174183
https://hg.mozilla.org/mozilla-central/rev/dd64403cf172
https://hg.mozilla.org/mozilla-central/rev/de044548f718
https://hg.mozilla.org/mozilla-central/rev/660e21737556
https://hg.mozilla.org/mozilla-central/rev/4cec29fdbcf4
https://hg.mozilla.org/mozilla-central/rev/4bdfc3bda7be
https://hg.mozilla.org/mozilla-central/rev/481df5e313c5
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
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.