Closed Bug 905369 Opened 7 years ago Closed 6 years ago

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

Categories

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

All
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 28
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: TimAbraldes, Assigned: mbrubeck)

References

()

Details

(Whiteboard: [beta28] p=2 )

Attachments

(1 file, 3 obsolete files)

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.
No longer blocks: 747789
This is super annoying for any users that encounter it. I'll take a look; hopefully it can be quickly resolved
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
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
Attached patch Patch v1 (WIP) (obsolete) — Splinter Review
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
Assignee: tabraldes → nobody
Whiteboard: [preview]
Status: ASSIGNED → NEW
Blocks: metrov1backlog
No longer blocks: 892575
Whiteboard: [preview] → [triage] feature=defect c=tbd u=tbd p=0
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=0
It was suggested in a recent triage meeting that a simple fix would be to filter https pages from top sites.
That would be a bummer. On desktop 5 out of 9 of my current top sites on about:newtab are https.
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
Whiteboard: feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=0
Assignee: nobody → sfoster
Whiteboard: [block28] feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=0
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.
Assignee: sfoster → nobody
Might have to do with winrt widget not creating a "hidden window" since it would require an old school win32 window procedure.
Assignee: nobody → jmathies
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=2
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=2 → [beta28] p=2
(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.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #812848 - Attachment is obsolete: true
Attachment #8347455 - Flags: feedback?(sfoster)
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 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.
Attachment #8347455 - Flags: feedback?(sfoster) → feedback+
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
Attachment #8347455 - Attachment is obsolete: true
Attached patch fix v.2 (obsolete) — Splinter Review
Lets try this instead.
Attachment #8348094 - Flags: review?(sfoster)
http://limpet.net/pass/ is a simple test case for this bug.
Attached patch alternate patchSplinter Review
This fix works for me -- can we try this instead?
Attachment #8348116 - Flags: review?(jmathies)
Attachment #8348094 - Attachment is obsolete: true
Attachment #8348094 - Flags: review?(sfoster)
Comment on attachment 8348116 [details] [diff] [review]
alternate patch

seems even better, I also see the cached fave icon.
Attachment #8348116 - Flags: review?(jmathies) → review+
Assignee: jmathies → mbrubeck
Whiteboard: [beta28] p=2 → [beta28] p=2 [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/607c4a6ec537
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] p=2 [fixed-in-fx-team] → [beta28] p=2
Target Milestone: --- → Firefox 29
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.
Attachment #8348116 - Flags: approval-mozilla-aurora?
Attachment #8348116 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 896135
Target Milestone: Firefox 29 → Firefox 28
Flags: in-qa-testsuite?
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Setting in-testsuite- since we don't support Metro anymore
Flags: in-qa-testsuite? → in-qa-testsuite-
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.