Last Comment Bug 723005 - nsNavHistory uses global Private Browsing state to make decisions
: nsNavHistory uses global Private Browsing state to make decisions
Status: VERIFIED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla19
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Marco Bonardo [::mak]
Mentors:
: 799933 (view as bug list)
Depends on: 801049 812639 812828 815363
Blocks: PBnGen mobilepb 799933 806711 806742 806743
  Show dependency treegraph
 
Reported: 2012-01-31 23:10 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2012-12-19 04:21 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP cleanup of private browsing knowledge in history components and marking consumers for review. (28.79 KB, patch)
2012-02-17 22:42 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
WIP cleanup of private browsing knowledge in history components and marking consumers for review. (28.64 KB, patch)
2012-09-21 20:56 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Remove all checks for global privacy status in history-related code, and add them to callers when approprite. (36.90 KB, patch)
2012-09-25 08:33 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
bzbarsky: review+
Details | Diff | Splinter Review
Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s (42.47 KB, patch)
2012-10-29 17:08 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
gavin.sharp: superreview+
Details | Diff | Splinter Review
Remove all checks for global privacy status in history-related code, and add them to callers when approprite. s (60.63 KB, patch)
2012-11-02 12:48 PDT, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Remove all checks for global privacy status in history-related code, and add them to callers when appropriate. s (58.18 KB, patch)
2012-11-13 08:43 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
mak77: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 23:10:58 PST
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.
Comment 1 Marco Bonardo [::mak] 2012-02-01 02:56:39 PST
(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.
Comment 2 Marco Bonardo [::mak] 2012-02-01 03:00:53 PST
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?
Comment 3 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-01 03:07:30 PST
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.
Comment 4 Marco Bonardo [::mak] 2012-02-01 03:27:55 PST
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.
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-01 10:31:58 PST
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.
Comment 6 Marco Bonardo [::mak] 2012-02-01 13:56:14 PST
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.
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-17 22:42:03 PST
Created attachment 598496 [details] [diff] [review]
WIP cleanup of private browsing knowledge in history components and marking consumers for review.
Comment 8 :Ehsan Akhgari 2012-06-26 19:00:44 PDT
Josh, is this something you'll be working on soon?
Comment 9 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-26 19:48:22 PDT
I've been putting it off. Feel free to work on it; I've got a couple other bugs I'm focusing on.
Comment 10 :Ehsan Akhgari 2012-09-20 17:17:15 PDT
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.. :/
Comment 11 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-21 20:56:25 PDT
Created attachment 663638 [details] [diff] [review]
WIP cleanup of private browsing knowledge in history components and marking consumers for review.

Here's a more recent and complete patch. I'm currently running it through tests right now.
Comment 12 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-22 15:09:13 PDT
https://tbpl.mozilla.org/?tree=Try&rev=549157b65e01
Comment 13 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-24 14:56:21 PDT
https://tbpl.mozilla.org/?tree=Try&rev=92bc3d44d4df
Comment 14 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-25 08:33:03 PDT
Created attachment 664522 [details] [diff] [review]
Remove all checks for global privacy status in history-related code, and add them to callers when approprite.
Comment 15 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-09-25 08:36:44 PDT
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 16 Boris Zbarsky [:bz] (still a bit busy) 2012-09-25 09:14:26 PDT
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 17 Marco Bonardo [::mak] 2012-10-01 13:30:04 PDT
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 18 Marco Bonardo [::mak] 2012-10-01 13:44:29 PDT
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
Comment 19 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-29 17:08:17 PDT
Created attachment 676400 [details] [diff] [review]
Remove all checks for global privacy status in history-related code, and add them to callers when approprite.  s

Now with assertions added and comments addressed.
Comment 20 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-29 17:10:20 PDT
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 21 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-10-29 17:10:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f9c4b882b741
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-30 10:29:38 PDT
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 23 Marco Bonardo [::mak] 2012-11-01 07:43:55 PDT
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
Comment 24 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-01 09:40:16 PDT
(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.
Comment 25 Marco Bonardo [::mak] 2012-11-02 05:56:13 PDT
(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.
Comment 26 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-02 12:09:51 PDT
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 27 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-02 12:47:49 PDT
https://tbpl.mozilla.org/?tree=Try&rev=94cfad2b22ab
Comment 28 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-02 12:48:29 PDT
Created attachment 677854 [details] [diff] [review]
Remove all checks for global privacy status in history-related code, and add them to callers when approprite.  s
Comment 29 Marco Bonardo [::mak] 2012-11-07 15:20:19 PST
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)
Comment 30 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-12 09:52:22 PST
>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).
Comment 31 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-13 08:38:40 PST
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 32 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-13 08:43:19 PST
Created attachment 681059 [details] [diff] [review]
Remove all checks for global privacy status in history-related code, and add them to callers when appropriate.  s
Comment 33 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-13 08:44:15 PST
https://tbpl.mozilla.org/?tree=Try&rev=f1ab920d9b14
Comment 34 Marco Bonardo [::mak] 2012-11-14 06:23:21 PST
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
Comment 35 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-14 08:09:43 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7cdf3dbb4c9

Test fixed, try is green for mochitest-browser-chrome.
Comment 36 Ryan VanderMeulen [:RyanVM] 2012-11-14 18:56:52 PST
https://hg.mozilla.org/mozilla-central/rev/b7cdf3dbb4c9
Comment 37 Brian Nicholson (:bnicholson) 2012-11-21 12:10:08 PST
*** Bug 799933 has been marked as a duplicate of this bug. ***
Comment 38 Andreea Pod 2012-12-19 04:21:30 PST
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

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