741059 - Make network favicon requests inherit private browsing context of document

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Josh Matthews [:jdm]]

Currently we just create a channel out of thin air with no context. We can't do that any longer; we need to find a way to get the privacy status of the document's load context into the request that eventually gets made.

Comment 1

[(no longer active)]

Where does the code which create this channel live?

Comment 2

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp#632

Comment 3

[Josh Matthews [:jdm]]

Ugh, I am not pleased with this. I really only can see us passing a boolean privacy value down through the callers of mozIAsyncFavicons.setAndFetchFaviconForPage, querying the new channel in AsyncFetchAndSetIconFromNetwork::Run for nsIPrivateBrowsingConsumer, and adding an override method to that interface which forces a particular privacy status.

Comment 4

[Josh Matthews [:jdm]]

This became a little bit harder with the removal of nsIPrivateBrowsingConsumer. It looks like AsyncFetchAndSetIconFromNetwork needs to implement nsILoadContext and store the privacy status that gets passed in.

Comment 5

[(no longer active)]

(In reply to comment #4)
> This became a little bit harder with the removal of nsIPrivateBrowsingConsumer.
> It looks like AsyncFetchAndSetIconFromNetwork needs to implement nsILoadContext
> and store the privacy status that gets passed in.

Why does it need to implement nsILoadContext?

Comment 6

[Josh Matthews [:jdm]]

Because the AsyncFetchAndSetIconFromNetwork::Run sets the notification callback of the new channel to |this| (begin AsyncFetchAndSetIconFromNetwork), and we now always determine the privacy status of a channel based on getting nsILoadContext from the notification callbacks.

Comment 7

[(no longer active)]

(In reply to comment #6)
> Because the AsyncFetchAndSetIconFromNetwork::Run sets the notification callback
> of the new channel to |this| (begin AsyncFetchAndSetIconFromNetwork), and we
> now always determine the privacy status of a channel based on getting
> nsILoadContext from the notification callbacks.

Ah, you're right.  Thanks!

Comment 8

[(no longer active)]

We need to fix this for Firefox 18 to address bug 787743.

Comment 9

[(no longer active)]

I talked about a solution to make it possible for nsIChannel users to override the privacy bit on a channel, and we came to the conclusion that a new nsIChannel::SetPrivate method is the way to go.  I went around to implement that, but I realized that there is another problem.  Currently we use NS_UsePrivateBrowsing to decide whether a channel is private, and that just calls NS_QueryNotificationCallbacks on the channel and then calls UsePrivateBrowsing on the resulting nsILoadContext.  That means that the SetPrivate override must either modify the load context (which is not even possible since the channel might not even have a load context) or we need to add yet another method on nsIChannel to determine whether the private bit has been overridden.

I started to add a [noscript] bool isPrivateModeOverriden() method on nsIChannel but I decided to sleep on this to make sure I can't think of a better solution.

And just for the record, this sucks -- big time.  :(

Comment 10

[Jason Duell]

How about

nsIChannel
  [noscript] bool usingPrivateBrowsing

and just have the channel impls first check a 'mOverridePB' member variable (say an enum with UNSET, TRUE, FALSE) and fallback to checking the callbacks for nsILoadContext?

Comment 11

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #10)
> How about
> 
> nsIChannel
>   [noscript] bool usingPrivateBrowsing
> 
> and just have the channel impls first check a 'mOverridePB' member variable
> (say an enum with UNSET, TRUE, FALSE) and fallback to checking the callbacks
> for nsILoadContext?

That would mean that we have to repeat the override bit checking logic in all of the channel implementations, as opposed to just the NS_UsePrivateBrowsing function, which is suboptimal.

Comment 12

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

DISCLAIMER: This is the worst patch that I have ever written.  Do not look at it if you're after beautiful things.  Do not even look at it if you're in a good mood.

This adds the setPrivate API as we discussed and it goes through a whole bunch of other necessary cruft which I came across as I was working on it.  Its only plus side is that it works, and it helps fix this bug.

Comment 13

[(no longer active)]

Attachment: Part 2: Use nsIChannel::SetPrivate in the async favicon loader

Comment 14

[(no longer active)]

Attachment: Part 3: Update the callers to nsIFaviconService::SetAndLoadFaviconForPage

Comment 15

[(no longer active)]

Attachment: Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

Comment 16

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=012e30bbecc8

Comment 17

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

I added the tests to the wrong patch.

Comment 18

[(no longer active)]

Attachment: Part 2: Use nsIChannel::SetPrivate in the async favicon loader

Comment 19

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs\

Fixed a number of build errors.

Comment 20

[(no longer active)]

Attachment: Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

Fixed a number of test failures.

Comment 21

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=989103733741

Comment 22

[Mozilla RelEng Bot]

Try run for 012e30bbecc8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=012e30bbecc8
Results (out of 127 total builds):
    exception: 5
    success: 55
    warnings: 24
    failure: 43
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-012e30bbecc8

Comment 23

[(no longer active)]

Try is fairly green on my latest push.

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 658752 [details] [diff] [review]
Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

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

All of this looks ok to me, but I probably shouldn't be the official reviewer.

::: browser/components/feeds/src/FeedWriter.js
@@ +1327,5 @@
>        return;
>      }
>      var faviconURI = makeURI(readerURI.prePath + "/favicon.ico");
>      var self = this;
> +    var usePrivateBrowsing = this._window.QueryInterface(Ci.nsIInterfaceRequestor)

Can this use the PrivateBrowsingUtils instead?

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 658719 [details] [diff] [review]
Part 3: Update the callers to nsIFaviconService::SetAndLoadFaviconForPage

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

rs=me. mbrubeck says we probably don't even need to change this, but I couldn't get ahold of romaxa to see if he's still making Fennec builds for Meego.

Comment 26

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #24)
> ::: browser/components/feeds/src/FeedWriter.js
> @@ +1327,5 @@
> >        return;
> >      }
> >      var faviconURI = makeURI(readerURI.prePath + "/favicon.ico");
> >      var self = this;
> > +    var usePrivateBrowsing = this._window.QueryInterface(Ci.nsIInterfaceRequestor)
> 
> Can this use the PrivateBrowsingUtils instead?

Yes, I'll do that.

Comment 27

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

I realized that I need to make the checks added inside SetNotificationCallbacks implementations a little bit more relaxed since the notification callback passed in may not provide an nsILoadContext, in which case it's OK to have the override bit set.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 658752 [details] [diff] [review]
Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

>+                                         .QueryInterface(Ci.nsIDocShell)
>+                                         .QueryInterface(Ci.nsILoadContext)

The first of those two lines is not needed.

r=me with that.

Comment 29

[Jason Duell]

Comment on attachment 658726 [details] [diff] [review]
Part 2: Use nsIChannel::SetPrivate in the async favicon loader

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

Ehsan: thanks for wading into all of this.

::: toolkit/components/places/nsIFaviconService.idl
@@ +81,5 @@
>    [deprecated]
>    void setAndLoadFaviconForPage(in nsIURI aPageURI,
>                                  in nsIURI aFaviconURI,
>                                  in boolean aForceReload,
> +                                in unsigned long aFaviconLoadType,

Any reason we chose not to make the LoadType argument optional (defaulting to NOT_PRIVATE if not passed)?  It makes for a long slew of changes to test code, etc, all of which set NOT_PRIVATE.   My plan for the B2G followup to this is to change the 'long' here to an object (a subset of nsILoadContext), and it will be even more tedious for all those callers to construct temp objects when they really all want the same default value {NOT_PRIVATE, appID=0, notInBrowser}.

Comment 30

[Jason Duell]

Comment on attachment 658752 [details] [diff] [review]
Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

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

bz or whomever: please correct me if I'm wrong, but it looks like we have two types of callers here:

1) Contexts (tabbrowser, feedwriter, nsDocShell) which have a docShell handy, and could thus be passing in a LoadContext just as easily as the isPrivate arg here. (I've added comments on how to get them)

2) Contexts that are always going to be in the parent process (BookmarkHTMLUtils, callers of PlacesUtils.restoreBookmarksFromJSONFile, the tests), and always want to pass NOT_PRIVATE, and so could work without change if we have the IDLs take an optional LoadContext that defaults to {NO_PRIVATE, appID=0, isInBrowerElement=false}.

If that's true can we just change these patches so that the IDLs take a nsILoadContext?  Seems trivial enough and would avoid IDL churn, as I'll have to write that patch anyway pretty much immediately for B2G v1.

If it's not true (i.e. there are callers here that always call with NOT_PRIVATE but may live in an app in the child process in B2G), than I need to know ASAP so I can try to figure out how B2G will get an appID for them.

::: browser/base/content/tabbrowser.xml
@@ +676,5 @@
>                this.mFaviconService.setAndFetchFaviconForPage(browser.currentURI,
> +                                                             aURI, false,
> +                                                             gPrivateBrowsingUI.privateWindow ?
> +                                                               this.mFaviconService.FAVICON_LOAD_PRIVATE :
> +                                                               this.mFaviconService.FAVICON_LOAD_NON_PRIVATE);

Could pass in browser.docShell here.

::: browser/components/feeds/src/FeedWriter.js
@@ +1331,5 @@
> +    var usePrivateBrowsing = this._window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                         .getInterface(Ci.nsIWebNavigation)
> +                                         .QueryInterface(Ci.nsIDocShell)
> +                                         .QueryInterface(Ci.nsILoadContext)
> +                                         .usePrivateBrowsing;

could pass in nsILoadContext here instead of its .usePrivateBrowsing

::: docshell/base/nsDocShell.cpp
@@ +8871,5 @@
>              }
>  
>              // Inform the favicon service that the favicon for oldURI also
>              // applies to aURI.
> +            CopyFavicon(oldURI, aURI, mInPrivateBrowsing);

could pass 'this' since nsDocShell implements nsILoadContext.

Comment 31

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #29)
> Comment on attachment 658726 [details] [diff] [review]
> Part 2: Use nsIChannel::SetPrivate in the async favicon loader
> 
> Review of attachment 658726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ehsan: thanks for wading into all of this.
> 
> ::: toolkit/components/places/nsIFaviconService.idl
> @@ +81,5 @@
> >    [deprecated]
> >    void setAndLoadFaviconForPage(in nsIURI aPageURI,
> >                                  in nsIURI aFaviconURI,
> >                                  in boolean aForceReload,
> > +                                in unsigned long aFaviconLoadType,
> 
> Any reason we chose not to make the LoadType argument optional (defaulting
> to NOT_PRIVATE if not passed)?  It makes for a long slew of changes to test
> code, etc, all of which set NOT_PRIVATE.   My plan for the B2G followup to
> this is to change the 'long' here to an object (a subset of nsILoadContext),
> and it will be even more tedious for all those callers to construct temp
> objects when they really all want the same default value {NOT_PRIVATE,
> appID=0, notInBrowser}.

I don't think that assuming the privacy state as a default argument is a good idea.  The callers _need_ to be aware that there are privacy implications to what they're doing, and therefore they need to be explicit about it.  This is true anywhere that PB needs to be considered.  It's true that we need to change all of the callers here (as I've done), but this is a one time cost, and it's the right thing to do.

Note that the requirements for the b2g appID and notInBrowser may differ here.  For those values, it might be OK to assume defaults.  I gave this some thought, and I think that it's actually wrong to put the PB bit in the same bucket as the b2g information, because they are conceptually different, and the only benefit that putting them in the same object would provide is some syntactic sugar, which would also make things awkward as you note.  So, please consider putting the b2g values in an object if needed, but let's keep the privacy bit separate.

Comment 32

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #30)
> Comment on attachment 658752 [details] [diff] [review]
> Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage
> 
> Review of attachment 658752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> bz or whomever: please correct me if I'm wrong, but it looks like we have
> two types of callers here:
> 
> 1) Contexts (tabbrowser, feedwriter, nsDocShell) which have a docShell
> handy, and could thus be passing in a LoadContext just as easily as the
> isPrivate arg here. (I've added comments on how to get them)
> 
> 2) Contexts that are always going to be in the parent process
> (BookmarkHTMLUtils, callers of PlacesUtils.restoreBookmarksFromJSONFile, the
> tests), and always want to pass NOT_PRIVATE, and so could work without
> change if we have the IDLs take an optional LoadContext that defaults to
> {NO_PRIVATE, appID=0, isInBrowerElement=false}.
> 
> If that's true can we just change these patches so that the IDLs take a
> nsILoadContext?  Seems trivial enough and would avoid IDL churn, as I'll
> have to write that patch anyway pretty much immediately for B2G v1.

Please see my comment above.  I've now shaped a strong opinion against bundling the privacy bit in the same object as the b2g info (even though it makes the API look bad).

Also, note that none of our various APIs for creating channels mandate the caller to pass an nsILoadContext, so at the end of the day a caller may just forget to do so, and we can't really have a sane assumption for a default PB bit value...

> If it's not true (i.e. there are callers here that always call with
> NOT_PRIVATE but may live in an app in the child process in B2G), than I need
> to know ASAP so I can try to figure out how B2G will get an appID for them.

I have not finished auditing all of the call sites, but yes, it is _possible_ that there are call sites in the child process which always pass NOT_PRIVATE.

> ::: browser/base/content/tabbrowser.xml
> @@ +676,5 @@
> >                this.mFaviconService.setAndFetchFaviconForPage(browser.currentURI,
> > +                                                             aURI, false,
> > +                                                             gPrivateBrowsingUI.privateWindow ?
> > +                                                               this.mFaviconService.FAVICON_LOAD_PRIVATE :
> > +                                                               this.mFaviconService.FAVICON_LOAD_NON_PRIVATE);
> 
> Could pass in browser.docShell here.

That's what the privateWindow attribute uses to retrieve the privacy bit.

> ::: browser/components/feeds/src/FeedWriter.js
> @@ +1331,5 @@
> > +    var usePrivateBrowsing = this._window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                         .getInterface(Ci.nsIWebNavigation)
> > +                                         .QueryInterface(Ci.nsIDocShell)
> > +                                         .QueryInterface(Ci.nsILoadContext)
> > +                                         .usePrivateBrowsing;
> 
> could pass in nsILoadContext here instead of its .usePrivateBrowsing

Are you suggesting that we should make the favicon retrieval API take both an nsILoadContext and a privacy bit?  That would not be great (as we do have callers which don't have a load context to pass).

> ::: docshell/base/nsDocShell.cpp
> @@ +8871,5 @@
> >              }
> >  
> >              // Inform the favicon service that the favicon for oldURI also
> >              // applies to aURI.
> > +            CopyFavicon(oldURI, aURI, mInPrivateBrowsing);
> 
> could pass 'this' since nsDocShell implements nsILoadContext.

Same here.

Comment 33

[Jason Duell]

> Are you suggesting that we should make the favicon retrieval API take both an 
> nsILoadContext and a privacy bit? 

Yes.  It's basically a requirement for necko B2G security, unless we can cook up another way to get the appID for these channels.

> it's actually wrong to put the PB bit in the same bucket as the b2g information, 
> because they are conceptually different

How so?  They are practically similar in that 1) we need to start adding them to all channels; 2) unlike channel callbacks they should not be allowed to be swapped out once they are set; and 3) we should have the info available to set them all at once in the same call site.  So I think bundling them together in some kind of "nsILoadContextInfo" IDL and having a nsIChannel.contextInfo member (instead of separate .isPrivate, .appId, .inBrowser fields) makes a lot of sense (especially since otherwise we'll have to dupe the horrible hackery in your necko patch 3x instead of once).  Does that make sense? 

> callers _need_ to be aware that there are privacy implications to what they're 
> doing, and therefore they need to be explicit about it.

I'm OK with passing everything explicitly.

Comment 34

[Jason Duell]

Comment on attachment 659031 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

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

So after some thought and searches for less gross alternatives, I'm with you on the basic semantics here:  we allow setting this info (whether it's just IsPrivate or a 'loadInfo' that combine it with appID/inBrowser) only if there's no loadGroup and channel callbacks, and we enforce that in all of the setPrivate/SetLoadGroup/setCallbacks functions, with a fatal error in debug builds.   It's really a very complex and ugly set of semantics, and I hope we can get rid of it soon, but for now I can't think of anything better.  Good job!

I'd like to see less code duplication and boilerplate here, though.   For starters, we can shove all the duplicated code that we use to check the conditions here into a base class that HTTP/FTP/Wyciwyg classes all inherit from.  And unless there's some need to call SetPrivate/LoadInfo on channel types that don't really support it (is there?  I don't see why), I think it's much cleaner to have a "nsILoadInfoChannel" class that inherits from nsIChannel and that we can QI.  (Or do you think it's better again to force all nsIChannels to code something for this, just so we're explicit about it?)

I'm attaching some IDLs and code sketches for this.

::: netwerk/base/public/nsIChannel.idl
@@ +297,5 @@
> +     * status cannot be obtained from the load group or load context (for
> +     * example, when the channel is not associated with any load group or load
> +     * context.  Setting this value directly should be avoided if possible.
> +     *
> +     * The implementations of nsIChannel are supposed to enforce the ordering

"Implementations of nsIChannel must enforce the..."

Comment 35

[Jason Duell]

Attachment: proposed IDLs and sketch of C++ helper base class.

Feel free to tell me if you think I'm barking up the wrong tree here.

Comment 36

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #33)
> > it's actually wrong to put the PB bit in the same bucket as the b2g information, 
> > because they are conceptually different
> 
> How so?  They are practically similar in that 1) we need to start adding
> them to all channels;

That is a non-goal for the privacy bit.  I'm only going to change those callers who _actually_ need to specify the PB bit, and don't do that already using a load group or load context.  The vast majority of the existing callers do not need to be modified here.

> 2) unlike channel callbacks they should not be allowed
> to be swapped out once they are set;

Well, in that they override whatever the callback/load group thinks, yes.

> and 3) we should have the info
> available to set them all at once in the same call site.

I don't know about the b2g bits, but as I said above, we don't really need to specify the PB bit in all call sites, so I don't think we actually need to set them all at once.

>  So I think
> bundling them together in some kind of "nsILoadContextInfo" IDL and having a
> nsIChannel.contextInfo member (instead of separate .isPrivate, .appId,
> .inBrowser fields) makes a lot of sense (especially since otherwise we'll
> have to dupe the horrible hackery in your necko patch 3x instead of once). 
> Does that make sense? 

Well, I think the only thing in common between these bits is the fact that they override whatever the load context/group says.  Also we need to know what the default behavior should be since it's possible that in some places we only want the PB bit set, or the b2g info set.

This is all basically what we talked about before, and there is also the concern that putting things in their own interface leads to ugliness for C++ callers.  And on top of that, I can't really write that patch effectively since I have no idea what the appId or inBrowser fields must be set to in any of these call sites.  So it seems like we're at an impasse here.  :(

Comment 37

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #34)
> So after some thought and searches for less gross alternatives, I'm with you
> on the basic semantics here:  we allow setting this info (whether it's just
> IsPrivate or a 'loadInfo' that combine it with appID/inBrowser) only if
> there's no loadGroup and channel callbacks, and we enforce that in all of
> the setPrivate/SetLoadGroup/setCallbacks functions, with a fatal error in
> debug builds.   It's really a very complex and ugly set of semantics, and I
> hope we can get rid of it soon, but for now I can't think of anything
> better.  Good job!

Yep, it's very ugly, but I think it's the only way to make things work with the existing nsIChannel APIs.

> I'd like to see less code duplication and boilerplate here, though.   For
> starters, we can shove all the duplicated code that we use to check the
> conditions here into a base class that HTTP/FTP/Wyciwyg classes all inherit
> from.

Hmm, I've already done that for the HTTP channel (using HttpBaseChannel), and as far as I can tell there is no common base for FTP or Wyciwyg channels (I even remember seeing a comment somewhere mentioning that the FTP channel classes should probably share a common base class.)  Or do you mean that I should do that refactoring (which might be a large project!) first?

> And unless there's some need to call SetPrivate/LoadInfo on channel
> types that don't really support it (is there?  I don't see why), I think
> it's much cleaner to have a "nsILoadInfoChannel" class that inherits from
> nsIChannel and that we can QI.  (Or do you think it's better again to force
> all nsIChannels to code something for this, just so we're explicit about it?)

I did things this way since I think it's sort of hard to decide if we won't ever need this for other channel types.  But I don't feel very strongly about this so I can change that if you want.

(I'll probably wait until we reach an agreement on what we want to do here first before making any changes, since last time it took me a couple of days to code up this patch in a way that works, and I don't wanna spend time changing it until we know exactly what the desired patch is.)

Comment 38

[(no longer active)]

Comment on attachment 659652 [details]
proposed IDLs and sketch of C++ helper base class.

(Note that my position that we should not do things this way still stands, but I'm providing feedback here in the interest of moving things forward.)

>// Only provide way to create these by setting all 3 values
>interface nsILoadInfo: nsISupports
>{
>  attribute unsigned long appId;
>  attribute boolean isInBrowserElement;
>  attribute boolean usePrivateBrowsing;
>};
>
>interface nsILoadContext : nsILoadInfo
>{
>  ...
>}
>
>interface nsILoadInfoChannel : nsIChannel
>{
>  // This value may only be set if the channel will not have a LoadGroup, and
>  // any callbacks that are set do not provide nsILoadInfo.  Otherwise the set
>  // functions will fail (in debug builds this is a fatal error).
>  nsILoadInfo loadInfo;
>};
>
>
>// C++ implementation: make nsHttpBaseChannel, FTP, Wyciwyg
>
>class LoadInfoChannel
>{
>  // Helper function for channel SetLoadGroup functions.
>  // Returns true if loadInfo has not been set. Else fatal error in debug
>  // builds, or returns false.
>  bool CanSetLoadGroup();
>
>  // Helper function for channel SetNotificationCallbacks functions.
>  // Returns true if loadInfo has not been set.  Else fatal error in debug
>  // builds, or returns false.
>  bool CanSetCallbacks();
>
>  // Returns NS_OK if loadInfo has not been set before, and no loadgroup has
>  // been set and callbacks are unset or do not implement nsILoadInfo.  Else
>  // fatal error in debug builds, or returns false.
>  nsresult SetLoadInfo(nsILoadInfo *loadInfo)
>
>private:
>  nsCOMPtr<nsILoadInfo> mLoadInfo;

Holding the reference to the object is not a good idea, since (at least in the case of PB) we don't want to hold on to a live object which can change what it thinks about its usePrivateBrowsing bit.  If we go down this route, we should just copy the information inside nsILoadInfo in the call to SetLoadInfo and store them on the object directly.  In other words, nsILoadInfo's purpose is to be syntax sugar (well, for JS at least, probably syntax vinegar for C++).  We should also document the fact that the nsILoadInfo object passed to SetLoadInfo will only be queried once and then discarded.  (And come to think of it, that would sort of suck, since I can't think of any other XPCOM interfaces with such semantics.)

>NS_QueryNotificationCallbacks():
>  // special-case if nsILoadInfo requested, and QI channel to nsILoadInfoChannel
>  // and return its loadInfo if non-null: otherwise same as normal

Why would we want NS_QueryNotificationCallbacks to ever return an nsILoadInfo?

>NS_UsePrivateBrowsing:  no changes needed...

Hmm, this would break if you don't have the appID information at the call site (for example, code in browser/ which attempts to load a favicon, where appId/inBrowserElement will be meaningless.

Comment 39

[Jason Duell]

ehsan and I achieved design synthesis here on IRC...

Comment 40

[(no longer active)]

(In reply to comment #39)
> ehsan and I achieved design synthesis here on IRC...

Jason is speaking truth here.  I was planning to prepare an alternate part 1 patch for this today but I ended up fighting updater fires...  I will hopefully finish up the patch and submit it for review tomorrow.

Comment 41

[Marco Bonardo [:mak]]

Comment on attachment 658726 [details] [diff] [review]
Part 2: Use nsIChannel::SetPrivate in the async favicon loader

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

looks good, some question:

::: toolkit/components/places/nsIFaviconService.idl
@@ +36,5 @@
>  
> +  // The favicon is being loaded from a private browsing window
> +  const unsigned long FAVICON_LOAD_PRIVATE = 1;
> +  // The favicon is being loaded from a non-private browsing window
> +  const unsigned long FAVICON_LOAD_NON_PRIVATE = 2;

why long? an uint16 would be more than enough here, unless there is some other reason (even if likely doesn't matter much nowadays)

I assume you didn't go for a bool to avoid having a misterious boolean passed into the function and force the caller to think about it (same reason you explained above)

Comment 42

[Marco Bonardo [:mak]]

Comment on attachment 658752 [details] [diff] [review]
Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

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

Provided tests keep passing I'm fine with the changes, though we seem to be passing NON_PRIVATE in tests that are actually testing old-style pb, and I wonder if we should care about this now or when we actually start implementing roackblocks for the new-style pb.

::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
@@ +78,5 @@
>    PlacesUtils.bookmarks.insertBookmark(
>                            PlacesUtils.unfiledBookmarksFolderId, pageURI,
>                            PlacesUtils.bookmarks.DEFAULT_INDEX, pageURI.spec);
> +  PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, FAVICON_URI, true,
> +                                                 PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE);

Some of the tests, like this one, are setting privatebrowsingEnabled to true, shouldn't we pass PRIVATE then?
I wonder if we should assert in case NON_PRIVATE is used while privateBrowsing is enabled, may that help finding these cases?

Comment 43

[Marco Bonardo [:mak]]

*roadblocks* too :)

Comment 44

[(no longer active)]

(In reply to comment #41)
> Comment on attachment 658726 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=658726
> Part 2: Use nsIChannel::SetPrivate in the async favicon loader
> 
> Review of attachment 658726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good, some question:
> 
> ::: toolkit/components/places/nsIFaviconService.idl
> @@ +36,5 @@
> >  
> > +  // The favicon is being loaded from a private browsing window
> > +  const unsigned long FAVICON_LOAD_PRIVATE = 1;
> > +  // The favicon is being loaded from a non-private browsing window
> > +  const unsigned long FAVICON_LOAD_NON_PRIVATE = 2;
> 
> why long? an uint16 would be more than enough here, unless there is some other
> reason (even if likely doesn't matter much nowadays)

Yes, the values will fit in a uint16_t.  I mostly prefer longs because they are generally faster!  :-)

If you feel strongly about this being 16-bits, please let me know and I'll change it.

> I assume you didn't go for a bool to avoid having a misterious boolean passed
> into the function and force the caller to think about it (same reason you
> explained above)

Precisely.

Comment 45

[(no longer active)]

(In reply to comment #42)
> Comment on attachment 658752 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=658752
> Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage
> 
> Review of attachment 658752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Provided tests keep passing I'm fine with the changes, though we seem to be
> passing NON_PRIVATE in tests that are actually testing old-style pb, and I
> wonder if we should care about this now or when we actually start implementing
> roackblocks for the new-style pb.
> 
> ::: toolkit/components/places/tests/favicons/test_setAndFetchFaviconForPage.js
> @@ +78,5 @@
> >    PlacesUtils.bookmarks.insertBookmark(
> >                            PlacesUtils.unfiledBookmarksFolderId, pageURI,
> >                            PlacesUtils.bookmarks.DEFAULT_INDEX, pageURI.spec);
> > +  PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, FAVICON_URI, true,
> > +                                                 PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE);
> 
> Some of the tests, like this one, are setting privatebrowsingEnabled to true,
> shouldn't we pass PRIVATE then?

Yes, that is a good point and I will make that change before relanding.

> I wonder if we should assert in case NON_PRIVATE is used while privateBrowsing
> is enabled, may that help finding these cases?

I don't really think we should add more code which relies on the global PB service at this time.

Comment 46

[Marco Bonardo [:mak]]

(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> Yes, the values will fit in a uint16_t.  I mostly prefer longs because they
> are generally faster!  :-)
> If you feel strongly about this being 16-bits, please let me know and I'll
> change it.

no strong feelings, as I said on nowadays system shouldn't matter much and is barely used, so whatever.

Comment 47

[Marco Bonardo [:mak]]

(In reply to Ehsan Akhgari [:ehsan] from comment #45)
> (In reply to comment #42)
> > Some of the tests, like this one, are setting privatebrowsingEnabled to true,
> > shouldn't we pass PRIVATE then?
> 
> Yes, that is a good point and I will make that change before relanding.

ok, btw note I pointed out one case, but likely there are more, the assert may even be a temporary solution to find them (add the asser, run tests, fix tests, remove the assert)

Comment 48

[(no longer active)]

(In reply to comment #47)
> (In reply to Ehsan Akhgari [:ehsan] from comment #45)
> > (In reply to comment #42)
> > > Some of the tests, like this one, are setting privatebrowsingEnabled to true,
> > > shouldn't we pass PRIVATE then?
> > 
> > Yes, that is a good point and I will make that change before relanding.
> 
> ok, btw note I pointed out one case, but likely there are more, the assert may
> even be a temporary solution to find them (add the asser, run tests, fix tests,
> remove the assert)

Sure, I can do that.

Comment 49

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

Comment 50

[(no longer active)]

Comment on attachment 660918 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

I should have a better patch soon.

Comment 51

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

Comment 52

[(no longer active)]

Attachment: Part 2: Use nsIChannel::SetPrivate in the async favicon loader

Comment 53

[(no longer active)]

Attachment: Part 3: Update the callers to nsIFaviconService::SetAndLoadFaviconForPage

Comment 54

[(no longer active)]

Attachment: Part 4: Update the callers to mozIAsyncFavicon::SetAndFetchFaviconForPage

Comment 55

[Jason Duell]

Comment on attachment 660952 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

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

OK, airport-style review, so I'm going to want to look at this one more time.  We're very close!

(I haven't had time to check in all channels, but make sure if we cache mIsPrivate in the channels we reset it if callbacks or loadgroup is set.)

::: docshell/base/SerializedLoadContext.cpp
@@ +22,5 @@
>    NS_QueryNotificationCallbacks(aChannel, loadContext);
>    Init(loadContext);
> +
> +  // Attempt to retrieve the private bit from the channel if it has been
> +  // overriden.

add to comment: "Note that it's OK to overload mIsPrivateBrowsing here, because it will never be set by both the LoadContext and nsIPrivateBrowsingChannel at the same time."

::: docshell/base/SerializedLoadContext.h
@@ +49,3 @@
>    // used to indicate if child-side LoadContext * was null.
>    bool          mIsNotNull;
> +  // used to indicate if child-side mUsePrivateBrowsing flag was valid.

"used to indicate if child-side mUsePrivateBrowsing flag is valid, even if mIsNotNull is false, i.e. child LoadContext was null."

::: netwerk/base/public/nsIPrivateBrowsingChannel.idl
@@ +27,5 @@
> +     * status cannot be obtained from the load group or load context (for
> +     * example, when the channel is not associated with any load group or load
> +     * context.  Setting this value directly should be avoided if possible.
> +     *
> +     * The implementations of nsIChannel are supposed to enforce the ordering

s/The implementations of nsIChannel/Implementation must enforce/  (Don't mention nsIChannel and "must" rather than supposed to)

@@ +29,5 @@
> +     * context.  Setting this value directly should be avoided if possible.
> +     *
> +     * The implementations of nsIChannel are supposed to enforce the ordering
> +     * semantics of this function by raising errors if setPrivate is called
> +     * on a channel which has a load group or a load context, and also to raise

"on a channel which already has a loadGroup and/or callbacks that implement nsILoadContext, or if the loadGroup or notification callbacks are set after setPrivate has been called."

As we talked on IRC this is a stricter version (callbacks can't be set now after SetPrivate even if they don't implement nsILoadContext).  If that barfs we can go back to the previous semantic.

@@ +42,5 @@
> +     * This function is used to determine whether the channel's private mode
> +     * has been overridden by a call to setPrivate.  It is intended to be used
> +     * by NS_UsePrivateBrowsing(), and you should not call it directly.
> +     *
> +     * @param aValue the overriden value.  This will only be set if the function

overridden.

::: netwerk/base/src/PrivateBrowsingChannel.h
@@ +29,5 @@
> +  {
> +      // Make sure that we don't have a load group or a load context
> +      // This is a fatal error in debug builds, and a runtime error in release
> +      // builds.
> +      nsILoadGroup* loadGroup = static_cast<Channel*>(this)->LoadGroup();

So you added a LoadGroup() function to all channels for this. I'd prefer we don't add that just for this if we can avoid it.  I suggest we just make this class be a friend of the various channels so it can access mLoadGroup directly (constructor could take a "T *" that it holds onto to do "mThis->mLoadGroup": lifetime is guaranteed to overlap w/mThis, of course).

Or we could use nsIRequest::GetLoadGroup.  But it's less efficient, and I'm thinking of the PrivateBrowsingChannel as being really part of the the classes' internal implmentations.

::: netwerk/protocol/ftp/nsFTPChannel.cpp
@@ +241,5 @@
> +  // builds.
> +  MOZ_ASSERT(!mPrivateBrowsingOverriden);
> +  if (mPrivateBrowsingOverriden) {
> +    return NS_ERROR_FAILURE;
> +  }

So instead of repeating this boilerplate over and over, let's implement this as "CanSetCallbacks" (and CanSetLoadGroup), put the MOZ_ASSERT in them, and so in the channels themselves we just have

if (!CanSetCallbacks())
   return NS_ERROR_FAILURE()

Comment 56

[Jason Duell]

Attachment: C++ sample code that accesses 'mLoadGroup' from base template class

ehsan: this is the style for accessing mLoadGroup I'm thinking of, in case my last comment wasn't articulate :)

Comment 57

[(no longer active)]

Comment on attachment 660952 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

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

(About checking all of the channels, I have done that, but please double check to make sure we're not merely trusting my sanity.  :-)

::: netwerk/base/src/PrivateBrowsingChannel.h
@@ +29,5 @@
> +  {
> +      // Make sure that we don't have a load group or a load context
> +      // This is a fatal error in debug builds, and a runtime error in release
> +      // builds.
> +      nsILoadGroup* loadGroup = static_cast<Channel*>(this)->LoadGroup();

Hmm, I did that because I really hate touching other classes' private parts, but I'll change this.  :-)

::: netwerk/protocol/ftp/nsFTPChannel.cpp
@@ +241,5 @@
> +  // builds.
> +  MOZ_ASSERT(!mPrivateBrowsingOverriden);
> +  if (mPrivateBrowsingOverriden) {
> +    return NS_ERROR_FAILURE;
> +  }

OK.  I thought about doing that as we talked but decided not to since it would only save one line!  But I'll do that.

Comment 58

[(no longer active)]

(In reply to Jason Duell (:jduell) from comment #56)
> Created attachment 661009 [details]
> C++ sample code that accesses 'mLoadGroup' from base template class
> 
> ehsan: this is the style for accessing mLoadGroup I'm thinking of, in case
> my last comment wasn't articulate :)

No need to keep the this pointer, we already have it! (static_cast<T*>(this))

Comment 59

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

Comment 60

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=5296611c476c

Comment 61

[Jason Duell]

Alas, looks from the try build like some tests are unhappy, but not with useful errors.

Comment 62

[(no longer active)]

Attachment: Part 2: Use nsIChannel::SetPrivate in the async favicon loader

I had a mistake that required a channel to implement nsIPrivateBrowsingChannel.  This patch fixes that.

Comment 63

[(no longer active)]

(In reply to comment #61)
> Alas, looks from the try build like some tests are unhappy, but not with useful
> errors.

The new part 2 version fixes those.

Comment 64

[(no longer active)]

Attachment: Part 1: Add the nsIChannel APIs

Updated the test to the new API

Comment 65

[Jason Duell]

Comment on attachment 661375 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

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

+r with a couple last nits addressed (no need to re-review if you agree to make the nits and this passes try).

Thanks again Ehsan!   This horrible patch is now slightly less horrible, which is good :)

::: docshell/base/SerializedLoadContext.cpp
@@ +25,5 @@
> +  // Attempt to retrieve the private bit from the channel if it has been
> +  // overriden.
> +  // Note that it's OK to overload mIsPrivateBrowsing here, because it will
> +  // never be set by both the LoadContext and nsIPrivateBrowsingChannel at
> +  // the same time.

Let's ditch my "note that it's OK" comment and just wrap the logic below that checks for PB override inside "if (!loadContext)".  Always better to enforce assumptions in code than in comment, and mirrors what you in the parent classes.

::: netwerk/base/public/nsIPrivateBrowsingChannel.idl
@@ +25,5 @@
> +     *
> +     * Note that this value is only meant to be used when the channel's privacy
> +     * status cannot be obtained from the load group or load context (for
> +     * example, when the channel is not associated with any load group or load
> +     * context.  Setting this value directly should be avoided if possible.

forgot to close parens in comment.

@@ +29,5 @@
> +     * context.  Setting this value directly should be avoided if possible.
> +     *
> +     * Implementations must enforce the ordering semantics of this function by
> +     * raising errors if setPrivate is called on a channel which has a load
> +     * group and/or callbacks that implement nsILoadContext, or if the loadGroup

s/load group/loadGroup/ so we're consistent and use name of nsIRequest member.

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +559,5 @@
> +
> +NS_IMETHODIMP
> +FTPChannelChild::SetLoadGroup(nsILoadGroup * aLoadGroup)
> +{
> +  if (!CanSetCallbacks()) {

It looks like a typo to be calling "CanSetCallBacks" from SetLoadGroup().  Let's create a "CanSetLoadGroup()" and just dupe the two lines of code (or have it call CanSetCallbacks with a comment noting that conditions are the same).  I was originally thinking of this when then two functions were going to have different implementations.

Comment 66

[Jason Duell]

Comment on attachment 661375 [details] [diff] [review]
Part 1: Add the nsIChannel APIs

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

BTW I checked that the patch corrects all cases where if we cache mPrivateBrowsing settings in either SetLoadGroup/SetCallbacks we also update it in the other one too.  Looks like we were missing one in wyciwyg--thanks for finding.

Comment 67

[(no longer active)]

Addressed the comments and pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/83a7ea5398a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc194e18570
https://hg.mozilla.org/integration/mozilla-inbound/rev/7058ce88d14f
https://hg.mozilla.org/integration/mozilla-inbound/rev/89027f6e7662

Comment 68

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/83a7ea5398a9
https://hg.mozilla.org/mozilla-central/rev/4dc194e18570
https://hg.mozilla.org/mozilla-central/rev/7058ce88d14f
https://hg.mozilla.org/mozilla-central/rev/89027f6e7662

Comment 69

[Sonthakit]

I am a developer of bookmarkfaviconchanger and I cannot accept this bug fix.
You change 
setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback);
into
setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, aFaviconLoadType, [optional] aCallback);

This will make every add-on that use this function crash.
If add-on not specified 4 argument, it will return NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments.
And if add-on use aCallback function as argument 4, it will not callback anymore because argument 4 turn into aFaviconLoadType

More... This will make programmer in difficult situation because every time you try to use this function, you must write

if (Firefox version < 18) {
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback);
} else {
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, aFaviconLoadType, [optional] aCallback);
}

It is very inconvenience. And I can expected that if FF 18 ship off with this feature. A lot of non-working add-on and crash will report.

What you should do is make aFaviconLoadType is a 5th and optional argument
   setAndLoadFaviconForPage(aPageURI, aFaviconURI, aForceReload, [optional] aCallback, [optional] aFaviconLoadType);

You can make default value of aFaviconLoadType in different ways
1) default is always true
2) default is always false
3) default can be true of false depend on situation (system selected)

That its!!

Comment 70

[Josh Matthews [:jdm]]

It's a required argument to ensure addons do not unintentionally leak private information from private browsing sessions. The addon compatibility checker should be able to catch addons that require updating, and until then it is up to users and developers to discover addons that require fixing.

Comment 71

[Sonthakit]

Do you forget something very important.
Add-on developer is human, he can sick and die.
No guarantee that every add-on developer will fix it.
Why not do anything that make old add-on compatible both for FF17 and FF18?

I suggest this...
If this parameter is require, you still can make it the 5th argument and  then make it optional.
And if the caller do not specified it then make it "true" if it is in private browsing.
And make it "False" if it is not private browsing mode.

Should be it automatic use secure way to get favicon when on private browsing?
Answer is simple.  YES... YES... YES...

At this time, I should tell every one something.

NEVER CHANGE FUNCTION LIKE THIS AGAIN !!!

Every OS such as window, macOS, Linux have never change function that make old software bug.
They try to make old software stable as much as possible.
If they have new idea, they make new function.
They will never "just change function" and ignore the bug that will occur.

If you want this parameter badly, the make your own new function "setAndLoadFaviconForPage2"

Comment 72

[Marco Bonardo [:mak]]

Honestly, the fact you noticed the API changed, means the fix is right, we indeed want devs to be well aware of incoming incompatibilities and provide fixes, when they matter (We try to use optional arguments everytime is possible and doesn't require explicit choices by the developer).  You were also using a deprecate API there, that may go away at any time, so looks like deprecation is something that gets unnoticed, we should also figure how to fix that problem.

We don't forget most devs are volunteers, we all were, and still are in our spare time, often dedicating well more than the work time to the project.  Unfortunately the nature itself of APIs and the quick pace the Web is evolving sometimes causes these kind of incompatibilities, but we can't be hostage of APIs, reason why the "frozen" APIs concept has been lifted in gecko2.0.

Comment 73

[Boris Zbarsky [:bzbarsky]]

> Every OS such as window, macOS, Linux have never change function that make old software
> bug.

That's flat out false for MacOS and Linux, actually.  They change functions sometimes, and remove them completely often.

I realize that this is annoying to deal with as an API client.  Our other option here, I think, was to completely disable the old method and come up with a new one, which is what MacOS and Linux would typically do...

Comment 74

[Boris Zbarsky [:bzbarsky]]

Oh, one more thing:

> And make it "False" if it is not private browsing mode.

The whole point is that private browsing mode is stopping being a global mode and becoming a per-document mode.  Which is why the global state can't be used for this function: it will no longer exist.  That was the whole point of this bug.

Comment 75

[Sonthakit]

Then make the default use "private browsing"
Old add-on that use this function don't care how to get favicon. As long as it set favicon, it is OK.
No matter it is global or not. Using default "private" for the current add-on do not cause problem.

And you're going to say private browsing is per window.
You forget something?

My code call this function in many ways. I call in module environment (no window). It call in bookmark sidebar. I call in bookmark manager. It call in my add-on XUL. And most difficult is... I call it after "quit-application-grant" to clear favicon if user uninstall my add-on. You see?

>The whole point is that private browsing mode is stopping being a global mode and becoming a per-document mode.  Which is why the global state can't be used for this function: it will no longer exist.  That was the whole point of this bug.

Are you joking? Many Add-on can request http in module environment (no document associate). Add-on can make internet connection in it's own XUL wndows. Add-on can make internet connection on timer interval without any document associate like email checker. How can it know that it should private or not if it is not global. If you do this, more add-on will fail or unsecure. More problem will come.

Let's see, now you make every add-on that use setAndLoadFaviconForPage fail.
If you change every function that require internet connection to the same as setAndLoadFaviconForPage, nearly all add-on will fail.

If you really really want to do this.
MAKE NEW FUNCTION - don't destroy the old one.
When compatibility check - tell developer that "WARNING - THIS FUNCTION WILL REMOVE IN FUTURE, PLEASE CHANGE TO THE NEW FUNCTION".
Give him some time, like 3-6 month before remove the old function.

Comment 76

[(no longer active)]

Sonthakit, can you please comment on bug 816003 and not on this one?  No matter what we decide to do there, there will be no further activity on this bug, so commenting here will not really be useful.