The default bug view has changed. See this FAQ.

Http auth prompt from other tabs displays over current tab

VERIFIED FIXED

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: Christian Schmidt, Assigned: jst)

Tracking

({fixed-aviary1.0.1, fixed1.7.6, verified1.7.6})

unspecified
fixed-aviary1.0.1, fixed1.7.6, verified1.7.6
Points:
---
Bug Flags:
blocking-aviary1.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
This is related to bug 262887. The fix for that bug makes download dialogs and
Javascript prompt/alert/confirm dialogs focus the tab that spawned them.

I think this behaviour should apply to HTTP authentication dialogs as well.

Steps to reproduce:
1. Open testcase
2. Open the website of your bank in another tab
3. Wait 5 seconds

Expected result:
The tab containing the testcase should be focused before an HTTP authentication
dialog is opened.

Actual result:
An HTTP authentication dialog is opened while the tab showing the bank website
has focus. This gives the impressions that the bank website spawned the dialog.
(Reporter)

Comment 1

12 years ago
Created attachment 170683 [details]
Testcase
-> Johnny in case it'd be easy to do the tab-focusing he did for the
prompt/alert/etc dialogs.
Assignee: bugs → jst
Whiteboard: [sg:fix]
Flags: blocking-aviary1.0.1?

Comment 3

12 years ago
+ for 1.0.1
Flags: blocking-aviary1.0.1? → blocking-aviary1.0.1+
(Assignee)

Comment 4

12 years ago
Created attachment 174418 [details] [diff] [review]
Focus tab that's opening an auth dialog.

This makes nsPrompt do what nsGlobalWindow does when opening modal dialogs, but
only for PromptUsernameAndPassword(). We should make this more mainstream and
probably move the almost identical code from nsGlobalWindow into nsPrompt too,
but I don't want to do that on the branches...
Attachment #174418 - Flags: superreview?(darin)
Attachment #174418 - Flags: review?(dveditz)
Whiteboard: [sg:fix] → [sg:fix] need review dveditz/darin

Comment 5

12 years ago
Comment on attachment 174418 [details] [diff] [review]
Focus tab that's opening an auth dialog.

Why are you only calling DispatchCustomEvent from PromptUsernameAndPassword? 
What about PromptPassword and the other nsIAuthPrompt and nsIPrompt methods?
Why not the same for promptPassword() and select() ? Don't they have the same issue?

The older fix was in nsGlobalWindow.cpp, but it looks like that ultimately calls
down to the windowwatcher prompt as well. Should all of the event creation be
moved to the same place?

if nsPrompt calls nsPromptService to do the work, why is this not in the lowest
level? Does that catch too many unintended dialogs or something?
Johnny says we should leave the GlobalWindow stuff for the branch (if it ain't
broke...), but can move them later. I buy that
(Assignee)

Comment 8

12 years ago
Created attachment 174537 [details] [diff] [review]
Same thing for PasswordPrompt().

This does the same thing for PasswordPrompt(), and yes, eventually we'll want
to move all this cothe code that currently does in for the DOM methods into the
nsPrompot code, but I don't want to do that now for the branches. That's trunk
kind of changes IMO.
Attachment #174418 - Attachment is obsolete: true
Attachment #174537 - Flags: superreview?(darin)
Attachment #174537 - Flags: review?(dveditz)
(Assignee)

Updated

12 years ago
Attachment #174418 - Flags: superreview?(darin)
Attachment #174418 - Flags: review?(dveditz)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All

Comment 9

12 years ago
Comment on attachment 174537 [details] [diff] [review]
Same thing for PasswordPrompt().

This looks good.  Couple things that might be good for the trunk:

1) remove the code that gets a auth prompt from the tree owner since we'll
always have a windowwatcher, and that is the canonical way to get a prompt.

2) move all of the DOM event generation into nsPromptService (for window.alert,
etc.)
Attachment #174537 - Flags: superreview?(darin) → superreview+
Comment on attachment 174537 [details] [diff] [review]
Same thing for PasswordPrompt().

r=dveditz
a=dveditz for 1.7 and aviary1.0.1 branches
Attachment #174537 - Flags: review?(dveditz)
Attachment #174537 - Flags: review+
Attachment #174537 - Flags: approval-aviary1.0.1+
Whiteboard: [sg:fix] need review dveditz/darin → [sg:fix] need checkin
(Assignee)

Comment 11

12 years ago
Fixed on branches. Leaving bug open for fixing this on the trunk too.
Keywords: fixed-aviary1.0.1, fixed1.7.6
(Assignee)

Comment 12

12 years ago
Created attachment 174625 [details] [diff] [review]
Trunk version, move event dispatching from DOM code to the prompter.
Attachment #174625 - Flags: superreview?(darin)
Attachment #174625 - Flags: review?(dveditz)
tested with 2005021806-1.0.1 firefox builds on Mac 10.3.8 and Win XP, and this
looks fixed: after I open another tab to a banking site, the previous tab
test-http-auth site properly grabs focus when its login prompt appears.

verified fixed for aviary 1.0.1.

Comment 14

12 years ago
Comment on attachment 174625 [details] [diff] [review]
Trunk version, move event dispatching from DOM code to the prompter.

>Index: docshell/base/nsDocShell.cpp

> nsDocShell::GetAuthPrompt(PRUint32 aPromptReason, nsIAuthPrompt **aResult)
> {
...
>+    rv = EnsureScriptEnvironment();
>+    NS_ENSURE_SUCCESS(rv, rv);

when might this fail?


>Index: embedding/components/windowwatcher/src/nsPrompt.cpp

> nsPrompt::Confirm(const PRUnichar* dialogTitle, 
...
>+  nsAutoDOMEventDispatcher autoDOMEventDispatcher(mParent);
>+
>+  if (autoDOMEventDispatcher.DefaultPrevented()) {
>+    return NS_OK;
>+  }
>+    return mPromptService->Confirm(mParent, dialogTitle, text, _retval);
>+
>+  return NS_OK;

something's funny about this.  typo?


sr=darin
Attachment #174625 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 15

12 years ago
(In reply to comment #14)
> > nsDocShell::GetAuthPrompt(PRUint32 aPromptReason, nsIAuthPrompt **aResult)
> > {
> ...
> >+    rv = EnsureScriptEnvironment();
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> when might this fail?

During docshell destruction, and of course if we're out of memory and we get
here before we've created the script environment.

> something's funny about this.  typo?

Yeah :) Fixed.
This caused crash bug 282894
(Assignee)

Comment 17

12 years ago
Created attachment 174902 [details] [diff] [review]
Same thing w/o causing bug 282894.

Fixes bug 282894. Marking darin's sr+
Attachment #174902 - Flags: superreview+
Attachment #174902 - Flags: review?(dveditz)
Attachment #174625 - Attachment is obsolete: true
Attachment #174625 - Flags: review?(dveditz)
Comment on attachment 174902 [details] [diff] [review]
Same thing w/o causing bug 282894.

r=dveditz
Attachment #174902 - Flags: review?(dveditz) → review+

Comment 19

12 years ago
Verified Fixed on branches.  Focus now goes to the page that created the
http-auth login dialog.

Aviary 1.0.1: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050221 Firefox/1.0.1

Mozilla 1.7.6: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050218

Leaving assigned to make sure we get this on the Trunk.
(Assignee)

Comment 20

12 years ago
Fixed on the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Group: security
The Mozilla version of this patch is wrong.  See bug 284993.
Depends on: 284993
The other interesting thing, to me, is that changes to SeaMonkey UI were made
without anything resembling review from the module owner....

Comment 23

12 years ago
Verified Fixed for the original bug.  Remaining issues are being addressed in
bug 284993.
Status: RESOLVED → VERIFIED
Keywords: fixed1.7.6 → verified1.7.6
mistakenly removed fixed1.7.6 --pardon the bugspam. set your filter/quicksearch
to "ZippidityDooDahHey" to catch these for easy removal/etc/
Keywords: fixed1.7.6

Comment 25

12 years ago
This was assigned as SA14407 (Mozilla/Firefox/Thunderbird);
http://secunia.com/advisories/14407/ , see vulnerability #2) and as Mozilla
Foundation Security Advisory 2005-21 at
http://www.mozilla.org/security/announce/mfsa2005-21.html .

Comment 26

12 years ago
Created attachment 187750 [details] [diff] [review]
Patch for 1.4 branch
Attachment #187750 - Flags: superreview?(dveditz)
Attachment #187750 - Flags: superreview?(dveditz) → superreview?(jst)
(Assignee)

Comment 27

12 years ago
Comment on attachment 187750 [details] [diff] [review]
Patch for 1.4 branch

sr=jst. The 1.4 branch already got the UI code (tabbrowser.xml) to handle these
events, right?
Attachment #187750 - Flags: superreview?(jst) → superreview+

Comment 28

12 years ago
(In reply to comment #27)
> (From update of attachment 187750 [details] [diff] [review] [edit])
> sr=jst. The 1.4 branch already got the UI code (tabbrowser.xml) to handle these
> events, right?
> 
"DOMWillOpenModalDialog" is hadled by tabbrowser.xml in mozilla 1.4.
But there is no handler for "DOMModalDialogClosed".
Also for mozilla, both in trunk and mozilla 1.7 "DOMModalDialogClosed" was not
been handled.
This event is handled only in toolkit/content/widgets/tabbrowser.xml, which is
in aviary tree.

Comment 29

12 years ago
(In reply to comment #28)
>But there is no handler for "DOMModalDialogClosed".
>Also for mozilla, both in trunk and mozilla 1.7 "DOMModalDialogClosed" was not
>been handled.
The event is only there to support a setTimeout hack which was checked in by
blake under the comment "testing something..."

Comment 30

12 years ago
> The event is only there to support a setTimeout hack which was checked in by
> blake under the comment "testing something..."

OK. Then I can check this into 1.4branch?

Comment 31

12 years ago
(In reply to comment #30)
>>The event is only there to support a setTimeout hack which was checked in by
>>blake under the comment "testing something..."
>OK. Then I can check this into 1.4branch?
Was that your only issue?

Comment 32

12 years ago
> Was that your only issue?

Yes. In comments #27, jst gave a sr only with this question.

Comment 33

12 years ago
>>Was that your only issue?
>Yes.
Sounds good to me then.

Updated

10 years ago
Whiteboard: [sg:fix] need checkin → [sg:fix]
You need to log in before you can comment on or make changes to this bug.