905369 - Defect - History entries that use http auth cause auth prompt to appear over start UI

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Every time I open MetroFX, I get an auth prompt over the StartUI.

rsilveira did some investigating: If a page in your history uses http auth, then an auth prompt will appear when opening metroFX and when interacting with that history tile.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This is super annoying for any users that encounter it. I'll take a look; hopefully it can be quickly resolved

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

When updating the favicons for the startUI, we eventually call ColorAnalyzer.findRepresentativeColor [1], which attempts to add the icon as an <img> element to the hidden window.

During the process of gecko loading the icon, it calls into the prompt service asking for an auth prompt to be created. There is code in the prompt service that explicitly checks whether the prompt is being created on a/the hidden window, in which case it does not create the prompt - see [2].

This hidden-window-checking code doesn't get called in the tab-modal prompt case (which goes through openTabPrompt() rather than openModalWindow()), so we'll probably want to add the hidden-window check to the tab-modal prompt case.

However, even if you add the check to the tab-modal prompt case, it succeeds so we still create the prompt. The next step of the investigation is to figure out why nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible [3] is returning true even when the parent window is the hidden window. I recall that hidden windows works strangely in metro (or maybe don't work at all?) so the solution to this issue might be to fix hidden windows in metro.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ColorAnalyzer.js?rev=285a029a9962#29
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js?rev=72bc1aebb5d0#368
[3] https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=5dbc32740161#3213

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 (WIP)

This patch includes some changes that should get us closer to fixing this bug, as well as a ton of logging that I've added during the course of investigating (some of it is useful, some of it is decidedly not useful).

From what I've been able to gather, setting the 'src' attribute on our <img> element here [1] kicks off a request for the image, which is handled by an 'nsHttpChannel'. The 'nsHttpChannel' has an 'nsHttpChannelAuthProvider' member which makes calls that eventually result in the auth prompt being shown.

For reasons that I haven't been able to track down, the 'domwin' argument passed into the prompt service is the DOM Window of the tab we are on (named something like browser-0 or browser-1) instead of the DOM Window of the hidden window (which, for the purposes of debugging, I've explicitly given the name 'THE HIDDEN WINDOW').

I've spent a good chunk of time investigating without making a major breakthrough, so I'm going to de-assign myself to put some distance/time between myself and this issue.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/ColorAnalyzer.js?rev=285a029a9962#35

Comment 4

[Jim Mathies [:jimm]]

It was suggested in a recent triage meeting that a simple fix would be to filter https pages from top sites.

Comment 5

[Sam Foster [:sfoster] (he/him)]

That would be a bummer. On desktop 5 out of 9 of my current top sites on about:newtab are https.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Note that https sites don't necessarily use http auth and sites that use http auth aren't necessarily https.

The problem described in this bug is specific to pages that use http auth; it's unrelated to https

Comment 7

[Sam Foster [:sfoster] (he/him)]

I'm not currently working on this. Last I heard, this is a "hidden window" issue. The code that handles that auth challenge and throws up the prompt does have the smarts to not prompt when the request is from a/the(?) hidden window, but something about metrofx is confounding it and its treating these background favicon requests as originating from a non-hidden window therefore the user needs to get the prompt.

Comment 8

[Jim Mathies [:jimm]]

Might have to do with winrt widget not creating a "hidden window" since it would require an old school win32 window procedure.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #2)
> However, even if you add the check to the tab-modal prompt case, it succeeds
> so we still create the prompt. The next step of the investigation is to
> figure out why nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible [3] is
> returning true even when the parent window is the hidden window. I recall
> that hidden windows works strangely in metro (or maybe don't work at all?)
> so the solution to this issue might be to fix hidden windows in metro.

After debugging this a bit, I'm finding that the parent window that gets handed in isn't the hidden window, it's the main chrome window. Still trying to understand why that is. I suspect it has something to do with the icons loading into the start page.

Comment 10

[Jim Mathies [:jimm]]

Attachment: fix

This fixes it. I don't think we should be using direct links to these right? about:start will try to load them off the web.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8347455 [details] [diff] [review]
fix

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

::: browser/metro/modules/View.jsm
@@ +84,5 @@
>      }
>      if ("string" == typeof aIconUri) {
>        aIconUri = makeURI(aIconUri);
>      }
> +    if (aIconUri.scheme == "http") {

Drive-by comment: Is the goal here to exclude https URIs?  Please add a comment to explain the purpose of this check.

@@ +91,1 @@
>      let faviconURL = (PlacesUtils.favicons.getFaviconLinkForIcon(aIconUri)).spec;

Does it work better if we use the "moz-anno:favicon:" URI returned here as the icon source?

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8347455 [details] [diff] [review]
fix

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

::: browser/metro/modules/View.jsm
@@ +85,5 @@
>      if ("string" == typeof aIconUri) {
>        aIconUri = makeURI(aIconUri);
>      }
> +    if (aIconUri.scheme == "http") {
> +      aItem.iconSrc = aIconUri.spec;

My understanding of the bug was that it applied to any host where an HTTP auth challenge was given, triggering our modal auth prompt, regardless of which scheme it loads from. Is that not the case?

@@ +91,3 @@
>      let faviconURL = (PlacesUtils.favicons.getFaviconLinkForIcon(aIconUri)).spec;
>      let xpFaviconURI = makeURI(faviconURL.replace("moz-anno:favicon:",""));
>  

IIRC, the ColorAnalyzer didn't like the moz-anno:favicon URLs, but maybe they are fine for assigning to .iconSrc. Worth a try at least.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8347455 [details] [diff] [review]
fix

I'm going to try a different approach. We get the calls for these prompts in our prompt service.  Should be fairly straight forward to identify the about:start src and fail the prompts from there.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/components/PromptService.js

Comment 14

[Jim Mathies [:jimm]]

Attachment: fix v.2

Lets try this instead.

Comment 15

[Matt Brubeck (:mbrubeck)]

http://limpet.net/pass/ is a simple test case for this bug.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: alternate patch

This fix works for me -- can we try this instead?

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8348116 [details] [diff] [review]
alternate patch

seems even better, I also see the cached fave icon.

Comment 18

[Matt Brubeck (:mbrubeck)]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/607c4a6ec537

Comment 20

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8348116 [details] [diff] [review]
alternate patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug in new Metro front-end

User impact if declined: Undesired password dialogs may appear when Firefox for Metro launches.

Testing completed (on m-c, etc.): Landed on m-c on 12/16.

Risk to taking this patch (and alternatives if risky): Very low-risk Metro-only patch that only affects how icons are loaded on the Firefox Start page.

String or IDL/UUID changes made by this patch: None.

Comment 22

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04a669b65fb3

Comment 23

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.2; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.2; rv:29.0) Gecko/20100101 Firefox/29.0

Verified using the provided URL on latest nightly (build ID:20140108030203) and latest aurora (build ID:20140108004006).
The authentication prompt is not shown anymore over the Start UI.

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

Setting in-testsuite- since we don't support Metro anymore