Comment on attachment 665658 [details] [diff] [review] Add privacy status to nsDownload. s Better documentation this time?

Note to self: rev nsIDownload iid.

Comment on attachment 665658 [details] [diff] [review] Add privacy status to nsDownload. s sr=me

Looping in comm-central folk for another API change.

I think we only use this in one of our tests (we add a fake download to see whether our sanitiser can remove it); fortunately toolkit's contentAreaUtils handles all the download/persist stuff for us.

Note to self - update NS_DOWNLOAD_CID when revving.

Comment on attachment 665658 [details] [diff] [review] Add privacy status to nsDownload. s Review of attachment 665658 [details] [diff] [review]: ----------------------------------------------------------------- Since I have to look at the dependent patches also, before reviewing the PWPBM-enabled download manager, I'll make a first-pass review here, hoping to give useful input and speed up the process. ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +974,3 @@ > if (!dl) > return NS_ERROR_OUT_OF_MEMORY; > + dl->mPrivate = mInPrivateBrowsing; Note... @@ +1338,5 @@ > if (!dl) > return NS_ERROR_OUT_OF_MEMORY; > > // give our new nsIDownload some info so it's ready to go off into the world > dl->mTarget = aTarget; ...how... @@ +2163,5 @@ > mLastUpdate(PR_Now() - (uint32_t)gUpdateInterval), > mResumedAt(-1), > mSpeed(0), > mHasMultipleFiles(false), > + mPrivate(aPrivacyContext && aPrivacyContext->UsePrivateBrowsing()), ...none of the other properties of nsDownload are initialized in the constructor, initializing mPrivate after construction looks more consistent and you needn't even pass nsnull arbitrarily above. ::: toolkit/components/downloads/nsIDownloadManager.idl @@ +110,5 @@ > * > + * @param aPrivacyContext A context that will be used to determine the privacy > + * status of the new download. May be null if there is > + * no logical context (ie. a source window or document) > + * that can be associated with this operation. From the comment, it's not clear what happens when aPrivacyContext is null. It should at least reply to the questions: - Is the download considered private? - How can I start a download and decide its privacy status when I have no context to start with? ::: toolkit/content/contentAreaUtils.js @@ +388,5 @@ > var targetFileURL = makeFileURI(persistArgs.targetFile); > > + var privacyContext = window.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIWebNavigation) > + .QueryInterface(Ci.nsILoadContext); I get that here we're using the load context of the chrome window because it stores the same flag as all the content window it contains (_setPerWindowPBFlag, http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#122, right?). However this is not the original load context of the page we're saving. It works, what concerns me is that it's not clear from the nsITransfer interface that we can use any load context, as long as it has the correct PBM flag. So, we can't and shouldn't infer other things from the load context (like if the original document was chrome or content window). Given that, I'd feel safer if we had an explicit aPrivate flag on nsITransfer and AddDownload, to prevent future misuse. Also, it's much easier to handle and to explain :-) With a boolean flag we'll not be able to associate the download with a specific window in the future, but doing that would've not been safe in any case, given how we use the parameter now and how add-ons may use it. We could just add a window parameter if and when the need comes.

Here's the patch with Paolo's comments addressed. I won't obsolete the old one in case Marco has in-progress splinter comments.

Comment on attachment 665658 [details] [diff] [review] Add privacy status to nsDownload. s looking directly the new version ( I appreciate the care though, so many times I ended up commenting an old version of a patch :) )

Comment on attachment 666591 [details] [diff] [review] Add privacy status to nsDownload. Review of attachment 666591 [details] [diff] [review]: ----------------------------------------------------------------- it looks ok, though I would like to have a second look once all the patches are updated and questions answered ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +1328,5 @@ > > +#if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING)) && defined(DEBUG) > + // This code makes sure that in global private browsing mode, the flag > + // passed to us matches the global PB mode. This can be removed when > + // per-window private browsing has been turned on. In other patches (the history ones) you are totally removing global pb support... so I'm sort of confused on the current target, if we keep supporting it that patch will break us, otherwise this code sounds no more useful... please clarify, since if we keep it working and allow to switch it on and off the history patch should have all the ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING stuff... @@ +1334,5 @@ > + do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID); > + if (pbService) { > + bool inPrivateBrowsing = false; > + if (NS_SUCCEEDED(pbService->GetPrivateBrowsingEnabled(&inPrivateBrowsing))) { > + MOZ_ASSERT(inPrivateBrowsing == aIsPrivate); nit: fwiw, you may use MOZ_ASSERT_IF @@ +2168,5 @@ > , nsIWebProgressListener2 > ) > > +nsDownload::nsDownload() > + : mDownloadState(nsIDownloadManager::DOWNLOAD_NOTSTARTED), please revert the indentation change here ::: toolkit/content/contentAreaUtils.js @@ +260,5 @@ > * @param aReferrer > * the referrer URI object (not URL string) to use, or null > * if no referrer should be sent. > + * @param aInitiatingDocument > + * The document from which the save was initiated. So, is it optional? cause it looks like it is from the code below... you should specify [optional], what it is used for and what happens by default... But actually, is there a specific reason this must be optional? Some case where we don't know the document? cause otherwise I'd just make it mandatory... @@ +374,5 @@ > * "text/plain" is meaningful. > * @param persistArgs.bypassCache > * If true, the document will always be refetched from the server > + * @param persistArgs.initiatingWindow > + * The window from which the save operation was initiated. ditto

>::: toolkit/components/downloads/nsDownloadManager.cpp >@@ +1328,5 @@ >> >> +#if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING)) && defined(DEBUG) >> + // This code makes sure that in global private browsing mode, the flag >> + // passed to us matches the global PB mode. This can be removed when >> + // per-window private browsing has been turned on. > >In other patches (the history ones) you are totally removing global pb support... so I'm sort >of confused on the current target, if we keep supporting it that patch will break us, >otherwise this code sounds no more useful... please clarify, since if we keep it working and >allow to switch it on and off the history patch should have all the ifdef >MOZ_PER_WINDOW_PRIVATE_BROWSING stuff... This is a temporary check to help catch regressions while we are still using the global PB service in Firefox. It's been useful in other areas of the code; I suppose I can re-add the checks in the history patch on a similar basis.

In fact, the assertion was very useful and caught some badness that was happening.

(In reply to Josh Matthews [:jdm] from comment #13) > while we are still using > the global PB service in Firefox. This is what concerns me, your other patches don't allow us to keep using global PB, so we need a final decision, either we keep them alongside or we throw away global PB, there's no way to do half and half.

(In reply to Marco Bonardo [:mak] from comment #16) > (In reply to Josh Matthews [:jdm] from comment #13) > > while we are still using > > the global PB service in Firefox. > > This is what concerns me, your other patches don't allow us to keep using > global PB, so we need a final decision, either we keep them alongside or we > throw away global PB, there's no way to do half and half. I don't understand your concern. All of the per-window PB changes are designed to continue working alongside the global service (which sets the privacy information of the root docshell of each window and propagates through children), and we can change the front-end to start doing per-window and everything should just work (at which point we'll take out the debug asserts).

(In reply to Josh Matthews [:jdm] from comment #17) > I don't understand your concern. Could be I didn't express properly. So I'm concerned about this: #if !(defined(MOZ_PER_WINDOW_PRIVATE_BROWSING)) This means PER_WINDOW_PRIVATE_BROWSING can be disabled at build time... If this happens global PB is the only remaining PB mode. Though in bug 723005 you removed any global PB block from history, so if per window PB is disabled, global PB will just not work.

That's just a build flag we're using to allow us to make certain all-or-nothing changes before we actually reach the point where we're ready to turn on per-window PB for desktop Firefox. We're not releasing any builds with that turned on, it's not documented anywhere, and it's going away.

Actually, I still am having trouble understanding your concern. Here's the situation: * MOZ_PER_WINDOW_PRIVATE_BROWSING is a build flag that is disabled by default * Global private browsing is the only exposed way of using private browsing in the browser * These assertion blocks are turned on by default. If someone builds with MOZ_PER_WINDOW_PRIVATE_BROWSING enabled, the assertions disappear. The code in (for example) the history changes doesn't stop checking the calling sites - ideally there is no change in behaviour, because all problematic calls to history-related functions have been caught via the assertions and fixed at the callers.

(In reply to Josh Matthews [:jdm] from comment #20) > all problematic calls to > history-related functions have been caught via the assertions and fixed at > the callers. Yes my fear was that MOZ_PER_WINDOW_PRIVATE_BROWSING was also disabling the callers checks about private window, but looks like it's not the case. So there should be no problem. So per window data is available regardless this PER_WINDOW_PB... just that the name of the config is sort of confusing :)

I'm not sure how I missed so many call sites for the internalSave stuff, but this makes try happy.

Sorry, too many trees and versions of patches.

Comment on attachment 667140 [details] [diff] [review] Add privacy status to nsDownload. s >+ Math.round(Date.now() * 1000), null, persist, isPrivate); I'll fix this before landing. >+ var aInitiatingDoc = params.initiatingDoc ? params.initiatingDoc : null; You may be surprised by this, given that I made the other similar values required. However, I audited all the call sites of openLinkIn and openLink, and the times in which this information is required (for "save") are quite rare.

Comment on attachment 666796 [details] [diff] [review] Add privacy status to nsDownload. s Review of attachment 666796 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +972,5 @@ > // We have a download, so lets create it > nsRefPtr<nsDownload> dl = new nsDownload(); > if (!dl) > return NS_ERROR_OUT_OF_MEMORY; > + dl->mPrivate = mInPrivateBrowsing; Paolo has a point about not needing to do complicate initialization in the constructor, though I'd still prefer if mPrivate had a default false value in the constructor initializers than a random value. ::: toolkit/components/downloads/test/unit/test_sleep_wake.js @@ +142,5 @@ > nsIWBP.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION; > let dl = dm.addDownload(nsIDM.DOWNLOAD_TYPE_DOWNLOAD, > createURI("http://localhost:4444/resume"), > createURI(destFile), null, null, > + Math.round(Date.now() * 1000), null, persist, null); should this be false rather than null? ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +1946,3 @@ > rv = aTransfer->Init(mSourceUrl, target, EmptyString(), > + mMimeInfo, mTimeDownloadStarted, mTempFile, this, > + channel && NS_UsePrivateBrowsing(channel)); wrong indentation

Comment on attachment 667140 [details] [diff] [review] Add privacy status to nsDownload. s Review of attachment 667140 [details] [diff] [review]: ----------------------------------------------------------------- yes, the risk is to have missed some callpoints, though even if I'd spend a full day searching mxr to ensure that, it's likely I'd still miss some, so no point in doing that, at least fixing them should be easy. ::: browser/base/content/utilityOverlay.js @@ +224,5 @@ > var aInBackground = params.inBackground; > var aDisallowInheritPrincipal = params.disallowInheritPrincipal; > // Currently, this parameter works only for where=="tab" or "current" > var aIsUTF8 = params.isUTF8; > + var aInitiatingDoc = params.initiatingDoc ? params.initiatingDoc : null; why the check? at a maximum it's undefined, not a problem for your usage ::: mobile/android/chrome/content/browser.js @@ +501,5 @@ > contentDisposition = ""; > type = ""; > } > + ContentAreaUtils.internalSave(aTarget.currentURI.spec, null, null, contentDisposition, type, false, "SaveImageTitle", null, aTarget.ownerDocument.documentURIObject, > + aTarget.ownerDocument, true, null); nit: reindent this properly?

https://tbpl.mozilla.org/?tree=Try&rev=9ad4c3c96927 https://hg.mozilla.org/integration/mozilla-inbound/rev/eff426a003e1

For applications like Thunderbird (or Sunbird) where there are no PB windows what sort of document do you pass to saveURL() or internalSave()?

If there are no plans for ever implementing something based on PB mode, it doesn't matter what document is passed.

In SeaMonkey I get this error message: > Sat Oct 06 2012 23:34:48 > Error: ReferenceError: Ci is not defined > Source file: chrome://global/content/contentAreaUtils.js > Line: 434 contentAreaUtils.js should not assume that Ci/Cc/Cr is defined in the global scope since it's used in more than Firefox.

Comment on attachment 668789 [details] [diff] [review] SeaMonkey bustage fix. Review of attachment 668789 [details] [diff] [review]: ----------------------------------------------------------------- ops, thanks for the fix.

Thanks. Setting checkin-needed because I don't have -inbound locally.

(In reply to Philip Chee from comment #33) > Thanks. Setting checkin-needed because I don't have -inbound locally. https://hg.mozilla.org/integration/mozilla-inbound/rev/c33b5e11209a

(In reply to Ed Morley [:edmorley UTC+1] from comment #35) > https://hg.mozilla.org/mozilla-central/rev/c33b5e11209a This part landed on mozilla19.

Comment on attachment 668789 [details] [diff] [review] SeaMonkey bustage fix. [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 795065 User impact if declined: SeaMonkey errors when trying to save a page: <https://bugzilla.mozilla.org/show_bug.cgi?id=799529#c2> Save Page as works with 'Text', but not with the 'HTML' (Web page) options. Download Manager says: Unable to save, Not started > Error: ReferenceError: Ci is not defined > Source file: chrome://global/content/contentAreaUtils.js > Line: 434 Line 434: > .QueryInterface(Ci.nsIInterfaceRequestor) Save Image As is not working at all. > Error: TypeError: aInitiatingDocument is undefined > Source file: chrome://global/content/contentAreaUtils.js > Line: 340 Line 340: > initiatingWindow : aInitiatingDocument.defaultView Testing completed (on m-c, etc.): mozilla-central, comm-central. Risk to taking this patch (and alternatives if risky): no risk bustage fix. String or UUID changes made by this patch: None.

Comment on attachment 668789 [details] [diff] [review] SeaMonkey bustage fix. approving low-risk, bustage fix for 17.0 on Beta.

Please see Bug 802472 and the Mozilla forum discussion: http://forums.mozillazine.org/viewtopic.php?f=23&t=2576141&p=12386465#p12386465 Several (all?) of the screenshot extensions aren't working. Bug 795065 was marked as fixed on 11 Oct, and the screenshot issue apparently started as a result.

Comment on attachment 667140 [details] [diff] [review] Add privacy status to nsDownload. s > function ViewSourceSavePage() > { > internalSave(window.content.location.href.substring(12), > null, null, null, null, null, >- "SaveLinkTitle", null, null, null, gPageLoader); >+ "SaveLinkTitle", null, null, gDocument, null, gPageLoader); > } Unfortunately view source doesn't have a gDocument variable. Bug coming up.

This change breaks add-ons which call saveURI(), such as Thumbnail Zoom Plus' "Save Enlarged Image As..." command. Are add-on developers expected to add the argument to their code in saveURI calls (and also nsITransfer init() calls), or should I file a bug about this add-on incompatibility? David Adler (developer of Thumbnail Zoom Plus)

As I understand it, this is a deliberate change in the API. Going forward AMO will require extension authors to respect the private browsing status of Firefox so you might as well start passing the right parameters. Phil Chee (developer of Flashblock and others)

Is there a doc anywhere explaining motivation for the change? EG is this to support a future feature where some windows can be in Private Browsing and others not? More to the point, how can I test to verify that my add-on is properly honoring private status? The various docs which discuss how to use saveURI haven't been updated yet. I could simply pass false to transfer.init() and null to persist.saveURI() but would that be correct? The code below seems to work in my add-on but I don't know if it is all necessary (win is the html doc's window, not the chrome window): let privacyContext = win.QueryInterface(Ci.nsIInterfaceRequestor) .getInterface(Ci.nsIWebNavigation) .QueryInterface(Ci.nsILoadContext); let isPrivate = privacyContext.usePrivateBrowsing; let transfer = Cc["@mozilla.org/transfer;1"].createInstance(Ci.nsITransfer); transfer.init(source, target, "", null, null, null, persist, isPrivate); persist.saveURI(source, null, null, null, null, aFile, privacyContext);

Yes, this change was a requirement for the upcoming work to support concurrent private and public windows. You can test it by performing actions in private browsing that trigger downloads via your add-on, and checking the disk cache (about:cache) for any revealing traces of the files downloaded. Your code looks correct to me, with the exception of the transfer.init call which is quite incorrect. init takes a load context only. You can make the code more maintainable by using the PrivateBrowsingUtils module (resource://gre/modules/PrivateBrowsingUtils.jsm) to both obtain the load context (PrivateBrowsingUtils.loadContextFromWindow(win)) and the privacy status you care about (PrivateBrowsingUtils.isWindowPrivate(win)).

Sorry, that should have been PrivateBrowsing.privacyContextFromWindow, not loadContextFromWindow. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/PrivateBrowsingUtils.jsm

I'll use PRivateBrowsingUtils; thanks for the tip. Re transfer.init(), it seems to expect a boolean; note that this is an nsITransfer, not an nsITransferable. From http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsITransfer.idl#14 48 * @param aIsPrivate Used to determine the privacy status of the new transfer. 49 * If true, indicates that the transfer was initiated from 50 * a source that desires privacy. 51 */ 52 void init(in nsIURI aSource, 53 in nsIURI aTarget, 54 in AString aDisplayName, 55 in nsIMIMEInfo aMIMEInfo, 56 in PRTime startTime, 57 in nsIFile aTempFile, 58 in nsICancelable aCancelable, 59 in boolean aIsPrivate);

Ack, I'm sorry. I constantly get those two interfaces confused. Carry on.

In case it helps anyone else, this is what I end up with (including ability to work with older Firefox versions including 3.6): try { Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm"); } catch (e) { // old Firefox versions (e.g. 3.6) didn't have PrivateBrowsingUtils. } ... _saveImage : function(win, imageURL, aFile) { this._logger.trace("_saveImage"); let ioService = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService); let persist = Cc["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]. createInstance(Ci.nsIWebBrowserPersist); let source = ioService.newURI(imageURL, "UTF8", null); let target = ioService.newFileURI(aFile); if (win && "undefined" != typeof(PrivateBrowsingUtils) && PrivateBrowsingUtils.privacyContextFromWindow) { var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(win); var isPrivate = privacyContext.usePrivateBrowsing; } else { // older than Firefox 19 or couldn't get window. var privacyContext = null; var isPrivate = false; } this._logger.debug("_saveImage: privacyContext=" + privacyContext + "; isPrivate=" + isPrivate); // set persist flags persist.persistFlags = (Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION); // Create a 'transfer' object and set it as the progress listener. // This causes the downloaded image to appear in the Firefox // "Downloads" dialog. let transfer = Cc["@mozilla.org/transfer;1"].createInstance(Ci.nsITransfer); transfer.init(source, target, "", null, null, null, persist, isPrivate); persist.progressListener = transfer; // save image to the file persist.saveURI(source, null, null, null, null, aFile, privacyContext); }

I think would be useful for everyone to have this conversation in mozilla.dev.extensions and I suspect there should be more insight there for developers and what they need to do.

Comment on attachment 667140 [details] [diff] [review] Add privacy status to nsDownload. s >diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js >@@ -104,17 +104,18 @@ function openUILink(url, event, aIgnoreB >+ initiatingDoc: event.target.ownerDocument requiring a non-null "event" for openUILink is going to cause lots of extension bustage (and caused bustage in Firefox itself, see bug 805523). Passing null if no event is specified seems like the better call here - it means failing to properly handle PB mode in the rare instances where we'll use the "save" action, but that should be a very small minority of cases, and we can add a warning for that or something. I'll file a bug.