788275 - Tab preview should respect the private browsing mode when attempting to load a favicon

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Josh Matthews [:jdm]]

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

Comment 1

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm#76 (_imageFromURI) for reference.

Comment 2

[(no longer active)]

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.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 659029 [details] [diff] [review]
Patch (v1)

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

Looks ok to me.

Comment 5

[(no longer active)]

Updated to use the new API and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6571e0aef937

Comment 6

[(no longer active)]

Backed out because of test failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/427ca4f66d83

Comment 7

[(no longer active)]

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.

Comment 8

[Jason Duell]

I think #2 is better too.  Make sure to update comments to reflect the change in semantics.

Comment 9

[(no longer active)]

Attachment: Part 1

Comment 10

[(no longer active)]

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.

Comment 11

[(no longer active)]

Attachment: Part 0

Comment 12

[(no longer active)]

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

Comment 13

[(no longer active)]

Attachment: Part 2

Comment 14

[(no longer active)]

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

Comment 15

[(no longer active)]

(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

Comment 16

[Mozilla RelEng Bot]

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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e8f96ff375ec

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[(no longer active)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[(no longer active)]

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

Comment 21

[(no longer active)]

Attachment: Part 0

Clearly my sanity should not be over-trusted!  This version of the patch gets rid of all of those casts.

Comment 22

[(no longer active)]

Landed part 0:

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c78f239c47

Comment 23

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/64c78f239c47

Comment 24

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cda2cc45466
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3acad5cf39

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cda2cc45466
https://hg.mozilla.org/mozilla-central/rev/7c3acad5cf39