Closed Bug 788275 Opened 7 years ago Closed 7 years ago
Tab preview should respect the private browsing mode when attempting to load a favicon
Besides being generally inefficient compared to the favicon cache we have, this interacts badly with things like private browsing (the favicons will be cached in the disk cached even for private windows, which is an information leak).
http://mxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm#76 (_imageFromURI) for reference.
While it would be a good thing to make this use the async favicon loading API, that is outside my scope of interest. I just need the correct private flag to be passed to the network channel (and that patch would be half of fixing the original bug, the other half being delivering the page URI to the call site), so I'll write the PB specific patch instead.
Summary: Tab preview should go through the Places favicon service instead of creating a channel by hand → Tab preview should respect the private browsing mode when attempting to load a favicon
Comment on attachment 659029 [details] [diff] [review] Patch (v1) Review of attachment 659029 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok to me.
Attachment #659029 - Flags: review?(jmathies) → review+
Updated to use the new API and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6571e0aef937
Target Milestone: --- → mozilla18
Backed out because of test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/427ca4f66d83
Hmm, I don't know why I did not come across this before. NetUtils.asyncFetch sets the notification callbacks before calling asyncOpen <http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#160>, which is unfortunate since that does not give us an obvious point at which we can call setPrivate. Here are the solutions I can think of: 1. Modify asyncFetch to add a third optional parameter to it which tells it to call setPrivate before calling asyncOpen (and after setting the notification callbacks on the socket). 2. Modify CanSetCallbacks and CanSetLoadGroup to make them accept pointers to the arguments of their callers, and do the dance of ensuring that the set callback/loadgroup does not provide an nsILoadContext* if asked for. Jason, which one of these solutions do you prefer? I slightly favor the second solution, but don't have a strong opinion either way.
I think #2 is better too. Make sure to update comments to reflect the change in semantics.
This patch hits this nasty bug where the ctor of nsCORSListenerProxy passes |this| to the SetNotificationCallbacks method, which now does an AddRef/Release on the passed in argument, which causes the object to get destroyed immediately after it was created. I'll fix that by adding an Init methot to the class.
Attachment #662152 - Attachment description: Part 3 → Part 2
I ended up pushing the old version of part 2 to try. Here's a correct push: https://tbpl.mozilla.org/?tree=Try&rev=e8f96ff375ec
(In reply to Ehsan Akhgari [:ehsan] from comment #14) > I ended up pushing the old version of part 2 to try. Here's a correct push: > https://tbpl.mozilla.org/?tree=Try&rev=e8f96ff375ec And, I ended up pushing that on top of bustage :( https://tbpl.mozilla.org/?tree=Try&rev=1a7c97a404e1
Try run for e8f96ff375ec is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=e8f96ff375ec Results (out of 2 total builds): exception: 2 Builds (or logs if builds failed) available at: http://firstname.lastname@example.org
Comment on attachment 661965 [details] [diff] [review] Part 0 >@@ -1086,22 +1054,23 @@ NS_StartCORSPreflight(nsIChannel* aReque >+ preflightListener = static_cast<nsIStreamListener*>(corsListener); You shouldn't need the cast here... Why is it needed? >+++ b/content/base/src/nsScriptLoader.cpp >+ listener = static_cast<nsIStreamListener*>(corsListener); Or here. And elsewhere in this patch. r=me
Attachment #661965 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #17) > Comment on attachment 661965 [details] [diff] [review] > Part 0 > > >@@ -1086,22 +1054,23 @@ NS_StartCORSPreflight(nsIChannel* aReque > >+ preflightListener = static_cast<nsIStreamListener*>(corsListener); > > You shouldn't need the cast here... Why is it needed? Because otherwise nsCOMPtr would try to pick an nsISupports* out of the thing you assign to it, and that conversion is ambiguous and it fails. The only way to avoid these casts is to QI I think which would be worse. Are you fine with leaving the casts in?
Most of those are not nsCOMPtr<nsISupports>, so why are they trying to get an nsISupports out? If it's needed to compile, I can live with it; just trying to understand why it's needed.
(In reply to comment #19) > Most of those are not nsCOMPtr<nsISupports>, so why are they trying to get an > nsISupports out? > > If it's needed to compile, I can live with it; just trying to understand why > it's needed. Actually, this made me curious. I was under the impression that nsCOMPtr has an operator=(nsISupports*) which makes this break. So I wondered. And that method does not exist. So I tried taking out all of the casts which I'm pretty sure I added yesterday to beat this thing to compile, and things seem to compile without the casts! I'm doing a full rebuild to check my sanity, and if that proves me to be insane, then I will attach a new patch without any of the casts!
Clearly my sanity should not be over-trusted! This version of the patch gets rid of all of those casts.
Attachment #661965 - Attachment is obsolete: true
Whiteboard: [leave open]
Whiteboard: [leave open]
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.