Closed Bug 101723 Opened 23 years ago Closed 23 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: 23 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: