284993 - changing tabs on modal dialog horks url bar

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050305
regressed between buildID 2005022306 and BuildID 2005022605

my preferences for Tabbed browsing are
[ ] Hide the tab bar when only one tTab is open
[ ] Select new tabs opened from links
[ ] add tabs
my preferences for Privacy&Security -> SSL are all checked 
When I open a link using right-click 'Open Link in New Tab' or middle-click,
a new tab is created and the page loads. Focus depends on http:// or https://
A http:// page loads in the background, focus is switched to  https:// page when
the SSL warning comes up. When the warning comes up, the spinning wheel in the
tab stops spinning (didn´t stop in the builds before)

JS console is showing an Error:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.focus]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/tabbrowser.xml :: setFocus :: line 597"  data: no]

steps to reproduce:
1. create new profile, I didn´t see it in my normal profiles.
2. check preferences ( [ ] Select new tabs opened from links, all SSL warnings
enabled, I didn´t see it when warnings were disabled.
3. load http://www.mozilla.org/start/
4. on that page, look at the URLs at the left below 'Tools'
5. load using right-click 'Open Link in New Tab' the Tinderbox url.
   see it loading in a new tab, focus doesn´t change
6. load using right-click 'Open Link in New Tab' the Bugzilla url.
   see it loading in a new tab, see that focus changes to the new tab
  when the security warning comes up.

7. open JS console, see errormessage.


I´m seeing this on any https: page, like webmails or buglists
https://bugzilla.mozilla.org/buglist.cgi?query_format=&short_desc_type=allwordssubstr&short_desc=&product=Composer&product=Core&product=Mozilla+Application+Suite&product=NSPR&product=NSS&product=PSM&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&emailtype1=substring&email1=&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=-1d&chfieldto=Now&chfield=%5BBug+creation%5D&chfieldvalue=&cmdtype=doit&order=Reuse+same+sort+as+last+time&field0-0-0=noop&type0-0-0=noop&value0-0-0=

Comment 1

[Hermann Schwab]

It´s not SSL causing this bug, but the Pop-Up. So it doesn´t seem to be a
problem of ignoring preferences, but losing focus.
I also sometimes get a completely empty location bar, can´t select, can´t copy
from it, but Reload restores the URL. 

In my normal (old) profile loading of images is blocked, I can restore it by
whitelisting the website, but it gets blocked again when I remove the site from
the whitelist. I manually removed extensions from my profile, deleted chrome and
localstore, xul.mfl, to no avail, so I created a new profile, to find this bug.
I´m using nightlies, zip-builds, unzipped into new, empty directories created by
the unzipper. 

Comment 2

[Hermann Schwab]

one day timeframe for regression: 2005022305 2005022405

When I open a bug in a new tab loading in the background, the SSL Warning Pop-Up
transfers the focus from my current to the new tab, (against my preference)
I also have seen this happeningon a http: website, when a pop-up  popped up, the
new tab went active.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-02-23+03%3A00&maxdate=2005-02-24+06%3A00&cvsroot=%2Fcvsroot

Perhaps Bug 131456 Memory use does not go down after closing tabs ?
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-02-23+10%3A19&maxdate=2005-02-23+10%3A19&cvsroot=%2Fcvsroot

the error seen in JS console:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowInternal.focus]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://global/content/bindings/tabbrowser.xml :: setFocus :: line 587"  data: no]

Tabbrowser.xml:
505       <method name="updateCurrentBrowser">
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/tabbrowser.xml#505

http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/tabbrowser.xml#595
595             function setFocus(element) {
596               document.commandDispatcher.suppressFocusScroll = true;
597               Components.lookupMethod(element, "focus").call(element);
598               document.commandDispatcher.suppressFocusScroll = false;
599             }
600 
601             // Use setTimeout to avoid focus outline ghosting.
602             setTimeout(setFocus, 0, whatToFocus);
603           ]]>
604         </body>
605       </method>

Comment 3

[Boris Zbarsky [:bzbarsky]]

The popup should be focusing the tab it belongs to (security issue).  I'm not
sure whether focus is supposed to return to the other tab after that... jst, you
landed this stuff, right?  What's the designed behavior?

Comment 4

[Boris Zbarsky [:bzbarsky]]

And this is a regression from bug 277574, not bug 131456.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Looks like no one bothered to add a DOMModalDialogClosed handler to the xpfe
tabbrowser.xml...  See
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1716
for the tookit version.

Requesting blocking of 1.7.6, since the buggy code was landed on the 1.7 branch...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to add handler

This does not actually fix this bug (though I think we need this anyway).  The
new tab is still shown instead of going back to the old one.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and the JS error that's thrown also prevents the page URL from showing in
the URL bar.  Upping severity to critical based on that.

Comment 8

[Boris Zbarsky [:bzbarsky]]

As Silver points out, that's likely due to lack of the forceSyncURLBarUpdate
property in the xpfe tabbrowser, and lack of a check of it in the xpfe browser.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Sorry for the spam, reverting accidental change to severity field.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Johnny, any progress here?  We really need this for 1.7.6 and it needs to ship
soon....

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix.

Found the problem, and I think I've got the right fix for it too, thanks to
input from Darin on this.

So the problem here is that we end up firing the "DOMWillOpenModalDialog" on
the new window (tab) while we're in the middle of initializing that window.
This ends up creating a new document (about:blank) when the event is fired, and
that document ends up being used for updating the URL bar etc.

This patch changes the docshell code to initialize the new window completely
(or more so, at least) before it fires the onlocationchanged notification
(which is what causes the dialog to be opened etc).

Comment 12

[Darin Fisher]

Comment on attachment 176685 [details] [diff] [review]
Fix.

sr=darin

Comment 13

[Worcester12345]

(In reply to comment #11)
> Created an attachment (id=176685) [edit]
> Fix.
> 
> Found the problem, and I think I've got the right fix for it too, thanks to
> input from Darin on this.
> 
> So the problem here is that we end up firing the "DOMWillOpenModalDialog" on
> the new window (tab) while we're in the middle of initializing that window.
> This ends up creating a new document (about:blank) when the event is fired, and
> that document ends up being used for updating the URL bar etc.
> 
> This patch changes the docshell code to initialize the new window completely
> (or more so, at least) before it fires the onlocationchanged notification
> (which is what causes the dialog to be opened etc).


Not sure, but is this the reason the "Disable Targets for Downloads" extension
exists?

Comment 14

[Johnny Stenback (:jst)]

(In reply to comment #13)
> Not sure, but is this the reason the "Disable Targets for Downloads" extension
> exists?

I have no idea, to be honest.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176685 [details] [diff] [review]
Fix.

>Index: docshell/base/nsDocShell.cpp
>+nsDocShell::SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest,
>+                          PRBool aFireOnLocationChanged)

aFireOnLocationChange would make more sense to me here.

Past that, I would really like this to get some testing on trunk before we land
it on branch... It looks safe enough, as far as any changes to this code go,
but the ordering issues in docshell scare the dickens out of me.  :(

Also, note that this patch does not fix this bug as initially filed... It's
probably worth filing a separate bug on that.  :(

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 176685 [details] [diff] [review]
Fix.

Sure, but time is short...  We're already late on 1.7.6.  Let's get this in
tonight and see how tomorrow's builds go. Pre-approving the patch assuming all
is well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm sorry, but "time is short" is NOT a reason to ship scary changes untested.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I agree with bz here.  Also, aren't Mozilla 1.7.6 and Firefox 1.0.1 supposed to
be equivalent?

Comment 19

[Worcester12345]

(In reply to comment #14)
> (In reply to comment #13)
> > Not sure, but is this the reason the "Disable Targets for Downloads" extension
> > exists?
> 
> I have no idea, to be honest.

Here's a link: http://www.cusser.net/extensions/disabletarget/

Comment 20

[Johnny Stenback (:jst)]

Sorry for not getting this in yesterday, but it's checked in on the trunk now.

And in fact, I do *not* see this problem on the 1.7 branch. The only problem
we've got there is the same problem we've got in Firefox 1.0 and 1.0.1, and that
is that the security dialog comes up on top of the currently active tab w/o
focusing the tab where the dialog came from.

I'd vote for shipping 1.7.6 w/o this fix, we'll fix in 1.7.7 and 1.0.2.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Hermann, would you mind refiling the original issue you had?  Please cc me,
jst@moz, neil@park, and bengoodger on the bug....  We still need to deal with
that problem, since this patch just fixed the "url not showing in url bar"
behavior....  I'm sorry this bug mutated.  :(

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 176685 [details] [diff] [review]
Fix.

After talking with drivers and jst, changing to minus now since the issue that
we still see on branch is not that critical, and we already match firefox's
behavior.

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

(In reply to comment #17)
> I'm sorry, but "time is short" is NOT a reason to ship scary changes untested.

Agreed.  My approval was contingent on testing of the patch.

(In reply to comment #18)
> I agree with bz here.  Also, aren't Mozilla 1.7.6 and Firefox 1.0.1 supposed to
> be equivalent?

It's important, but it's not as important as making sure that all the security
fixes we take are good fixes.  This bug initially appeared that way, and jst
corrected that opinion.

Comment 24

[Bill Gianopoulos [:WG9s]]

The code checked in for this bug appears to have caused the regression reported
in bug 285738.

Comment 25

[Daniel Veditz [:dveditz]]

This also appears to have caused regression bug 286810

Comment 26

[Steve Chapel]

Did this cause regression bug 304243?

Comment 27

[Boris Zbarsky [:bzbarsky]]

This caused bug 322773