The global state is going away. The history component is particularly annoying, since it exposes the private browsing state via InPrivateBrowsingMode and IsHistoryDisabled to a bunch of consumers.

(In reply to Josh Matthews [:jdm] from comment #0) > The global state is going away. What does this mean? may I get more data, documentation, blog posts, whatever explaining the changes? > The history component is particularly > annoying, since it exposes the private browsing state via > InPrivateBrowsingMode and IsHistoryDisabled to a bunch of consumers. So, if PB is changing, likely all the way we implement PB support (that was pretty trivial by PB design choice) has to be revised, it's not that simple. Though once I figure out what's happening, we may plot a strategy.

Ah I see, per-window private browsing. We'll have to check cosumers one by one, and history entry points one by one. How do we plan to handle add-ons adding stuff to history while you visit a page in a pb window?

I probably should have added some words to the effect of "going away eventually". Oh well, I'll probably get the same reaction in the other 40 bugs I filed where I left that out. I'm not entirely certain of the overall strategy yet, but chrome code that isn't really associated with a window context feels to me like it should be treated as non-PB.

sorry, I was just a bit surprised :) Btw my fear regarding add-ons are add-ons storing data relative to a certain page you are visiting. So addon A stores data for pages, you visit page1 in a nonPB window and page2 in a PB window, the add-on should be able to distinguish. I suppose it may use the same docshell checks we are going to use, though that should be well documented, and we'll have to put our trust on them.

It doesn't seem particularly different than the existing situation, where add-ons have to implement PB observers to distinguish between modes. We may end up implementing a window-level API that determines whether a given window contains PB docshells, just to make it easy for relevant consumers.

well, it's different in the sense right now history disallows any addition when pb mode is active, but I agree we should require add-ons to actually check pb status BEFORE trying to add stuff. I also agree that a simpler api for them would be really really nice. We will have to audit Places code and file dependencies once there is a design proposal for per-window pb.

Josh, is this something you'll be working on soon?

I've been putting it off. Feel free to work on it; I've got a couple other bugs I'm focusing on.

Josh, can you please update this bug with a list of things that one needs to look into? The mobile folks want this, and I may have to spend some time on it this weekend.. :/

Here's a more recent and complete patch. I'm currently running it through tests right now.

Comment on attachment 664522 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. Boris, can you look at the docshell/uriloader changes (or nominate somebody else to)?

Comment on attachment 664522 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. For docshell, I'd prefer a "ModifyGlobalHistory()" method or something that returns "mUseGlobalHistory && !mInPrivateBrowsing". r=me with that.

Comment on attachment 664522 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. Review of attachment 664522 [details] [diff] [review]: ----------------------------------------------------------------- my concern on the patch is mostly that we are losing a bunch of pb tests, basically we need new tests to ensure we are not adding history and download history from a pb window... as a minimum

Comment on attachment 664522 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. Review of attachment 664522 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to give it a second look, I want to check again entry points in current code... did you already get a green out of this? I was expecting more tests to fail... ::: browser/components/privatebrowsing/test/unit/do_test_placesTitleNoUpdate.js @@ +38,5 @@ > { > do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1); > > pb.privateBrowsingEnabled = true; > + trailing spaces @@ +43,3 @@ > do_check_eq(PlacesUtils.history.getPageTitle(TEST_URI), TITLE_1); > > pb.privateBrowsingEnabled = false; the whole pbenabled = true/false and title check in the middle are sort of pointless to keep since all the rest of the test has been removed, just remove them ::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl @@ +143,5 @@ > + > + /* > + * This input originated from a context designated as private. > + */ > + readonly attribute boolean fromPrivateContext; please get Gavin's SR on this interface change ::: toolkit/components/places/AsyncFaviconHelpers.cpp @@ +455,5 @@ > NS_ENSURE_TRUE(navHistory, NS_ERROR_OUT_OF_MEMORY); > rv = navHistory->CanAddURI(aPageURI, &canAddToHistory); > NS_ENSURE_SUCCESS(rv, rv); > + page.canAddToHistory = !!canAddToHistory && > + aFaviconLoadType != nsIFaviconService::FAVICON_LOAD_PRIVATE; nit: indent by just 2 spaces, or align to the other condition, don't care if it goes over 80 for a single case. ::: toolkit/components/places/nsNavHistory.cpp @@ +3669,5 @@ > + bool isPrivate; > + nsresult rv = input->GetFromPrivateContext(&isPrivate); > + NS_ENSURE_SUCCESS(rv, rv); > + if (isPrivate) > + return NS_OK; Add a comment here, will be nice for future reference ::: toolkit/components/satchel/nsFormFillController.cpp @@ +573,5 @@ > > +NS_IMETHODIMP > +nsFormFillController::GetFromPrivateContext(bool *aFromPrivateContext) > +{ > + *aFromPrivateContext = false; is there a bug filed to cover proper support in formfillcontroller? One may expect it to work and use the API... Please add a comment pointing to the bug. ::: toolkit/content/widgets/autocomplete.xml @@ +165,5 @@ > <property name="searchCount" readonly="true" > onget="this.initSearchNames(); return this.mSearchNames.length;"/> > > + <property name="fromPrivateContext" readonly="true" > + onget="PrivateBrowsingUtils.isPrivateWindow(window)"/> Maybe I'm looking at this wrongly, but it looks broken, you are importing PrivateBrowsingUtils into this in the constructor, so here I was expecting this.PrivateBrowsingUtils... that said, sounds a bit poor, I think you should rather do something like this http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1014

Now with assertions added and comments addressed.

Comment on attachment 676400 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s Gavin, can you sr the interface changes?

Comment on attachment 676400 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s >diff --git a/toolkit/components/autocomplete/nsIAutoCompleteInput.idl b/toolkit/components/autocomplete/nsIAutoCompleteInput.idl > interface nsIAutoCompleteInput : nsISupports >+ /* >+ * This input originated from a context designated as private. >+ */ >+ readonly attribute boolean fromPrivateContext; I would call the attribute "inPrivateContext", and make the comment something more like: "Indicates whether this input is in a "private browsing" context. nsIAutocompleteSearches for these inputs should not persist any data to disk." (and maybe elaborate a bit more on what other things searches shouldn't do if there's more I'm missing).

Comment on attachment 676400 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s Review of attachment 676400 [details] [diff] [review]: ----------------------------------------------------------------- Still some things to fix ::: browser/base/content/browser-places.js @@ +294,2 @@ > PlacesUtils.history.setCharsetForURI(uri, charset); > itemId = PlacesUtils.getMostRecentBookmarkForURI(uri); This is for the addition of a bookmark, that we allow in PB. The addition of a charset means the site has been visited at some time in the past, but maybe being a bookmark we don't care that much, since if you bookmark something it's likely you visited it or will visit it. It doesn't store a timestamp. It's basically like setting the description of the bookmark, if there's a description the page has been visited at some time, but it's a bookmark so that's expected. ::: browser/base/content/browser.js @@ +3490,3 @@ > !aUrlToAdd.contains(" ") && > !/[\x00-\x1F]/.test(aUrlToAdd)) > PlacesUIUtils.markPageAsTyped(aUrlToAdd); There are many other callpoints that invoke .markPageAsXXX utils, these methods add the url to an array of pages, doesn't directly manipulate data, the next added visit uses this data. We need to decide a policy regarding these, with old PB mode we were just skipping all of them. If the policy stays the same, there are more callpoints to fix (especially in PlacesUIUtils) http://mxr.mozilla.org/mozilla-central/search?string=markPageAs ::: docshell/base/nsDownloadHistory.cpp @@ +39,5 @@ > + do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID); > + if (pbService) { > + bool inPrivateBrowsing = false; > + if (NS_SUCCEEDED(pbService->GetPrivateBrowsingEnabled(&inPrivateBrowsing))) { > + MOZ_ASSERT(!inPrivateBrowsing); you can add a message to MOZ_ASSERT to clarify the reason it asserts ::: toolkit/components/places/nsAnnotationService.cpp @@ +284,5 @@ > + // 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. > + nsCOMPtr<nsIPrivateBrowsingService> pbService = > + do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID); What about putting this into mozilla::services? Or we don't cause it's going away? @@ +292,5 @@ > + MOZ_ASSERT(!inPrivateBrowsing); > + } > + } > +#endif > +} - This should be moved to Helpers.h/cpp that is our file used to share utils across all Places compiled codebase. Should also live in the mozilla::places namespace - The function should be wrapped in a macro, not viceversa, so that when we don't need it it's not being called, it's a useless call overhead like this. (note we also have an nsPlacesMacros.h, though if the macro is just wrapping the function it's even fine to put it in Helpers). - unless there's a valid reason I'd like PB to be expanded to the whole name, EnsureNotPB is too cryptic naming. ENSURE_NOT_PRIVATE_BROWSING macro would be nice. ::: toolkit/components/places/nsNavHistory.cpp @@ +3671,5 @@ > return NS_OK; > > + // The eventual goal here is to update a frecency database based on > + // the input received. We want to avoid any such modifications when > + // the source of the input is a private window. "// If the source is a private window don't add any input history." Fwiw, this has nothing to do with frecency. ::: toolkit/components/satchel/nsFormFillController.cpp @@ +582,5 @@ > + nsCOMPtr<nsIDOMDocument> inputDoc; > + mFocusedInput->GetOwnerDocument(getter_AddRefs(inputDoc)); > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(inputDoc); > + nsCOMPtr<nsISupports> container = doc->GetContainer(); > + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(container); does do_QueryInterface(NS_ISUPPORTS_CAST(nsIDocument *, doc)) works? OR alternatively just static_cast<nsISupports*>(doc).. there should not be the need to QI to nsISupports

(In reply to Marco Bonardo [:mak] (Away 2 Nov) from comment #23) > ::: browser/base/content/browser-places.js > @@ +294,2 @@ > > PlacesUtils.history.setCharsetForURI(uri, charset); > > itemId = PlacesUtils.getMostRecentBookmarkForURI(uri); > > This is for the addition of a bookmark, that we allow in PB. The addition of > a charset means the site has been visited at some time in the past, but > maybe being a bookmark we don't care that much, since if you bookmark > something it's likely you visited it or will visit it. It doesn't store a > timestamp. > It's basically like setting the description of the bookmark, if there's a > description the page has been visited at some time, but it's a bookmark so > that's expected. Unfortunately, if I allow this call in PB mode, we trigger the assertion. So I either need to not include the assertions in this patch (or some limited subset of them), or avoid performing this call. > ::: browser/base/content/browser.js > @@ +3490,3 @@ > > !aUrlToAdd.contains(" ") && > > !/[\x00-\x1F]/.test(aUrlToAdd)) > > PlacesUIUtils.markPageAsTyped(aUrlToAdd); > > There are many other callpoints that invoke .markPageAsXXX utils, these > methods add the url to an array of pages, doesn't directly manipulate data, > the next added visit uses this data. > We need to decide a policy regarding these, with old PB mode we were just > skipping all of them. > If the policy stays the same, there are more callpoints to fix (especially > in PlacesUIUtils) > http://mxr.mozilla.org/mozilla-central/search?string=markPageAs Are you in the best position to decide the policy? I don't feel like I'm informed enough to do so. > ::: toolkit/components/places/nsAnnotationService.cpp > @@ +284,5 @@ > > + // 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. > > + nsCOMPtr<nsIPrivateBrowsingService> pbService = > > + do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID); > > What about putting this into mozilla::services? Or we don't cause it's going > away? I don't think it's worth it. > @@ +292,5 @@ > > + MOZ_ASSERT(!inPrivateBrowsing); > > + } > > + } > > +#endif > > +} > > - This should be moved to Helpers.h/cpp that is our file used to share utils > across all Places compiled codebase. Should also live in the mozilla::places > namespace > - The function should be wrapped in a macro, not viceversa, so that when we > don't need it it's not being called, it's a useless call overhead like this. > (note we also have an nsPlacesMacros.h, though if the macro is just wrapping > the function it's even fine to put it in Helpers). > - unless there's a valid reason I'd like PB to be expanded to the whole > name, EnsureNotPB is too cryptic naming. ENSURE_NOT_PRIVATE_BROWSING macro > would be nice. I would fully expect it (and the calls) to be optimized away when MOZ_PER_WINDOW_PRIVATE_BROWSING is defined, but I'll make it a macro regardless. > ::: toolkit/components/satchel/nsFormFillController.cpp > @@ +582,5 @@ > > + nsCOMPtr<nsIDOMDocument> inputDoc; > > + mFocusedInput->GetOwnerDocument(getter_AddRefs(inputDoc)); > > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(inputDoc); > > + nsCOMPtr<nsISupports> container = doc->GetContainer(); > > + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(container); > > does do_QueryInterface(NS_ISUPPORTS_CAST(nsIDocument *, doc)) works? OR > alternatively just static_cast<nsISupports*>(doc).. there should not be the > need to QI to nsISupports I'm not entirely certain what you're referring to; there's no nsISupports QI happening. doc->GetContainer returns an already_AddRefed, so the nsCOMPtr<nsISupports> is necessary.

(In reply to Josh Matthews [:jdm] from comment #24) > (In reply to Marco Bonardo [:mak] (Away 2 Nov) from comment #23) > > ::: browser/base/content/browser-places.js > > @@ +294,2 @@ > > > PlacesUtils.history.setCharsetForURI(uri, charset); > > > itemId = PlacesUtils.getMostRecentBookmarkForURI(uri); > > Unfortunately, if I allow this call in PB mode, we trigger the assertion. So > I either need to not include the assertions in this patch (or some limited > subset of them), or avoid performing this call. This happens just after creating a bookmark (so a page already exists) and goes through setPageAnnotation. Looks like we just disallow adding any page annotation even if those don't track user activity. Though this is compatible to the existing behavior, so I'm fine keeping your check for now but a follow-up should be filed about it, cause here I think we're too strict and loosing valuable info that has no privacy hit. > > ::: browser/base/content/browser.js > > @@ +3490,3 @@ > > > !aUrlToAdd.contains(" ") && > > > !/[\x00-\x1F]/.test(aUrlToAdd)) > > > PlacesUIUtils.markPageAsTyped(aUrlToAdd); > > > > There are many other callpoints that invoke .markPageAsXXX utils, these > > methods add the url to an array of pages, doesn't directly manipulate data, > > the next added visit uses this data. > > Are you in the best position to decide the policy? I don't feel like I'm > informed enough to do so. Not sure, maybe I am. Though in the lack of guidelines I'd say we should keep the old behavior that is to block these calls, we can evaluate at a later time to drop some strictness. > I'm not entirely certain what you're referring to; there's no nsISupports QI > happening. doc->GetContainer returns an already_AddRefed, so the > nsCOMPtr<nsISupports> is necessary. Oh, I missed the ->GetContainer part, so thought it was a plain QI.

Ok, I've carefully looked at every non-test usage of every method I removed PB checks from, and I've made the changes that should achieve feature parity with the existing PB mode. One exception remains; we'll want to ensure that sync does not run in global PB mode, which I believe is already the case. There are various places in the sync code that use methods that used to do nothing, so we'll need to ensure that those don't run while in the global mode.

Comment on attachment 677854 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s Review of attachment 677854 [details] [diff] [review]: ----------------------------------------------------------------- The request I have here, is that you file a bug to add new tests for the most common cases: 1. Adding a visit 2. Adding an icon 3. Autocomplete input history This is basically a r+, though there's still a couple problems/questions to evaluate below ::: browser/components/places/src/PlacesUIUtils.jsm @@ +548,5 @@ > aWindow : this._getTopBrowserWin(); > > + var urls = []; > + for (var i = 0; i < aItemsToOpen.length; i++) { > + var item = aItemsToOpen[i]; while you are touching this code, could you please flip this to use for (let item of aItemsToOpen) ? ::: toolkit/components/places/BookmarkHTMLUtils.jsm @@ +323,5 @@ > let lastModified = this._safeTrim(aElt.getAttribute("last_modified")); > > + let isPrivate = false; > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > + let lastWin = Services.wm.getMostRecentWindow("navigator:browser"); This is polluting toolkit code with browser concepts... I wonder if we shouldn't just do nothing in the bookmarks html case and let it just go through (there's no privacy hit from restoring bookmarks.html) If in these cases we trigger aborts that may be problematic (I don't think, unless tests are hitting them, that anyone wants to restore bookmarks in a debug build) we can take this pollution temporarily but a bug should be filed to solve it soon. @@ +644,5 @@ > * the user visits the page. Our made up one should get expired when the > * page no longer references it. > */ > + _setFaviconForURI: function setFaviconForURI(aPageURI, aIconURI, aData, aIsPrivate) { > + let privacyFlag = PlacesUtils.favicons[aIsPrivate ? "FAVICON_LOAD_PRIVATE" : "FAVICON_LOAD_NON_PRIVATE"]; hm, I'd prefer a more common and readable aIsPrivate ? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE : PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE ::: toolkit/components/places/PlacesUtils.jsm @@ +1394,5 @@ > break; > case this.TYPE_X_MOZ_PLACE: > + let isPrivate = false; > +#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING > + let lastWin = Services.wm.getMostRecentWindow("navigator:browser"); Again this is putting browser concepts into toolkit... I'd probably let this pass through, there's no privacy hit here since we are restoring bookmarks Does these trigger the abort in some test? Same as above basically. ::: toolkit/components/places/nsPlacesMacros.h @@ +38,5 @@ > return _sInstance; \ > } > + > +#if !defined(MOZ_PER_WINDOW_PRIVATE_BROWSING) || !defined(DEBUG) > +# define ENSURE_NOT_PRIVATE_BROWSING please add "nothing" comment like # define ENSURE_NOT_PRIVATE_BROWSING /* nothing */ ::: toolkit/content/widgets/autocomplete.xml @@ +168,5 @@ > + Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm", utils); > + utils.PrivateBrowsingUtils > + </field> > + > + <property name="fromPrivateContext" readonly="true" since you renamed the idl I suppose you may also want to rename this (lack of tests, sigh)

>The request I have here, is that you file a bug to add new tests for the most common cases: >1. Adding a visit >2. Adding an icon >3. Autocomplete input history 1 appears to be covered by browser_visituri_privatebrowsing.js. I've written a test for 2. I'm now trying to figure out how to test the autocomplete stuff, because each idea I come up with is covered by some other component (such as the satchel code for forms, the searchbars-specific stuff in the frontend, etc).

Marco, it doesn't look like the form input-related changes in this patch are easily testable (since the end result is just an addition to the moz_inputhistory database). Given that toolkit/components/satchel/test_privbrowsing.html covers general autocomplete history, I think my work here is finished.

Comment on attachment 681059 [details] [diff] [review] Remove all checks for global privacy status in history-related code, and add them to callers when appropriate. s Review of attachment 681059 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you still have some b-c tests to fix, but I don't have further comments. ::: toolkit/components/places/nsAnnotationService.cpp @@ +281,1 @@ > NS_IMETHODIMP added useless newline here ::: toolkit/components/places/tests/browser/browser_favicon_privatebrowsing.js @@ +1,3 @@ > +/** > + * One-time DOMContentLoaded callback. > + */ missing license header ::: toolkit/content/widgets/autocomplete.xml @@ +168,5 @@ > + Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm", utils); > + utils.PrivateBrowsingUtils > + </field> > + > + <property name="isPrivateContext" readonly="true" This is still wrong, should be inPrivateContext

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7cdf3dbb4c9 Test fixed, try is green for mochitest-browser-chrome.

Verified fixed following the steps from Bug 799933 on: - build: Firefox for Android 20.0a1(2012-12-18) - device: Samsung Galaxy Nexus - OS: Android 4.1.2