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

RESOLVED FIXED in Firefox 18

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

unspecified
mozilla18
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18+ fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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).
(Reporter)

Comment 1

5 years ago
http://mxr.mozilla.org/mozilla-central/source/browser/modules/WindowsPreviewPerTab.jsm#76 (_imageFromURI) for reference.
Assignee: nobody → ehsan
Blocks: 787743
tracking-firefox18: --- → +
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
Created attachment 659029 [details] [diff] [review]
Patch (v1)
Attachment #659029 - Flags: review?(josh)
(Reporter)

Comment 4

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

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

Looks ok to me.
Attachment #659029 - Flags: review?(josh)
Attachment #659029 - Flags: review?(jmathies)
Attachment #659029 - Flags: feedback+

Updated

5 years ago
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.
Created attachment 661948 [details] [diff] [review]
Part 1
Attachment #661948 - Flags: review?(jduell.mcbugs)
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.
Created attachment 661965 [details] [diff] [review]
Part 0
Attachment #661965 - Flags: review?(bzbarsky)
https://tbpl.mozilla.org/?tree=Try&rev=3fd800a23941
Created attachment 662152 [details] [diff] [review]
Part 2
Attachment #659029 - Attachment is obsolete: true
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

Comment 16

5 years ago
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/eakhgari@mozilla.com-e8f96ff375ec
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!
Created attachment 662289 [details] [diff] [review]
Part 0

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
Landed part 0:

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c78f239c47
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/64c78f239c47
status-firefox18: --- → fixed
Attachment #661948 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cda2cc45466
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3acad5cf39
Whiteboard: [leave open]
(Reporter)

Updated

5 years ago
Blocks: 794502
(Reporter)

Updated

5 years ago
No longer blocks: 794502
https://hg.mozilla.org/mozilla-central/rev/4cda2cc45466
https://hg.mozilla.org/mozilla-central/rev/7c3acad5cf39
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Depends on: 795892
You need to log in before you can comment on or make changes to this bug.