Closed
Bug 794602
Opened 12 years ago
Closed 12 years ago
nsWebBrowserPersist::SaveURIInternal creates a channel out of thin air
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(firefox18+ fixed)
RESOLVED
FIXED
mozilla18
People
(Reporter: jdm, Assigned: jdm)
References
Details
(Keywords: addon-compat, dev-doc-needed)
Attachments
(1 file, 2 obsolete files)
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.
Updated•12 years ago
|
Assignee: nobody → josh
tracking-firefox18:
--- → +
Assignee | ||
Comment 1•12 years ago
|
||
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 :(
Assignee | ||
Updated•12 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #665783 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•12 years ago
|
||
Looping in Neil for the Seamonkey perspective and heads up.
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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.
Attachment #665783 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 6•12 years ago
|
||
This should not require the download changes.
No longer depends on: 795065
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
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?
Attachment #665783 -
Flags: review?(ehsan)
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.
Dave, the devtools stuff?
Attachment #665783 -
Flags: review?(dcamp)
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.
Olli, the content/ stuff?
Attachment #665783 -
Flags: review?(bugs)
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 665783 [details] [diff] [review]
Make nsWebBrowserPersist::SaveURIInternal use channels with valid privacy statuses.
Mark, the mobile/ stuff?
Attachment #665783 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 12•12 years ago
|
||
Shoot, I need to do those requests over again since I forgot the rebased version.
Assignee | ||
Updated•12 years ago
|
Attachment #665783 -
Attachment is obsolete: true
Attachment #665783 -
Flags: review?(mark.finkle)
Attachment #665783 -
Flags: review?(ehsan)
Attachment #665783 -
Flags: review?(dcamp)
Attachment #665783 -
Flags: review?(bugs)
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #667636 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
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/
Attachment #667676 -
Flags: review?(roc)
Attachment #667676 -
Flags: review?(mark.finkle)
Attachment #667676 -
Flags: review?(mak77)
Attachment #667676 -
Flags: review?(jwalker)
Attachment #667676 -
Flags: review?(ehsan)
Attachment #667676 -
Flags: review?(bugs)
Attachment #667676 -
Flags: review?(bmcbride)
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...
Attachment #667676 -
Flags: review?(roc) → review+
Assignee | ||
Comment 16•12 years ago
|
||
Updated•12 years ago
|
Attachment #667676 -
Flags: review?(bmcbride) → review+
Updated•12 years ago
|
Attachment #667676 -
Flags: review?(ehsan) → review+
Updated•12 years ago
|
Attachment #667676 -
Flags: review?(jwalker) → review+
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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.)
Attachment #667676 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Notify comm-central folks about this upcoming change.
Keywords: dev-doc-needed
Updated•12 years ago
|
Attachment #667676 -
Flags: review?(mark.finkle) → review+
Comment 20•12 years ago
|
||
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.
Attachment #667676 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 21•12 years ago
|
||
Assignee | ||
Comment 22•12 years ago
|
||
Backed out. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5e9f36d02e.
Comment 23•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox18:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•