Last Comment Bug 277574 - Http auth prompt from other tabs displays over current tab
: Http auth prompt from other tabs displays over current tab
Status: VERIFIED FIXED
[sg:fix]
: fixed-aviary1.0.1, fixed1.7.6, verified1.7.6
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on: 284993
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-08 12:55 PST by Christian Schmidt
Modified: 2007-08-13 20:12 PDT (History)
11 users (show)
chofmann: blocking‑aviary1.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (404 bytes, text/html)
2005-01-08 12:56 PST, Christian Schmidt
no flags Details
Focus tab that's opening an auth dialog. (8.70 KB, patch)
2005-02-15 15:36 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Same thing for PasswordPrompt(). (9.58 KB, patch)
2005-02-16 17:52 PST, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: review+
darin.moz: superreview+
dveditz: approval‑aviary1.0.1+
Details | Diff | Review
Trunk version, move event dispatching from DOM code to the prompter. (25.55 KB, patch)
2005-02-17 15:35 PST, Johnny Stenback (:jst, jst@mozilla.com)
darin.moz: superreview+
Details | Diff | Review
Same thing w/o causing bug 282894. (25.54 KB, patch)
2005-02-20 13:39 PST, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: review+
jst: superreview+
Details | Diff | Review
Patch for 1.4 branch (8.00 KB, patch)
2005-06-29 21:06 PDT, Leon Sha
jst: superreview+
Details | Diff | Review

Description Christian Schmidt 2005-01-08 12:55:16 PST
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 2005-01-08 12:56:52 PST
Created attachment 170683 [details]
Testcase
Comment 2 Daniel Veditz [:dveditz] 2005-02-06 10:36:04 PST
-> Johnny in case it'd be easy to do the tab-focusing he did for the
prompt/alert/etc dialogs.
Comment 3 chris hofmann 2005-02-14 14:27:13 PST
+ for 1.0.1
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-15 15:36:57 PST
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...
Comment 5 Darin Fisher 2005-02-16 13:31:38 PST
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] 2005-02-16 14:01:00 PST
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] 2005-02-16 17:11:59 PST
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, jst@mozilla.com) 2005-02-16 17:52:20 PST
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.
Comment 9 Darin Fisher 2005-02-16 18:15:30 PST
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] 2005-02-17 12:39:41 PST
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, jst@mozilla.com) 2005-02-17 13:33:18 PST
Fixed on branches. Leaving bug open for fixing this on the trunk too.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-17 15:35:01 PST
Created attachment 174625 [details] [diff] [review]
Trunk version, move event dispatching from DOM code to the prompter.
Comment 13 sairuh (rarely reading bugmail) 2005-02-18 11:33:26 PST
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 2005-02-18 15:53:58 PST
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, jst@mozilla.com) 2005-02-18 19:16:30 PST
(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] 2005-02-20 02:34:42 PST
This caused crash bug 282894
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-20 13:39:41 PST
Created attachment 174902 [details] [diff] [review]
Same thing w/o causing bug 282894.

Fixes bug 282894. Marking darin's sr+
Comment 18 Daniel Veditz [:dveditz] 2005-02-20 16:39:06 PST
Comment on attachment 174902 [details] [diff] [review]
Same thing w/o causing bug 282894.

r=dveditz
Comment 19 Jay Patel [:jay] 2005-02-21 16:18:35 PST
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, jst@mozilla.com) 2005-02-23 15:33:58 PST
Fixed on the trunk.
Comment 21 Boris Zbarsky [:bz] 2005-03-06 12:34:41 PST
The Mozilla version of this patch is wrong.  See bug 284993.
Comment 22 Boris Zbarsky [:bz] 2005-03-09 13:39:55 PST
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] 2005-03-10 17:01:05 PST
Verified Fixed for the original bug.  Remaining issues are being addressed in
bug 284993.
Comment 24 sairuh (rarely reading bugmail) 2005-03-10 17:59:41 PST
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 2005-05-10 16:36:05 PDT
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 2005-06-29 21:06:01 PDT
Created attachment 187750 [details] [diff] [review]
Patch for 1.4 branch
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2005-08-11 23:03:24 PDT
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 2005-08-15 02:29:40 PDT
(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 neil@parkwaycc.co.uk 2005-08-15 03:45:20 PDT
(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 2005-08-15 03:51:47 PDT
> 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 neil@parkwaycc.co.uk 2005-08-15 04:23:47 PDT
(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 2005-08-15 04:29:35 PDT
> Was that your only issue?

Yes. In comments #27, jst gave a sr only with this question.
Comment 33 neil@parkwaycc.co.uk 2005-08-15 04:35:40 PDT
>>Was that your only issue?
>Yes.
Sounds good to me then.

Note You need to log in before you can comment on or make changes to this bug.