Last Comment Bug 794602 - nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air
: nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Embedding: APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla18
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Myk Melez [:myk] [@mykmelez]
Mentors:
Depends on: 798978 815001 820522
Blocks: pbchannelfail 794606 798685
  Show dependency treegraph
 
Reported: 2012-09-26 13:25 PDT by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-12-11 17:45 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses. (38.50 KB, patch)
2012-09-27 22:32 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
bzbarsky: review+
Details | Diff | Splinter Review
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses. (44.96 KB, patch)
2012-10-03 13:47 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses. (44.95 KB, patch)
2012-10-03 15:05 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
ehsan: review+
ehsan: review+
mark.finkle: review+
roc: review+
jwalker: review+
bugs: review+
blair: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-26 13:25:42 PDT
http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#1219

At minimum, this means that Save Image As in private browsing mode can end up storing the image in the disk cache, which is very bad. I don't the full extent to which nsIWebBrowserPersist is used elsewhere in the code.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-27 10:58:33 PDT
Boris, I'm looking at adding an nsILoadContext parameter to nsIWebBrowserPersist::saveURI (documented thoroughly, I might add). Do you have any opinions on this? The results at https://mxr.mozilla.org/addons/search?string=saveuri show that addons appear to be using this method quite a bit :(
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-27 22:32:21 PDT
Created attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

I'll throw some other reviewers at the consumer bits. I'd like you to review all the WebBrowserPersist stuff, and in particular note my XXX comment. I've walked through the SaveDocument code several times now, and I still can't figure out how StartUpload fits into things.
Comment 3 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-27 22:39:49 PDT
Looping in Neil for the Seamonkey perspective and heads up.
Comment 4 neil@parkwaycc.co.uk 2012-09-28 02:55:49 PDT
(In reply to Josh Matthews from comment #3)
> Looping in Neil for the Seamonkey perspective and heads up.
SeaMonkey (indirectly) uses StartUpload to upload files to FTP. Also I think Composer/Kompozer/NVu etc. might use nsWebBrowserPersist to upload documents (including associated resources) to FTP, but I'm not familiar with that code (perhaps Kaze might be able to help you there). Either way I'm not sure that private browsing applies here, as we're not downloading anything.

As for SaveDocument, my guess is that you want to cache the privacy flag on a boolean member so that you can access it in EnumPersistURIs.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-09-28 20:10:53 PDT
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Comment 4 is spot on.

Also, need to change the IID here, and is there a good reason to make the new method noscript?

r=me modulo all that.
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-02 11:03:08 PDT
This should not require the download changes.
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-02 12:17:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=acd948d87ca6
Comment 8 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 13:43:24 PDT
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Ehsan, can you review the browser-y stuff please?
Comment 9 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 13:44:49 PDT
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Dave, the devtools stuff?
Comment 10 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 13:45:49 PDT
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Olli, the content/ stuff?
Comment 11 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 13:46:15 PDT
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Mark, the mobile/ stuff?
Comment 12 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 13:47:38 PDT
Created attachment 667636 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Shoot, I need to do those requests over again since I forgot the rebased version.
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 15:05:56 PDT
Created attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.
Comment 14 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 15:12:27 PDT
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

Ehsan, browser/, toolkit/content
Smaug, content/
Joe, devtools/
Mark, mobile/
Marco, downloads/
Blair, extensions/
Rob, widget/
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-03 15:29:35 PDT
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

I don't know why OS/2 needs special love here, but whatever...
Comment 16 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-03 15:29:55 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6e52a2333601
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-03 16:06:36 PDT
Optimizer, I r+ed this already because it looks OK to me and because it's a bug that touches lots of parts and therefore hard to get agreement on. Could you take a look and comment that you're OK, or if not, we can either file a follow-up or ask for a change (if we're quick).
Comment 18 Olli Pettay [:smaug] 2012-10-03 17:05:57 PDT
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.


>  * Interface for persisting DOM documents and URIs to local or remote storage.
>  */
>-[scriptable, uuid(dd4e0a6a-210f-419a-ad85-40e8543b9465)]
>+[scriptable, uuid(35c1f231-582b-4315-a26c-a1227e3539b4)]
> interface nsIWebBrowserPersist : nsICancelable
> {
>   /** No special persistence behaviour. */
>   const unsigned long PERSIST_FLAGS_NONE = 0;
>   /** Use cached data if present (skipping validation), else load from network */
>   const unsigned long PERSIST_FLAGS_FROM_CACHE = 1;
>   /** Bypass the cached data. */
>   const unsigned long PERSIST_FLAGS_BYPASS_CACHE = 2;
>@@ -125,27 +126,41 @@ interface nsIWebBrowserPersist : nsICanc
>    *                   <CODE>nullptr</CODE>.
>    * @param aPostData  Post data to pass with an HTTP request or
>    *                   <CODE>nullptr</CODE>.
>    * @param aExtraHeaders Additional headers to supply with an HTTP request
>    *                   or <CODE>nullptr</CODE>.
>    * @param aFile      Target file. This may be a nsIFile object or an
>    *                   nsIURI object with a file scheme or a scheme that
>    *                   supports uploading (e.g. ftp).
>+   * @param aPrivacyContext A context from which the privacy status of this
>+   *                   save operation can be determined. Must only be null
>+   *                   in situations in which no such context is available
>+   *                   (eg. the operation has no logical association with any
>+   *                   window or document)
>    *
>    * @see nsIFile
>    * @see nsIURI
>    * @see nsIInputStream
>    *
>    * @return NS_OK Operation has been started.
>    * @return NS_ERROR_INVALID_ARG One or more arguments was invalid.
>    */
>   void saveURI(in nsIURI aURI, in nsISupports aCacheKey,
>       in nsIURI aReferrer, in nsIInputStream aPostData,
>-      in string aExtraHeaders, in nsISupports aFile);
>+      in string aExtraHeaders, in nsISupports aFile,
>+      in nsILoadContext aPrivacyContext);
>+
>+  /**
>+   * @see saveURI
>+   */
>+  void savePrivacyAwareURI(in nsIURI aURI, in nsISupports aCacheKey,
>+      in nsIURI aReferrer, in nsIInputStream aPostData,
>+      in string aExtraHeaders, in nsISupports aFile,
>+      in boolean aIsPrivate);
This needs better documentation. I have no idea what
aIsPrivate means in this context. (Had to read the method implementation to understand it.)
Comment 19 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-04 08:36:57 PDT
Notify comm-central folks about this upcoming change.
Comment 20 :Ehsan Akhgari 2012-10-04 11:58:17 PDT
Comment on attachment 667676 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.

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

r=me on the rest of the toolkit parts.
Comment 21 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-04 13:23:12 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/e94dbc151121
Comment 22 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-04 19:52:54 PDT
Backed out. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5e9f36d02e.
Comment 23 Mark Banner (:standard8, limited time in Dec) 2012-10-05 02:30:57 PDT
(In reply to Josh Matthews [:jdm] from comment #19)
> Notify comm-central folks about this upcoming change.

Thanks for the heads up, there's only one instance for Thunderbird which I'll fix when this lands, there's SeaMonkey changes required though, so cc'ing some SM folk.
Comment 24 Justin Wood (:Callek) 2012-10-05 02:37:13 PDT
(In reply to Mark Banner (:standard8) from comment #23)
> (In reply to Josh Matthews [:jdm] from comment #19)
> > Notify comm-central folks about this upcoming change.
> 
> Thanks for the heads up, there's only one instance for Thunderbird which
> I'll fix when this lands, there's SeaMonkey changes required though, so
> cc'ing some SM folk.

SeaMonkey also does not have any notion of Private Browsing [yet] so iirc what you said to me before we'd only need |null| passed as an extra param, so it would be helpful if you patch us when you patch TB. If not "one of us" can fix soonish.
Comment 25 Ed Morley [:emorley] 2012-10-05 03:58:03 PDT
https://hg.mozilla.org/mozilla-central/rev/ff5e9f36d02e

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