1283067 - Favicon request doesn't timeout or close when related window is closed (1255270 is not fixed yet) on Windows due to WindowsPreviewPreTab.jsm

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1279208 +++

+++ This bug was initially created as a clone of Bug #1255270 +++

While the fix from bug 1279208 fixed the places consumer of the favicon, it seems WindowsPreviewPerTab.jsm does *another* request for the favicon, even though it's disabled by default.

Felipe, which of these are options and how hard are they:

- stop it doing all its work when it's disabled.
- reuse the places favicon code
- reuse the tabs (to which we seem to keep references anyway) to get favicons from there, potentially only firing the onLinkIconAvailable notification when the favicon's <image> actually loads and preloading the default favicon's data for everything else

- something else?

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #0)
> +++ This bug was initially created as a clone of Bug #1279208 +++
> 
> +++ This bug was initially created as a clone of Bug #1255270 +++
> 
> While the fix from bug 1279208 fixed the places consumer of the favicon, it
> seems WindowsPreviewPerTab.jsm does *another* request for the favicon, even
> though it's disabled by default.
> 
> Felipe, which of these are options and how hard are they:
> 
> - stop it doing all its work when it's disabled.
> - reuse the places favicon code
> - reuse the tabs (to which we seem to keep references anyway) to get
> favicons from there, potentially only firing the onLinkIconAvailable
> notification when the favicon's <image> actually loads and preloading the
> default favicon's data for everything else
> 
> - something else?

Comment 2

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #28)
> Which ironically is turned off by default because it has
> issues, but apparently still totally does network requests for this stuff
> (even when disabled)

That's horrible, sounds like a Tp perf hit too. Why do we retain such functionality when it's clearly not what we want and we clearly don't maintain it?
Shouldn't the scale pend towards "dead" in these cases?

Comment 3

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #28)
> > Which ironically is turned off by default because it has
> > issues, but apparently still totally does network requests for this stuff
> > (even when disabled)
> 
> That's horrible, sounds like a Tp perf hit too. Why do we retain such
> functionality when it's clearly not what we want and we clearly don't
> maintain it?
> Shouldn't the scale pend towards "dead" in these cases?

I would not be opposed to killing it, but it's a UI-exposed pref (bottom of the 'general' pane on windows). Just like with panorama, there'll be a vocal minority who will be... vocal. I'd prefer not to make this sec bug depend on having a discussion / determination about killing it, just fix it the simplest way we can.

Comment 4

[Marco Bonardo [:mak]]

You are right, I didn't recall it was exposed in the prefs pane. Telemetry could help understanding how much used it is.
To me looks like it's something that an add-on should implement, and then that vocal minority could just opt into it.

Sorry for the OT, we clearly also need an interim fix.

Comment 5

[:Gijs (he/him)]

Attachment: Preliminary Patch

I think this works. I'm effectively piggybacking this on top of the favicon service stuff. Marco/Felipe, thoughts?

I think we should have a separate non-security bug about the fact that all this code runs even when the feature is disabled, but I didn't want to attempt to fix that here as it wouldn't address the fundamental security issue.

Comment 6

[Marco Bonardo [:mak]]

what happens in permanent pb mode or in general when the favicons service doesn't have an icon? setAndFetchFaviconForPage doesn't  callback in that case (bug 740457), and we'll get no icon, not even the default one?
Fwiw, we could fetch the icon from moz-anno:favicon:iconurl and that would basically return the icon only if the service has it, or the default icon. Since the page is loaded in a tab it's likely we already have the icon, so there should be no need to roundtrip to try to reload it.
Though, that wouldn't solve the permanent PB mode problem.

Comment 7

[Marco Bonardo [:mak]]

I wonder if we should make the favicons service store icons in a memory store when we are in pb mode, so we can still serve them through moz-anno:favicon: urls, that may solve a lot of headaches around.
Though it may take some time to do that, also cause I'm in the middle of rewriting the favicons store in bug 977177 and I can't do that until I'm done with the refactoring.

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #6)
> Fwiw, we could fetch the icon from moz-anno:favicon:iconurl and that would
> basically return the icon only if the service has it, or the default icon.
> Since the page is loaded in a tab it's likely we already have the icon,

Wellll... this code runs at the same time as when we set the image attribute on the tab (same turn of the event loop, anyway). So it seems like it's unlikely that the image is already there... am I missing the point?

> Though, that wouldn't solve the permanent PB mode problem.

Why, wouldn't it show the default icon in that case?

I'm somewhat tempted to just get the default icon once when the module is initiated so we have it cached, and then assign it to every instance of the preview class. If it just never returns an icon in PB, then I'm pretty OK with breaking that and always showing the default icon, if that's the price we pay for doing the right thing security-wise. Given the full preview of the page when loaded, it seems to me the icon is much less important anyway.

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Marco Bonardo [::mak] from comment #6)
> > Fwiw, we could fetch the icon from moz-anno:favicon:iconurl and that would
> > basically return the icon only if the service has it, or the default icon.
> > Since the page is loaded in a tab it's likely we already have the icon,
> 
> Wellll... this code runs at the same time as when we set the image attribute
> on the tab (same turn of the event loop, anyway). So it seems like it's
> unlikely that the image is already there... am I missing the point?

well, if it runs at the same time, yes it's likely it won't be set, but honestly:
1. we could delay the icon fetch here by 1s, without big trouble. It's very unlikely that the user opens a tab and at the same exact time opens AeroPeek previews.
2. in the worst case it would be only one tab out of N, the current loading one.
3. the user may have visited the page earlier, and then we'd already have an icon.
Not a big deal, imo.

> > Though, that wouldn't solve the permanent PB mode problem.
> 
> Why, wouldn't it show the default icon in that case?

Yes, but all tabs would have the default icon... not a great experience.

> I'm somewhat tempted to just get the default icon once when the module is
> initiated so we have it cached, and then assign it to every instance of the
> preview class. If it just never returns an icon in PB, then I'm pretty OK
> with breaking that and always showing the default icon, if that's the price
> we pay for doing the right thing security-wise. Given the full preview of
> the page when loaded, it seems to me the icon is much less important anyway.

The fact is, you are re-doing what the favicons service is already doing. That's why I suggested to rather fetch contents from a moz-anno:favicon: (using NetUtils.asyncFetch I guess). It would have the same "defects" as your current patch, but would be simpler.
By "defects" I mean the fact every tab will get the default icon in PB mode.

I don't know if that's acceptable, if it's not acceptable, then also your current patch won't do.

Comment 10

[Marco Bonardo [:mak]]

also, your patch doesn't seem to solve the fact this module fetches things even if it's disabled. Is that impossible to do, or just stuff for a follow-up bug to be filed?

Comment 11

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #10)
> also, your patch doesn't seem to solve the fact this module fetches things
> even if it's disabled. Is that impossible to do, or just stuff for a
> follow-up bug to be filed?

(In reply to :Gijs Kruitbosch from comment #5)
> I think we should have a separate non-security bug about the fact that all
> this code runs even when the feature is disabled, but I didn't want to
> attempt to fix that here as it wouldn't address the fundamental security
> issue.

:-)

Comment 12

[Marco Bonardo [:mak]]

I assume the review request needs an updated patch at this point?

Comment 13

[:Gijs (he/him)]

Comment on attachment 8766570 [details] [diff] [review]
Preliminary Patch

Yes. Trickier than I thought. :-(

Comment 14

[:Gijs (he/him)]

Attachment: Patch v0.1

This should be better, and is smaller + simpler. Marco, let me know what you think.

This intentionally accepts that we will show a default favicon in the taskbar tab previews in private browsing.

Orthogonally, I also noticed that SVG favicons don't show up correctly (predates this patch).

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8767779 [details] [diff] [review]
Patch v0.1

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

It looks better, I still have some questions but it's mostly done.

::: browser/components/places/PlacesUIUtils.jsm
@@ +153,5 @@
>     */
>    _makeCompletionCallback(win, id) {
>      return {
>        onComplete(uri) {
> +        Services.obs.notifyObservers(uri, "places-favicon-loaded", null);

honestly, we already have a notification, you could register an history observer with PlacesUtils.history.addObserver (I'd suggest to use a weak reference in this case) and then you can use onPageChanged (with ATTRIBUTE_FAVICON) that will give you both the page uri and the icon uri.

That said, if there's a specific reason to introduce an nsIObserver topic, we could do that, but it should be more specific or people could start using this thinking it will fire for any favicon change, while it doesn't (maybe favicon-stored-on-pageload or something similar?)

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +53,2 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

if you planned to make these alphabetically ordered, something is wrong yet :)
Btw, off-hand I'd defineLazyMobileGetter PageThumbs

@@ +71,5 @@
>                                     "@mozilla.org/browser/favicon-service;1",
>                                     "nsIFaviconService");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUIUtils",
> +                                  "resource:///modules/PlacesUIUtils.jsm");

unused?

@@ +112,2 @@
>    else
> +    _imageFromURI(faviconSvc.defaultFavicon, privateMode, callback);

while here, would you mind bracing this if/else?

@@ +626,5 @@
> +    } catch (ex) {}
> +
> +    let requestURL = shouldRequestFaviconURL ?
> +      "moz-anno:favicon:" + aIconURL :
> +      aIconURL;

fwiw, I just landed "page-icon:pageurl" protocol, though that won't be uplift-able.

So moz-anno:favicon: doesn't pass-through those protocols? sounds like a bug to me, we should probably file it and annotate the bug # in a TODO comment here.

@@ +699,5 @@
>  
>      this.prefs.removeObserver(TOGGLE_PREF_NAME, this);
>      this.prefs.removeObserver(DISABLE_THRESHOLD_PREF_NAME, this);
>      this.prefs.removeObserver(CACHE_EXPIRATION_TIME_PREF_NAME, this);
> +    Services.obs.removeObserver(this, "places-favicon-loaded", false);

nit: if you add weakRef support, you can likely remove all of these and just bail out from Observe if (!this._enabled)

@@ +788,5 @@
> +        aSubject.QueryInterface(Ci.nsIURI);
> +        let uriSpec = aSubject.spec;
> +        for (let win of this.windows) {
> +          for (let [tab, preview] of win.previews) {
> +            if (tab.getAttribute("image") == aSubject.spec) {

what's the point of passing an nsIURI if then you always want the spec, wouldn't be simpler to pass the spec as aData?

Comment 16

[:Gijs (he/him)]

Attachment: Patch v0.2

Comment 17

[:Gijs (he/him)]

Attachment: Patch v0.3

D'oh, forgot to remove the notifyObservers call.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8768213 [details] [diff] [review]
Patch v0.3

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

::: browser/modules/WindowsPreviewPerTab.jsm
@@ +68,5 @@
>                                     "@mozilla.org/image/tools;1",
>                                     "imgITools");
>  XPCOMUtils.defineLazyServiceGetter(this, "faviconSvc",
>                                     "@mozilla.org/browser/favicon-service;1",
>                                     "nsIFaviconService");

nit: this is pointless, could just use PlacesUtils.favicons everywhere now.

@@ +610,5 @@
>      }
>    },
>  
> +  directRequestProtocols: new Set([
> +    "file", "chrome", "resource", "about"

please rememeber to file the bug about the misbehavior of moz-anno:favicon with these.

@@ +683,5 @@
>        return;
>  
>      this.prefs.addObserver(TOGGLE_PREF_NAME, this, false);
>      this.prefs.addObserver(DISABLE_THRESHOLD_PREF_NAME, this, false);
>      this.prefs.addObserver(CACHE_EXPIRATION_TIME_PREF_NAME, this, false);

nit: As I said, I'd also change these to weakref, remove the removeObserver calls from destroy, and bailout from observe() if (!this._enabled). But I understand if you don't want to do further changes.

Comment 19

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #18)
> @@ +683,5 @@
> >        return;
> >  
> >      this.prefs.addObserver(TOGGLE_PREF_NAME, this, false);
> >      this.prefs.addObserver(DISABLE_THRESHOLD_PREF_NAME, this, false);
> >      this.prefs.addObserver(CACHE_EXPIRATION_TIME_PREF_NAME, this, false);
> 
> nit: As I said, I'd also change these to weakref, remove the removeObserver
> calls from destroy, and bailout from observe() if (!this._enabled). But I
> understand if you don't want to do further changes.

I assumed you only meant this for the new observer.

I looked at doing this, but unfortunately the pref observer change would be more involved, because the pref observer is also used to enable/disable the module, and so bailing out in observe() based on this._enabled would mean you could never enable the module by flipping the pref, because we'd bail out from the pref observer. This is fixable, it's just not trivial and I'd like not to potentially introduce other bugs by trying to take care of the whole thing here, as it's unrelated to this fix.

Comment 20

[:Gijs (he/him)]

Attachment: Patch

Addressed nit about favicon service reference and fixes a (pre-existing, but perhaps less likely before these changes) race, carrying over / r+ over IRC, respectively.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Eh, at this stage all the other patches have landed, so it probably won't be hard to put 2 and 2 together. I don't really see that we have other choices than "land everything ASAP all the way to 48 and 45 esr".

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not explicitly. I'm just fetching the favicon using a different method, which hopefully adds some sand/salt.

Which older supported branches are affected by this flaw?
Everything.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Shouldn't be too difficult, though I don't expect it to apply seemlessly all the way to ESR.

How likely is this patch to cause regressions; how much testing does it need?
There's a known regression wrt tabs in private browsing for which favicons are not available that Marco and I are OK with taking for this feature which is disabled by default and which will be fixable separately. This is the Simplest Thing That Could Possibly Work considering uplift etc. We should probably do some testing, though I'm not sure how much this feature was ever tested, and so I'm not sure to what degree testing will really help find new (rather than existing but unnoticed) issues.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

Land everything ASAP all the way to 48 and ESR45.

:-)

Please nominate patches for Aurora, Beta, and ESR45.

Comment 22

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/b005ddc33470

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/integration/fx-team/rev/1dd03896ee5f

because I removed a parameter from getFaviconAsImage and so I should have updated all the callsites. :-\

Comment 24

[:Gijs (he/him)]

Attachment: Aurora

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: sec-high security issue. See also bug 1255270 and bug 1279208
[Describe test coverage new/current, TreeHerder]: there is minimal test coverage for the feature I'm changing on treeherder. We should still manually verify the bug is fixed once this has landed and stuck, and that the feature (icons in the tab previews on windows) isn't some kind of catastrophically broken.
[Risks and why]: reasonably low. Feature is off by default anyway, but the code we're changing always runs (results just aren't seen).
[String/UUID change made/needed]: no.

Comment 25

[:Gijs (he/him)]

Attachment: Beta

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: sec-high security issue. See also bug 1255270 and bug 1279208
[Describe test coverage new/current, TreeHerder]: there is minimal test coverage for the feature I'm changing on treeherder. We should still manually verify the bug is fixed once this has landed and stuck, and that the feature (icons in the tab previews on windows) isn't some kind of catastrophically broken.
[Risks and why]: reasonably low. Feature is off by default anyway, but the code we're changing always runs (results just aren't seen).
[String/UUID change made/needed]: no.

Comment 26

[:Gijs (he/him)]

Attachment: ESR 45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: sec-high security bug (windows-only)
Fix Landed on Version: 50, uplifted to 48/49 assuming I get beta/aurora uplift approval
Risk to taking this patch (and alternatives if risky): reasonably low. The user-visible part of this fix is off by default anyway, but the security implications apply even when the feature is off because the code is... not very clever. We should change this separately (not for ESR). Note that the previous fixes in this domain (1279208 and 1255270) have just landed on ESR.
String or UUID changes made by this patch: no.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b005ddc33470
https://hg.mozilla.org/mozilla-central/rev/1dd03896ee5f

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

windows 8 saw 2 perf improvements:
https://treeherder.mozilla.org/perf.html#/alerts?id=1728

* tabpaint: 2.8%
* tsvg_opacity: 25.2% 

thanks for making Firefox faster!

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8768580 [details] [diff] [review]
Beta

Sec-high, Beta48+

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8768579 [details] [diff] [review]
Aurora

Sec-high, Aurora49+

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8768581 [details] [diff] [review]
ESR 45

Sec-high, ESR45+

Comment 32

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/24e82cb41040
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dd9395767575
remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/d2be13b2616c

Would be good if QA can verify this on Windows as well.

Toni, can you verify this is fixed on nightly? (yesterday's nightly should do - https://queue.taskcluster.net/v1/task/L5XXvVAARIu_pPfTREor9A/artifacts/public%2Fbuild%2Ffirefox-50.0a1.en-US.win32.zip - or you can wait for today's...)

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out of esr45 because it caused mass bustage on Windows.

Example failure: https://treeherder.mozilla.org/logviewer.html#?job_id=59481&repo=mozilla-esr45


https://hg.mozilla.org/releases/mozilla-esr45/rev/93487b5d9467

Comment 34

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #33)
> I had to back this out of esr45 because it caused mass bustage on Windows.
> 
> Example failure:
> https://treeherder.mozilla.org/logviewer.html#?job_id=59481&repo=mozilla-
> esr45
> 
> 
> https://hg.mozilla.org/releases/mozilla-esr45/rev/93487b5d9467

I made a mistake resolving the conflicts in the patch. Updated, built locally and tested it's not as busted as the previous attempt, and now relanded:

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/057e4d2a39cc

Comment 35

[:Gijs (he/him)]

Verified on nightly and aurora in bug 1279208. ESR verification pending, rebase issue on beta was addressed just now, should be re-verified with beta 9 or later.

Comment 36

[Toni Huttunen]

I confirm that the fix works with nightly ;)

Comment 37

[:Gijs (he/him)]

Also verified in bug 1279208 for esr45 (on tinderbox, the new release will be a while still). Marking verified.

Comment 38

[Georg Koppen]

(In reply to :Gijs Kruitbosch from comment #5)
> I think we should have a separate non-security bug about the fact that all
> this code runs even when the feature is disabled, but I didn't want to
> attempt to fix that here as it wouldn't address the fundamental security
> issue.

Did you file a follow-up bug somewhere? Getting this fixed might be neat as I suspect this is related to an issue in our bug tracker (https://trac.torproject.org/projects/tor/ticket/16747) where favicons are downloaded twice but one of the requests is not using the Tor circuit it should.

Comment 39

[:Gijs (he/him)]

(In reply to Georg Koppen from comment #38)
> (In reply to :Gijs Kruitbosch from comment #5)
> > I think we should have a separate non-security bug about the fact that all
> > this code runs even when the feature is disabled, but I didn't want to
> > attempt to fix that here as it wouldn't address the fundamental security
> > issue.
> 
> Did you file a follow-up bug somewhere? Getting this fixed might be neat as
> I suspect this is related to an issue in our bug tracker
> (https://trac.torproject.org/projects/tor/ticket/16747) where favicons are
> downloaded twice but one of the requests is not using the Tor circuit it
> should.

That it's not using the tor circuit may be fixed if it's this one, but I suspect it's the tab request. Could be wrong though. Anyway, yes, I filed (and fixed) a bug: bug 1285196.

Comment 40

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #39)
> (In reply to Georg Koppen from comment #38)
> > (In reply to :Gijs Kruitbosch from comment #5)
> > > I think we should have a separate non-security bug about the fact that all
> > > this code runs even when the feature is disabled, but I didn't want to
> > > attempt to fix that here as it wouldn't address the fundamental security
> > > issue.
> > 
> > Did you file a follow-up bug somewhere? Getting this fixed might be neat as
> > I suspect this is related to an issue in our bug tracker
> > (https://trac.torproject.org/projects/tor/ticket/16747) where favicons are
> > downloaded twice but one of the requests is not using the Tor circuit it
> > should.
> 
> That it's not using the tor circuit may be fixed if it's this one

to be clear: I made the windows preview per tab one use the same request infra as the bookmarks/places one. So that might eliminate a request, which might fix it. That happened in this bug, so you should see it if you take all these patches, even without the one from bug 1285196.

However, as noted, I suspect the problematic request is the one from the tabbrowser's <tab>.

If the windowspreviewpertab thing really is the duplicate request, it should only happen on Windows...

The general issue with the duplicate requests I would like to solve in bug 1288054. As noted there, there are existing issues that this would address, and it would make the solution to this bug a lot more straightforward than it ended up being. :-(

Orthogonally: Please make sure that for the esr branch and the tor browser, you've taken the final landings (see bug 1279208). There were some bumps getting this all uplifted to 48 and 45esr. (ni for this).

Comment 41

[Georg Koppen]

I followed this dance pretty closely, thanks for your hard work in this regard. When we are rebasing we are taking the Mozilla release tag, so choosing the wrong patch should be a non-issue but thanks for the reminder.

Yeah, the problem in the bug I linked to is a Windows only issue.