277574 - Http auth prompt from other tabs displays over current tab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Christian Schmidt]

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.

Comment 1

[Christian Schmidt]

Attachment: Testcase

Comment 2

[Daniel Veditz [:dveditz]]

-> Johnny in case it'd be easy to do the tab-focusing he did for the
prompt/alert/etc dialogs.

Comment 3

[chris hofmann]

+ for 1.0.1

Comment 4

[Johnny Stenback (:jst)]

Attachment: 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...

Comment 5

[Darin Fisher]

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?

Comment 6

[Daniel Veditz [:dveditz]]

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?

Comment 7

[Daniel Veditz [:dveditz]]

Johnny says we should leave the GlobalWindow stuff for the branch (if it ain't
broke...), but can move them later. I buy that

Comment 8

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 9

[Darin Fisher]

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.)

Comment 10

[Daniel Veditz [:dveditz]]

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

r=dveditz
a=dveditz for 1.7 and aviary1.0.1 branches

Comment 11

[Johnny Stenback (:jst)]

Fixed on branches. Leaving bug open for fixing this on the trunk too.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Trunk version, move event dispatching from DOM code to the prompter.

Comment 13

[sairuh (rarely reading bugmail)]

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

[Darin Fisher]

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

Comment 15

[Johnny Stenback (:jst)]

(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.

Comment 16

[Daniel Veditz [:dveditz]]

This caused crash bug 282894

Comment 17

[Johnny Stenback (:jst)]

Attachment: Same thing w/o causing bug 282894.

Fixes bug 282894. Marking darin's sr+

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 174902 [details] [diff] [review]
Same thing w/o causing bug 282894.

r=dveditz

Comment 19

[Jay Patel [:jay]]

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.

Comment 20

[Johnny Stenback (:jst)]

Fixed on the trunk.

Comment 21

[Boris Zbarsky [:bzbarsky]]

The Mozilla version of this patch is wrong.  See bug 284993.

Comment 22

[Boris Zbarsky [:bzbarsky]]

The other interesting thing, to me, is that changes to SeaMonkey UI were made
without anything resembling review from the module owner....

Comment 23

[Jay Patel [:jay]]

Verified Fixed for the original bug.  Remaining issues are being addressed in
bug 284993.

Comment 24

[sairuh (rarely reading bugmail)]

mistakenly removed fixed1.7.6 --pardon the bugspam. set your filter/quicksearch
to "ZippidityDooDahHey" to catch these for easy removal/etc/

Comment 25

[Juha-Matti Laurio]

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

[Leon Sha]

Attachment: Patch for 1.4 branch

Comment 27

[Johnny Stenback (:jst)]

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?

Comment 28

[Leon Sha]

(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

[[e-mail]]

(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

[Leon Sha]

> 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

[[e-mail]]

(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

[Leon Sha]

> Was that your only issue?

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

Comment 33

[[e-mail]]

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