Closed Bug 101723 Opened 23 years ago Closed 22 years ago

lock icon only works for the first tab

Categories

(SeaMonkey :: Tabbed Browser, defect)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: a.schild, Assigned: jag+mozilla)

References

(
URL
)

Details

(Keywords: privacy, relnote, Whiteboard: [ADT1])

Attachments

(2 files, 7 obsolete files)

Screen shot of bogus security info displayed after original secure tab is closed
34.59 KB, image/png
Details
null check securityUI, fix jar.mn
27.08 KB, patch
samir_bugzilla
: review+
jag+mozilla
: superreview+
asa
: approval+
Details | Diff | Splinter Review 
When using the multitab interface and visit a http page in one tab, and a https
page in another tab, then the "lock" icon in the lower right corner of the
browser don't get updated as I change between the different tabs.
It seems that it always displays the "status" of the first tab.
The same (wrong) behaviour exists for the View-Pageinfo menu.
Confirming I see the same thing, build 2001092603, Win98.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 102205 has been marked as a duplicate of this bug. ***
-> tabbrowser
Component: XP Toolkit/Widgets → Tabbed Browser
QA Contact: jrgm → blakeross
Summary: Using the new multitab-interface doesn't reflect the correct "security" infos in the status bar → tabbrowser: "security" info in the status bar confused between tabs
*** Bug 102368 has been marked as a duplicate of this bug. ***
*** Bug 103153 has been marked as a duplicate of this bug. ***
*** Bug 103382 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
*** Bug 105515 has been marked as a duplicate of this bug. ***
*** Bug 105864 has been marked as a duplicate of this bug. ***
*** Bug 106174 has been marked as a duplicate of this bug. ***
*** Bug 106210 has been marked as a duplicate of this bug. ***
*** Bug 106364 has been marked as a duplicate of this bug. ***
*** Bug 106814 has been marked as a duplicate of this bug. ***
*** Bug 107378 has been marked as a duplicate of this bug. ***
Confirming same problem on RH Linux 7.1.  cvs build 20011029

->097
Target Milestone: mozilla1.0 → mozilla0.9.7
*** Bug 109945 has been marked as a duplicate of this bug. ***
*** Bug 110019 has been marked as a duplicate of this bug. ***
13 dupes in a month and a half...yipes!

Making the summary excessively search-friendly.
OS: Windows 2000 → All
Hardware: PC → All
Summary: tabbrowser: "security" info in the status bar confused between tabs → tabbrowser: "security" (SSL) info/button/icon/padlock in the status bar only reflects first tab/page
*** Bug 103789 has been marked as a duplicate of this bug. ***
*** Bug 110346 has been marked as a duplicate of this bug. ***
*** Bug 110542 has been marked as a duplicate of this bug. ***
Nearly filed a bug myself, despite searching for icon, padlock and https. Wonder
why this bug didn't turn up?

Shouldn't this be assigned under the Security: general component?

Same symptom confirmed on build Mozilla/5.0 (Windows; U; Win98; en-US;
rv:0.9.5+) Gecko/20011116.

*** Bug 111094 has been marked as a duplicate of this bug. ***
confirming bug with build 2001112009 (rv. 0.9.6, Win98)
*** Bug 112137 has been marked as a duplicate of this bug. ***
I'm glad to see that this is only a bug with the tab interface... I nearly got
heart failure to see the insecure lock icon after I placed an order online while
browsing in tab mode!!
Target Milestone: mozilla0.9.7 → mozilla0.9.8
*** Bug 113342 has been marked as a duplicate of this bug. ***
QA Contact: blakeross → sairuh
*** Bug 114044 has been marked as a duplicate of this bug. ***
*** Bug 114822 has been marked as a duplicate of this bug. ***
*** Bug 115978 has been marked as a duplicate of this bug. ***
*** Bug 115803 has been marked as a duplicate of this bug. ***
*** Bug 116560 has been marked as a duplicate of this bug. ***
*** Bug 116736 has been marked as a duplicate of this bug. ***
Blocks: 117203
see bug 117203 (which I bet is fixed if this problem gets fixed) for some 
privacy issues with this behavior.
Keywords: privacy
Could bug 106659 be a duplicate of this? It seems related in any case.

-Z
should we nominate this as a most frequent dupe bug?
*** Bug 117674 has been marked as a duplicate of this bug. ***
*** Bug 118232 has been marked as a duplicate of this bug. ***
*** Bug 118349 has been marked as a duplicate of this bug. ***
Bug owner: what is the plan to restore proper security to the browser when
tabbed browsing is enabled.  I think that the 0.9.8 target is a must to see this
bug fixed.  If you need the assistance of the PSM team please contact me.

cc lord.

*** Bug 118836 has been marked as a duplicate of this bug. ***
When the original first tab is closed, the padlock, etc. continue to show the
status of the original first tab, not the new first tab.  HTH
This is a simple bug to fix.  onSecurityChange just needs to be called when you
switch tabs (which is what I do for onLocationChange already).
Target Milestone: mozilla0.9.8 → mozilla0.9.9
*** Bug 120768 has been marked as a duplicate of this bug. ***
Reassigning to new component owner.
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
Peter, let me know if you need any help from the security engineering team. This
is a serious security flaw in tabbed browsing, and at least two Mozilla
milestones have been released with this bug.

*** Bug 121353 has been marked as a duplicate of this bug. ***
See my bug 121361 that shows a complete testcase and deals also about the "Page
Info" Security tab.
Blocks: 121361
Blocks: 120043
No longer blocks: 121361
*** Bug 121361 has been marked as a duplicate of this bug. ***
Egads.  Comment #42 seems to imply that once the icon is screwed up, it's
permanently screwed up.  A forced refresh/reload of a known-secure page
certainly does /not/ fix the icon for me, using 0.9.7.

Follow-on info for my previous comment (#50):  it's not just the "first tab,"
it's any non-https tab.

I thought that maybe I could just do my https browsing in the first tab, and
everything else in other tabs.  Nope.

I started a fresh Mozilla, went to a known-secure site, and the padlock was
good.  I popped up a second tab, went to mozilla.org, and the icon changed to
unlocked.  From then on it didn't matter what tabs I closed or refreshed or
anything, that padlock stayed in insecure mode.
Component: Tabbed Browser → Page Info
QA Contact: sairuh → pmac
Regarding comment #43, this is not as simple as just adding a onSecurityChange
call after the onLocationChange call at line 374 of
xpfe/global/resources/content/bindings/tabbrowser.xml (although, that needs to
be done, as well).

The security icon does not change for any tab besides the first tab. If it was
_just_ the update on switching tabs that was a problem, that should work (ie.
going to a secure site in the 4th tab should update the icon, but switching
between an unsecure tab and a secure tab would not).

Additionally, if you go to a site with mixed security (https://www.amazon.com/
should do this for you) you should get warnings (assuming you haven't disable
them, of course). You do get these when browsing in the first tab, but not in
any other tab. The code responsible for generating these warnings is in the
onStateChange handler found in
security/manager/boot/src/nsSecureBrowserUIImpl.cpp. This same code is
responsible for generating all onSecurityChange events. Since we see neither the
warnings for mixed security nor the icon change, I believe this listener is not
getting events for the non-first tabs. I don't yet understand why, though, since
that listener should be associated with the tabbrowser object. The Init function
in nsSecureBrowserUIImpl.cpp is where is calls AddProgressListener to bind itself.

Regardless, I believe the test on line 213 of
xpfe/global/resources/content/bindings/tabbrowser.xml will cause problems (and
perhaps the one discussed above), as state changes for a particular tab's
browser may occur when that tab is not being viewed. If a tab is loaded in the
background all of its state changes will not be passed on to listeners. While
that is what we want most of the time, we don't want it to miss security related
things. Of course, it (nsSecureBrowserUIImpl) will try to update the security
icon inappropriately, setting the state directly, not via onSecurityChange, so
that will be another problem. It can easily be fixed, though, by just calling
onSecurityChange on the browser, rather than changing the icon directly from
onStateChange.

Second problem: once we have onSecurityState events being sent to the tabbrowser
listener appropriately, we will have to remember the state for each browser (as
it cannot be re-determined for certain without reloading) even if it's not the
currently selected tab (we only propagate the onSecurityChange event if it's the
current tag). Then when the tab selection changes, we send an onSecurityChange
event with the stored security state for that page (the addition at line 374
mentioned at the beginning of this comment). Ideally, onStatusChange events
would be handled this way, as well.

So, in summary, I believe this is what needs to be done for this bug, but I
can't figure out why the onStateChange handler in nsSecureBrowserUIImpl.cpp is
not responding for tabs #2 and higher. Either it's not getting called, or it's
responding incorrectly. I don't have the means to test this myself, at the moment.
The reason it's not getting called for tabs other than the first one is that we
register the explicit content window in that tab, since that's what
nsSecureBrowserUIImpl listens to for page load changes.

To summarize what Justin is saying, we'll need to hook up a SecureBrowserUI
object per tab, store the state per tab while also passing through the selected
tab's state to the browser UI, and update the browser UI when switching tabs.

The tricky part is when we do throw up dialogs, since there's no clear visual
indication which tab the dialog belongs to. Though generally one should be able
to find the originating tab from the info provided in the dialog, I'm not sure
if this is satisfactory.
Regarding dialogs, a similar problem already exists with background loading tabs
and things like accept cookie prompts. You're looking at one page, but being
asked about cookies or images related to another page. However, I don't think
it's a big deal, as all of these dialogs (including security related ones) will
come very shortly after the user has done something to initiate a background tab
load. They will know why they are getting the dialogs, and really, a user's
response to any of these dialogs will not depend on them seeing what has loaded
so far. In fact, with these security dialogs they can only select "ok".

when a tab gives an alert,
focus that tab and show the alert

something to think about:
instead of modal alerts, consider (in the future) providing hooks to do "sheets"
like Mac OS X. A sheet is a dialog that is "attached" to a window.

So, if you have five untitled text files, and you do File-Save, a sheet can pop
up (if the application is using that API) which is attached to the specific text
document. If you want, you can switch over to another of the text documents and
the previous window (and it's connected sheet) will happily wait for your
attention in a background window.

If Mozilla could do something like sheets (but it would have to support per-tab
sheets, not just per-window), then we'd have a perfect way to do alerts and what
not. Just put an alert sheet in the tab. Next time you go to that tab, deal with
the alert. Mozilla won't bother you about it before then (well, maybe it could
put some '!' mark on the tab or something to get your attention in a
non-obtrusive fashion).

Sheets a pretty neat UI tool.

-matt
(does anyone who knows XPI/XUL lingo want to translate "per-tab sheets support"
into an RFE that would make sense?)
Sheets may be cool, but IMHO have nothing to do with fixing this bug or getting 
to Moz 1.0.  I think Matt's opening suggestion is the "right thing" to do:

> when a tab gives an alert,
> focus that tab and show the alert

Substitute the word "window" for "tab" in his suggestion, and you have 
described the behavior of practically every modern windowing UI.
Bug 102120 seemes to be related to this somehow - you might not want the browser
to automatically change the focus to a new tab or to any tab displaying an error.

*** Bug 122487 has been marked as a duplicate of this bug. ***
*** Bug 122588 has been marked as a duplicate of this bug. ***
This attachment illustrates the sort of absurdity that can result from this
bug.  To produce it, I opened a secure page, then opened a new tab with
Bugzilla's home page in it, then closed the first tab.	Then I brought up the
page-info security dialog, and that's what this screen shot is.  As you can
see, it now claims that bugzilla.mozilla.org has a certificate of verified
authenticity.  It seems to have transferred all the security info from the
other site to the new one, filling in the hostname from Bugzilla instead of the
site the certificate is really associated with.  There are some pretty big
security risks entailed here, possibly leading people to incorrectly trust
and/or distrust sites that don't deserve it -- maybe thinking a site is secure
when it isn't, or thinking that a site is trying to "hijack" another site's
certificate when no such thing is happening.
regarding comment #56, I find forced synchronous interaction very intrusive, and
it would take much out of the usefulnes of tabs (I can already do similar things
with additional windows which are then notifying me asynchronously). The "sheet"
idea is much better, imho, so when switching to a different tab, the associated
events and/or user interaction will be handled there (ie, confirming cookies
and whatnot).

*** Bug 122775 has been marked as a duplicate of this bug. ***
This bug currently has 34 dups and 30 votes.
Shouldn't the severity be raised to Major?
In reply to 57, I personaly dislike apps taking focus.. Hell, I even like focus
going to, for example 'open link in new window'. The reason Im doing that is
because I want to deal with it later..

But anyway, the point is that if you shouldnt change focus to a tab with an
error. Prehaps the problem tab could blink (like Win start bar boxes) if there
is an error.
this bug has nothing to do with fixing error popups in a new tab, its about
security icon.. Hyatt said its an easy fix in comment #43, and this by the way
doesn't affect the security of Moz.. its just a visual side effect of tab
browsing being added.

for popup's and errors see either bug 28586 or bug 80293, if you are really
concerned, while going to a secure site, don't use more than one tab for now.
make that all relevant security info.. not just 'icon'.  If you have some cool
ideas about how to implement hooks for errors to new tabs, then please feel free
to stop by the latter bug I listed, the first bug I listed is about inability to
find DNS entries.  
just tossing in my two bits...

http://bugzilla.mozilla.org/show_bug.cgi?id=101723#c55

I stand by my comment linked above that sheets would be an ideal way to handle
things, but I modify my position to say that focusing the tab would be less than
ideal, and perhaps the tab could blink or turn red or something to get my
attention without forcing me to leave the current tab.

-matt
This bug should REALLY be elevated to CRITICAL severity/priority. It is a MAJOR
security flaw.

Regarding Comment #65 From Dennis (dman84) 2002-02-03 16:43

> ....if you are really
> concerned, while going to a secure site, don't use more than one tab for now.

It is impossible to browse any secure sites and get the correct security
information if any tabs have been used at any time. The first URL a user enters
means that TWO tabs are open. Closing the first "about:blank" tab leaves the
second one with the buggy security behaviour.

If you can't fix the buggy tab behaviour, perhaps it would be better to ensure
that for any secure site, a "New Navigator Window" is opened instead of
continuing in the current tab. At least until the errant behaviour is fixed.
*** Bug 123297 has been marked as a duplicate of this bug. ***
*** Bug 123331 has been marked as a duplicate of this bug. ***
*** Bug 122993 has been marked as a duplicate of this bug. ***
this is now relnoted for 097, 098 and future until it's fixed.
Keywords: relnote
*** Bug 123478 has been marked as a duplicate of this bug. ***
This really seems like it should be a higher priority and a much worse severity
than it is.  It's survived 5 months now through multiple releases -with nothing
mentioned in the release notes - known bugs-.  It gives people the false sense
of security simply because people are trained into looking for the lock to
-ensure- that a secure encrypted connection is going on (yay, for wetware
programming!), and that the blocking bugs give misleading information to the
user.  This bug is still marked as new, and for some reason, doesn't have enough
keywords or something to show up, which is why we're duping so much (I searched
multiple times before filing my dupe).
My apologies, it is in the release notes under "New Additions" and not
"Security" which is where I was looking.

I have a tendency to agree with your assessment. It appears that this issue has 
been around for a while, and since it (visually appears) to affect security 
perhaps it should be given a bit more significance.
Accepting, moving to tabbed browsing, changing summary.
Status: NEW → ASSIGNED
Component: Page Info → Tabbed Browser
Summary: tabbrowser: "security" (SSL) info/button/icon/padlock in the status bar only reflects first tab/page → lock icon only works for the first tab
marking nsbeta1+, this must be fixed for mozilla1.0/MachV
Component: Tabbed Browser → Page Info
Keywords: nsbeta1 → nsbeta1+
Summary: lock icon only works for the first tab → tabbrowser: "security" (SSL) info/button/icon/padlock in the status bar only reflects first tab/page
La.
Component: Page Info → Tabbed Browser
Summary: tabbrowser: "security" (SSL) info/button/icon/padlock in the status bar only reflects first tab/page → lock icon only works for the first tab
*** Bug 123724 has been marked as a duplicate of this bug. ***
i actually put it near security, endico moved it to the new section.  placement 
in relnotes is an interesting thing.  Sometimes when writing release notes I 
discuss it in the bug, sometimes on irc.  I don't see anywhere in the bug 
where people expressed an interest in providing input for a release note so 
I'm not ashamed (or anything else) at not having discussed its content or 
placement in this bug.  I think perhaps we should color or somehow give new 
entries some special style.  Next time I have time to play with release notes 
(say 1.1) I'll discuss possible approaches (one would be to use iframes so that 
a release note could appear in multiple places).
Re. comment #74, it's not only the icon that shows the wrong security info, but
clicking on it to retrieve security details also shows the wrong info. So there
is much more wrong than just an icon display. You have no means to get the
correct security info for a tab, as it seems. IMO.
*** Bug 123777 has been marked as a duplicate of this bug. ***
Re comment 82, see my attachment showing how I got Mozilla to claim that
bugzilla.mozilla.org had a verified secure certificate.  In some circumstances,
Mozilla will mix security info from one site with the hostname of another, which
is likely to be thoroughly confusing to users and may mislead them greatly in
deciding what sites to trust.
http://bugzilla.mozilla.org/attachment.cgi?id=67084&action=view
Re: comment 82 and 84, the page info display extracts security info from the
SecureBrowserUI object, which is accessed through an attribute set on the
packlock window. Since fixing the padlock icon requires creating SecureBrowserUI
objects for all content windows, fixing the page info portion of the issue is
trivial: just change the padlock's window attribute to point to the current
tab's SecureBrowserUI (~3 additional lines). Of course, it's possible I missed
something which makes it much more elaborate...
QA Contact: pmac → sairuh
*** Bug 124196 has been marked as a duplicate of this bug. ***
*** Bug 124364 has been marked as a duplicate of this bug. ***
Blocks: 124140
*** Bug 124669 has been marked as a duplicate of this bug. ***
-> 1.0
For real.
Target Milestone: mozilla0.9.9 → mozilla1.0
*** Bug 125249 has been marked as a duplicate of this bug. ***
*** Bug 125329 has been marked as a duplicate of this bug. ***
*** Bug 125969 has been marked as a duplicate of this bug. ***
*** Bug 126197 has been marked as a duplicate of this bug. ***
*** Bug 126254 has been marked as a duplicate of this bug. ***
*** Bug 126412 has been marked as a duplicate of this bug. ***
Verified in 0.9.7. and 0.9.8 milestones on WinXP

IMHO, the severity of this bug should be increased as it has major impact on
user confidence when dealing with https/http sites in tabs.

Most users, including myself, have been 'trained' to trust the padlock. As is,
the padlock CANNOT be trusted.

Greetings,
  Ed.
*** Bug 126542 has been marked as a duplicate of this bug. ***
This bug creates a potential security problem for end-users. Increasing severity
to major. I'm sure the developers are already working on it, (nsbeta1+), so the
severity is basically a formality. Appending a trailing "/" to the URL.
URL: https://www.thawte.com → https://www.thawte.com/
Severity: normal → major
*** Bug 127039 has been marked as a duplicate of this bug. ***
*** Bug 127091 has been marked as a duplicate of this bug. ***
It seems that many elements are not correctly refreshed when many tabs are
presents, see bug 127152
Andrew: nsbeta1+ doesn't mean developers are already working on it, it means 
that _netscape_ developers should work on it for nsbeta1 (whenever/whatever 
that is).
Having read some other point of view in the forum, I think that this bug should
take one of the following directions:

   - nominate it for mozilla1.0 to follow the already 48 votes for this bug,
   - remove the tabbed browsing from Mozilla 1.0 because it is not mature enough.

You surely have understood that I am for option one.
I'd second that suggestion to go for option 1. I certainly wouldn't want the
tabbed browsing feature to be removed - although it has its flaws, I find it
extremely useful.
Keywords: mozilla1.0
*** Bug 127890 has been marked as a duplicate of this bug. ***
*** Bug 128183 has been marked as a duplicate of this bug. ***
*** Bug 128203 has been marked as a duplicate of this bug. ***
*** Bug 128247 has been marked as a duplicate of this bug. ***
*** Bug 128275 has been marked as a duplicate of this bug. ***
Adding I3 to Status Whiteboard for the time being until ADT can identify a
marking process. MachV should NOT ship without this bug being resolved due to
security concern.
Whiteboard: I3
Blocks: 128632
I made a version of my "too big" tabbrowser patch
which fixes also this.
(Sorry that the fix is included in the patch which
is actually made for other bugs. But it
should be pretty easy to understand which
parts have something to do with this bug.)

The diff is in
http://www.cs.helsinki.fi/u/pettay/bugzilla/bug113798_with_securityUI/

This patch is tested only in Linux and it may still have
some problems with securityUI (even if I haven't found any).
*** Bug 129091 has been marked as a duplicate of this bug. ***
Whiteboard: I3 → [ADT1]
Attached patch fix, taken from parts of patch from comment 112 (obsolete) — — Splinter Review 
I took the parts of the patch in comment #113 that had something to do with
this bug.
Ah, excellent, this is what I was thinking of. Your fix is basically correct (it
sets up a secure browser ui object per browser, hooks it up correctly, and deals
with storing the state per browser and broadcasting it to the UI when switching
tabs).

In addition I was thinking about moving the securityUI.js logic into browser's
constructor (that setPropertyAsSupports("xulwindow", window) can be removed,
b.t.w.), looking at an attribute |securitybutton| (yeah, that should be made
more generic at some point) to see whether to init with the button referenced by
the id, or with null. Then remove securityUI.js from the tree, fix mail/news and
what else was using that to specify |securitybutton|, except for navigator of
course which will use your code.

Put the securityButton in nsBrowserStatusHandler on that object as a member
instead of a var in navigator.js.

+            mBrowser: aTabBrowser.getBrowserForTab(aTab),

It doesn't look like we need that change in tabbrowser.xml (for this patch, at
least).

What do you think, Smaug?
I have a question.
When I open more than 40 tabs in a window on Win2K, scroll bar disappears in 
all tabs and switching between tabs is extremely slow until you close couple of 
tabs.  Is it a part of the same bug?
The line:
+            mBrowser: aTabBrowser.getBrowserForTab(aTab),
is for other bugs.

I think we should put the security stuff to the tabbrowser
and try to keep the browser as simple as possible.


(BTW. I think the tabbrowser.xml should not be in
global/bindings but in navigator -folder. )
*** Bug 129339 has been marked as a duplicate of this bug. ***
Attached patch updated patch (obsolete) — — Splinter Review 
just removed 
+	     mBrowser: aTabBrowser.getBrowserForTab(aTab),
as suggested.
*** Bug 129362 has been marked as a duplicate of this bug. ***
*** Bug 129391 has been marked as a duplicate of this bug. ***
regarding comment #116, that is a totally seperate bug regarding tabs overflow
with too many tabs in view.  see bug 66919 and bug 12759 for related way to
handle that.
In the newest "big tabbrowser patch"
(http://www.cs.helsinki.fi/u/pettay/bugzilla/bug113798_with_securityUI/) 
the gSecurityButton is in nsBrowserStatusHandler and
the initialization of the securityUI is in browser.xml.
The securityUI.js is becoming pretty useless.
Attached patch Fix security lock and page info for tabs (obsolete) — — Splinter Review 

        Attachment #72669 -
        Attachment is obsolete: true

        Attachment #72846 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        above, -uw (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

        Attachment #73102 -
        Attachment is obsolete: true

        Attachment #73103 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        above, -uw (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
On advice of kaie, who's working on the mail/news security lock/info stuff for 
both the messenger window and the mail compose window, I've removed the 
securityOverlay.xul from the mail/news code, since it's going to use code more 
suited to their needs.

Smaug, what do you think of the patch like this? It's basically what I had in 
mind, and I've been able to reuse some of your code here, thanks for that.

I just discovered that we're not setting the tooltip text with this patch, let 
me ponder on how to do that for a while.

    
  

        Attachment #73132 -
        Flags: needs-work+

    
    

    
  
Maybe the onSecurityChange could use switch-case.
It should be a bit faster.

onSecurityChange : function(aWebProgress, aRequest, aState)
  {
    const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
    switch (aState) {
      case
(nsIWebProgressListener.STATE_IS_SECURE|nsIWebProgressListener.STATE_SECURE_HIGH):
        this.gSecurityButton.setAttribute("level", "high");
        break;
      case
(nsIWebProgressListener.STATE_IS_SECURE|nsIWebProgressListener.STATE_SECURE_LOW):
        this.gSecurityButton.setAttribute("level", "low");
        break;
      case nsIWebProgressListener.STATE_IS_BROKEN:
        this.gSecurityButton.setAttribute("level", "broken");
        break;
      default:
        this.gSecurityButton.removeAttribute("level");
        break;
    }
}

And the securityState
should be cached in nsBrowserStatusHandler.js, so that we call setAttribute()
only when there is really a securityChange (?).  

    
    

    
  
Yeah, I thought about the switch. We could add the caching, but I don't see that
as being an immediate performance problem, so no need to complicate the code
more I think.

There's another issue where we don't store the current state when we're not in
tabbed mode, and then when we switch we "lose" the current state.

    
    

    
  
Wow, finally a case for the much-hated with statement:

onSecurityChange : function(aWebProgress, aRequest, aState)
  {
    with (Components.interfaces.nsIWebProgressListener) {
      switch (aState) {
        case STATE_IS_SECURE | STATE_SECURE_HIGH:
          this.gSecurityButton.setAttribute("level", "high");
          break;
        case STATE_IS_SECURE | STATE_SECURE_LOW:
          this.gSecurityButton.setAttribute("level", "low");
          break;
        case STATE_IS_BROKEN:
          this.gSecurityButton.setAttribute("level", "broken");
          break;
        default:
          this.gSecurityButton.removeAttribute("level");
          break;
      }
    }
  }

Note that no ambiguous names lie within the body of the with statement, so the
usual caveat about with does not apply.  Whether someone might add such a name
in the future, I can't say.  Thought I'd mention this to avoid the overlong case
labels.

/be

    
    

    
  
Brendan should know better.

const wpl = Components.interfaces.nsIWebProgressListener;
switch(aState) {
    case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH:
      // ...
}

No with, hold the line!

    
    

    
  
cc david drinan.
Please review this patch on behalf of PSM.


    
    

    
  
I'm not done yet, and I may end up making some large changes to this patch, so I
would hold off on a review until I request it.

    
    

    
  
I do not understand why we need the 'unTabbedMode'.
In my 'big patch' there is only the tabbed-mode.
'The handling of many things' is much easier, 
when you don't have to think about two different modes.


jag:
>There's another issue where we don't store the current state when we're not in
>tabbed mode, and then when we switch we "lose" the current state.

    
    

    
  
*** Bug 129863 has been marked as a duplicate of this bug. ***

    
    

    
  
The reason is that a performance hit was measured when you're in tabbed mode.
I'm not quite sure what was measured, so I was going to do some measuring myself
to see if we can start in "tabbed mode" (even with the tab strip hidden) like
you also suggested.

    
    

    
  
I tried the JavaScript Debugger's profiling and
according to it the JS in tabbrowser.xml does not
take too much time. Using mTabProgressListeners (which then calls
nsBrowserStatusHandler) add only very minor overhead.
onStateChange in mTabProgressListener is of course the
most expensive method because it does all the icon and
tab label stuff.


    
    

    
  
*** Bug 130489 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 130627 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 130488 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 131027 has been marked as a duplicate of this bug. ***

    
    

    
  


  

        Attachment #73132 -
        Attachment is obsolete: true

        Attachment #73133 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 74258 [details] [diff] [review]
Tooltips fixed, transition from non-tabbed to tabbed mode fixed

Whoops, that change to jar.mn should be removing the securityOverlay.dtd
instead. Fixed in my local tree.

    
    

    
  
David, Javi, is it correct what I say in paragraph 4?


Jag, comments to your patch:

1) All accesses to securityButton and securityUI should be protected with null
checks, in case security is not installed, or the current window has security
disabled.

2) Is it correct that you remove securityOverlay.xul from jar.mn?

3) I assume that for each tabbed window there is an instance of SecureBrowserUI.
Correct?

4) You moved the definition for the behaviour of the onSecurityChange function
away from secureBrowserUI. This function is now empty. Instead you introduced a
JS method that now contains the logic to set the different lock icon states.

I'd prefer if that logic stayed inside the security component, for the following
reasons:
- the security component is an optional package, and its logic should not be
contained in the general browser part.
- the logic should, if possible, stay inside the directory that the security
team owns, to have to ask less people if we want to change things.
- the XPFE code is browser dependent, and might not be contained in embedding
windows (not sure), but it should be general enough.
- embedders need a way to get notified on security changes, too, and I think
that's why there is the possibility to register the button with the
secureBrowserUI, to have it set the status updates.

In your new onSecurityChange JS function in nsBrowserStatusHandler, could you
just forward the call to getBrowser().securityUI, if it is non-null?

That would require that the reference to the lock icon button does not get
removed from secureBrowserUI.


    
    

    
  
>1) All accesses to securityButton and securityUI should be protected with null
>checks, in case security is not installed, or the current window has security
>disabled.

That is not completely correct. The security button is always in XXX, but I
should indeed null check securityUI before accessing it.

>2) Is it correct that you remove securityOverlay.xul from jar.mn?

Fixed.

>3) I assume that for each tabbed window there is an instance of SecureBrowserUI.
>Correct?

Correct, if by "tabbed window" you mean the tab itself. If you mean the window
that contains the tabs, then the answer is no, there is an instance of
SecureBrowserUI per tab (per <browser>).

>4) You moved the definition for the behaviour of the onSecurityChange function
>away from secureBrowserUI. This function is now empty. Instead you introduced a
>JS method that now contains the logic to set the different lock icon states.

-  /* Deprecated support for mSecurityButton */
-  if (mSecurityButton) {
-    NS_NAMED_LITERAL_STRING(level, "level");
-
-    if (state == (STATE_IS_SECURE|STATE_SECURE_HIGH)) {
-      res = mSecurityButton->SetAttribute(level, NS_LITERAL_STRING("high"));
-    } else if (state == (STATE_IS_SECURE|STATE_SECURE_LOW)) {
-      res = mSecurityButton->SetAttribute(level, NS_LITERAL_STRING("low"));
-    } else if (state == STATE_IS_BROKEN) {
-      res = mSecurityButton->SetAttribute(level, NS_LITERAL_STRING("broken"));
-    } else {
-      res = mSecurityButton->RemoveAttribute(level);
-    }
-  }

I take it you're talking about this section.

>I'd prefer if that logic stayed inside the security component, for the following
>reasons:
>- the security component is an optional package, and its logic should not be
>contained in the general browser part.

Actually, the "logic" here is updating the UI according to the security state
flags it gets. Those flags are defined on nsIWebProgressListener, and are not
dependent on whether a security module was installed or not. I think it's fine
to put this (minimal!) logic close to the UI (since not everyone might want to
have a security button, or update it in the same way) that uses it, since the
modularity is still guaranteed by the use of those abstract state flags.

>- the logic should, if possible, stay inside the directory that the security
>team owns, to have to ask less people if we want to change things.

Any change you make to the logic of this will affect other listeners to these
same security change notifications, any change there will need some form of
migration and talking to people outside the security group, I would think.

>- the XPFE code is browser dependent, and might not be contained in embedding
>windows (not sure), but it should be general enough.

Embedding windows will need to do their own thing, I think. If you also look at
the change to embedding/browser/webBrowser/nsWebBrowser.cpp, you'll see they're
not registering a button at all.

>- embedders need a way to get notified on security changes, too, and I think
>that's why there is the possibility to register the button with the
>secureBrowserUI, to have it set the status updates.

It would be much cleaner for them to listen to register themselves as an
nsIWebProgressListener (which they probably have already) and add some logic
similar to the one I put in nsBrowserStatusHandler, but with different actions
on each of the possible states.

>In your new onSecurityChange JS function in nsBrowserStatusHandler, could you
>just forward the call to getBrowser().securityUI, if it is non-null?
>
>That would require that the reference to the lock icon button does not get
>removed from secureBrowserUI.

True, but if I recall today's meeting correctly the plans were to remove the
button from the state management object, which is also why the comment reads
"deprecated support for mSecurityButton".

If you look at what this object should do, I think it should purely build state
based on the notification it gets, and send out notifications of its own that
others can listen to and do with what they want.

I will add the null checks on securityUI (good call) and attach a new patch
(including the fixed jar.mn, which I fixed after you pointed it out online).

    
    

    
  
"That is not completely correct. The security button is always in XXX, but I
should indeed null check securityUI before accessing it."

XXX here is navigator.xul.

    
    

    
  


  

        Attachment #74258 -
        Attachment is obsolete: true

    
    

    
  
Jag, you convinced me, thanks for your comments and new patch, and I'm ready to
give you r=.

However, I asked David to have a look at our comments, to make sure the changes
to nsISecureBrowserUI are ok with him, too.


    
    

    
  
The patch looks good to me.

    
    

    
  
Comment on attachment 74290 [details] [diff] [review]
null check securityUI, fix jar.mn

hyatt sez sr=him

        Attachment #74290 -
        Flags: superreview+

    
    

    
  
Comment on attachment 74290 [details] [diff] [review]
null check securityUI, fix jar.mn

r=sgehani

        Attachment #74290 -
        Flags: review+

    
    

    
  
Comment on attachment 74290 [details] [diff] [review]
null check securityUI, fix jar.mn

sr=sspitzer for the mailnews parts.

    
    

    
  
Comment on attachment 74290 [details] [diff] [review]
null check securityUI, fix jar.mn

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

        Attachment #74290 -
        Flags: approval+

    
    

    
  
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
    

    
  
linux x86 2002-03-20-21

Confirmed that bug is fixed.  Good work!

    
    

    
  
Marking verified as per comment 156 and my own testing. Great Stuff!
Status: RESOLVED → VERIFIED

    
    

    
  
...and looks good on mac 10.1.3 using 2002.03.21.08 comm bits.

    
    

    
  
*** Bug 132829 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 133287 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 133172 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 133599 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 133985 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 134116 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 134218 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 134383 has been marked as a duplicate of this bug. ***

    
    

    
  
I'm removing this release note item for this bug from the Mozilla 1.0 and
future versions of the release notes because this bug is marked fixed.

Mail me if you think this item should be re-added.

    
    

    
  
It's still not working on 0.9.9 under Win2K Pro. What build do you have it 
working with?

    
    

    
  
*** Bug 135661 has been marked as a duplicate of this bug. ***

    
    

    
  
this fix was landed *after* 0.9.9

you need to use a nightly to have this bug patched (or wait untill 1.0)

    
    

    
  
*** Bug 136383 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 136396 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 136653 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 135661 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 137150 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 137407 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 137421 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 139013 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 140450 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 140771 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 141924 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 143416 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 163223 has been marked as a duplicate of this bug. ***

    
    

    
  
*** Bug 163698 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: