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

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Josh Matthews [:jdm]]

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]]

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

[(no longer active)]

Attachment: 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

[(no longer active)]

Attachment: Part 2: Make the download manager aware of the deleteTemporaryPrivateFileWhenPossible API

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 5

[(no longer active)]

Attachment: Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 618148 [details] [diff] [review]
Part 3: Make insExternalAppHandler::OpenWithApplication aware of the deleteTemporaryPrivateFileWhenPossible API

r=me

Comment 7

[(no longer active)]

Attachment: Part 4: Make the view source window aware of the deleteTemporaryPrivateFileWhenPossible API

Comment 8

[(no longer active)]

Attachment: Part 5: Use the channel's private browsing flag to determine how to handle the temporary file in nsExternalAppHandler::OpenWithApplication

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

[(no longer active)]

Attachment: Part 6: Delete the temporary private files when the last private browsing window is closed

Comment 11

[(no longer active)]

Attachment: Part 7: Remove the usage of the global private browsing in the external app helper service since it is no longer needed

Comment 12

[(no longer active)]

Attachment 618157 [details] [diff] is the last patch for this bug.  :-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 15

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=e7ecbb548a0d

Comment 16

[(no longer active)]

Attachment: 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]]

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]]

However, let me be the first to say \o/

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

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

[(no longer active)]

(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

[(no longer active)]

(In reply to :Gavin Sharp (use [e-mail] 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

[(no longer active)]

(In reply to :Gavin Sharp (use [e-mail] 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

[(no longer active)]

Attachment: 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 25

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=08a534d81e17

Comment 26

[(no longer active)]

Gavin: review ping!

Comment 27

[(no longer active)]

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]]

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 29

[(no longer active)]

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

Comment 30

[(no longer active)]

Oops, resolved too soon!  :-)

Comment 31

[Ed Morley [:emorley]]

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

Comment 32

[(no longer active)]

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

Comment 33

[Ed Morley [:emorley]]

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

Comment 34

[[e-mail]]

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

[(no longer active)]

(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?