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.

Where does the code which create this channel live?

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.

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.

(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?

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.

(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!

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

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. :(

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?

(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.

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.

I added the tests to the wrong patch.

Fixed a number of build errors.

Fixed a number of test failures.

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/eakhgari@mozilla.com-012e30bbecc8

Try is fairly green on my latest push.

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 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.

(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.

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 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 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 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.

(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.

(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.

> 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 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..."

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

(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. :(

(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 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.

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

(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 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 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?

*roadblocks* too :)

(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.

(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.

(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.

(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)

(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 on attachment 660918 [details] [diff] [review] Part 1: Add the nsIChannel APIs I should have a better patch soon.

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()

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

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.

(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))

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

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

(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.

Updated the test to the new API

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 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.

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

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

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!!

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.

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"

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.

> 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...

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.

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.

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.