Closed Bug 57395 Opened 24 years ago Closed 24 years ago

Changing themes loses OnStateChange listener for nsSecureBrowserUIImpl

Categories

(Core Graveyard :: Skinability, defect, P1)

Product:
Core Graveyard
Component:
Skinability
Platform:
x86
Linux
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: paulkchen)

References

Details

Attachments

(5 files)

Patch to cause Init() routine to be called on a skin switch
1.84 KB, patch
Details | Diff | Splinter Review
Same patch but applied to nsBrowserInstance.cpp instead of nsChromeRegistry.cpp
1.70 KB, patch
Details | Diff | Splinter Review
corrected patch for nsBrowserInstance.cpp
1.61 KB, patch
Details | Diff | Splinter Review
Same as previous patch except for slight change in coding style
1.48 KB, patch
Details | Diff | Splinter Review
Patch that does not have problem with what's-related sidebar
1.48 KB, patch
Details | Diff | Splinter Review 
Buildid 2000101921 trunk.

Reproducible: Always
Steps to Reproduce:

1.  Go to secure site.  Lock goes into locked state
2.  Change skins (using View menu).  Lock stays in locked state
3.  Go to insecure site.  Lock stays in locked state.

Furthermore, the "You have requested an insecure document" popup does not appear
even though it is enabled.

And clicking on the lock icon does not bring up the security manager.

This is a somewhat serious security problem, in my opinion -- after changing
themes at a secure site all sites look like they are secure and there is no way
to bring up the security manager to verify that information.
Changing summary to be more relevant to the real problem with this bug.
Summary: lock icon stuck after changing themes → Changing themes at secure site makes all sites look secure
ccing people
Status: UNCONFIRMED → NEW
Ever confirmed: true
Sending skin switching bug to skinability, nominating for rtm
Assignee: hangas → ben
Component: Themes → Skinability
Keywords: rtm
QA Contact: pmac → blakeross
Severity: normal → major
Keywords: correctness
There's more to this bug than what has been described above.  If you start at an 
insecure site and change the skin, then all future sites visited in that window 
look insecure.  So to generalize, whenever you change skins, the state of the 
window (secure vs insecure) remains frozen at whatever state it was in when the 
skin-switch occured.

What appears to be happening is that nsSecureBrowserUIImpl::OnStateChange never 
gets called again after the skin-change occurs.  Now we need to figure out why.
The OnStateChange listener for nsSecureBrowserUIImpl is added in the Init 
routine of nsSecureBrowserUIImpl.  That routine is called when the window is 
initially created.  When the skin is changed, all the listeners are removed for 
the old docshell and then, one-by-one, reinserted for the new one.  But the 
nsSecureBrowserUIImpl::Init routine is never called again, so it's 
OnChangeHandler does not get reinstalled for the new skin.

Still investigating to see how to fix this.
Uh oh.  This would be bad.  A significant security issue worth investigating for
RTM.
Priority: P3 → P1
Whiteboard: [rtm need info]
On Windows, the lock *never* changes to the secure icon, even when I'm at 
secure sites.  Is this a separate {known} bug?
OK, I see that bug 57695 was just filed for that issue.
*** Bug 57695 has been marked as a duplicate of this bug. ***
Updating summary from

   Changing themes at secure site makes all sites look secure

to the more accurate

   Changing themes loses OnStateChange listener for nsSecureBrowserUIImpl
Summary: Changing themes at secure site makes all sites look secure → Changing themes loses OnStateChange listener for nsSecureBrowserUIImpl
*** Bug 57698 has been marked as a duplicate of this bug. ***
blakeross : which build are you testing for the secure sites. I test the daily
builds of netscape6 with PSM and the lock locks always , except some sites I
know of.
Can you list the sites that don't lock for you?

nitin, remember this happens only with a skin switch (or, for a different bug,
in the classic skin)

amazon.com, bofa.com, discovercard.com
I test mozilla branch and trunk builds.  I can't really reproduce this bug yet 
because of bug 57695, which means the icon will always be in the unlocked state 
anyways.  But 57695 doesn't seem to apply to Linux.
Maybe we could move the attachment code into nsBrowserInstance, and then when
reinitializeContentVariables gets called, the listener could be reattached.

Paul, over to the knowledgeable one ...
Assignee: ben → pchen
I have been experimenting with fixes and have some good results and some not so 
good results to report.  Specifically I made modifitions along the following 
lines:

1. The Init() method in nsSecureBrowserUIImpl.cpp currently takes two arguments 
-- a window and a button.  The only thing it does with the button argument is 
to initialize mSecurityButton.  I bracketed that initialization so that it is 
skipped if the botton argument is null

2. The RefreshSkins() method of nsChromeRegistry.cpp currently goes around a 
loop once for each dom window.  Inside that loop I added a call to the Init() 
method mentioned in 1 above, passing it the dom window for the first argument 
and a null button for the second.

With these two changes in place I have fixed the problem of the pop-up boxes not 
appearing (saying "you are submitting to an insecure site", etc) but not the 
problem of the lock icon state.  The initialization for establishing listeners 
for both the pop-up boxes at form-submit time and the lock-icon state at 
document-load time are done in the above-mentioned Init() routine.  Since the 
first part is working, I know that my experimental patch handled that 
re-initialization correctly.  But the initialization of the listener for the 
lock-icon state uses the window parameter that was passed to the Init() routine, 
so the dom window that I am passing from the call in RefreshSkins() is 
apparently not the correct value to pass.

Still investigating.
Ah, I think the problem has to do with the fact that I'm passing in a null 
button on the re-initialization.  It looks like I'm getting a new instance of 
the secureBrowserUI for which mSecurityButton is null.  This results in the 
OnStateChange listener exiting prematurely (without fixing the lock icon) when 
it detects a page-load-finished because of the code:

   if (!mSecurityButton)
     return res;

So if I'm right, all I need to do is figure out a good value to use for button 
when I call the Init() routine to reinitialize after the skin change.
Ummmm, Steve, the "good" value for the button, is the DOM element for that
security icon. In other words, get the appropriate document, and do a
GetElementByID("security-button"). That's the button you need to pass into the
init function. As Hyatt mentioned, we probably want to do this inside
nsBrowserInstance::reinitializeContentVariables() because we are in a better
context to get at that button. It's going to be tough to get at that DOM element
from inside RefreshSkins()
Here's a very interesting alternate way to fix the problem.  What we need is the 
ability to force the SetSecurityButton routine in SecurityUI.js to get recalled 
after a skin switch occurs.  That routine will call the Init() method in 
nsSecureBrowserUIImpl.cpp.  To prove that that will fix the problem, I modified 
the displayPageInfo routine in that same file as follows:

function displayPageInfo()
{
+   SetSecurityButton();
-   if (securityUI)
-     securityUI.displayPageInfoUI();
}

I'm not recommending this as a fix, but just as a vehicle for testing.  With 
this change, I can click on the lock icon after the skin-change occurs and that 
forces the SetSecurityButton to be recalled which in turn results in a call to 
nsSecureBrowserUIImpl::Init().  And after having clicked on the lock icon, it 
behaves properly after a skin switch -- i.e., it gets locked and unlocked at the 
appropriate times.

So now the question is what is the right way to force a call to the 
SetSecurityButtion routine at the time of the skin switch.

I have a patch that sort of works.  I say that because I believe that there 
might be another bug that is blocking this patch from working.

Specifically, with the patch I am now calling the sSecureBrowserUIImpl::Init() 
routine after changing the skins and passing it the correct window argument and 
the correct button argument.  So everything should work.  But I observe that the 
lock icon still does not change.

Here is what is happening.  After the skin changes, I am observing that the 
onStartLoad listener and onStopLoad listener is getting called twice when a page 
loads.  The onStartLoad listener in particular calls on checkProtocolContext 
which sets nsIsSecureDocument.  The first time it is called that variable is 
indeed being set to true when we go to an https site.  But then the second call 
is causing it to be false.  Then later in the onStopLoad listener, we test that 
variable and establish a closed-lock icon if it is true.  But as I just 
explained, the double-call to the onStartLoad listener erases the "true" setting 
of the variable so the lock is not getting locked.

I verified all this with the debugger and if I don't allow the second call to 
set the variable to false, the lock icon becomes closed.  Also I observe that if 
I do a reload of a page, then these double calls don't occur and the lock icon 
gets closed.

So I'm presenting my patch as the fix for the problem but realize that this fix 
is dependent on fixing the other problem -- namely the double calling of the 
listeners after the skins change occurs.  After I do some more testing and am 
able to demonstrate that bug conclusively, I will file a bug report on it and 
make this bug dependent on that one.
More information on this secondary bug.  I didn't file a separate bug report on 
it because I don't seem to be able to demonstrate it if I don't apply the patch 
that I presented above.

What's happening is that after I do the skin-switch and execute the code in my 
patch, I notice that there are spurious loads of http://xslt.alexa.com.  These 
loads seem to occur after each page is loaded.  And it is these spurious loads 
that are changing the value of nsIsSecureDocument and therefore causing the lock 
icon to not be set correctly.

I can demonstrate this by adding the following ad-hoc patch in the startload 
listener in nsSecureBrowserUIImpl.cpp to ignore startloads from this strange 
site.  With this ad-hoc patch in place, the lock icon appears to be working 
properly.

So what is this strange xslt.alexa.com site and why is it being accessed?

@@ -331,6 +331,13 @@
     if ((aProgressStateFlags & STATE_START) &&
         (aProgressStateFlags & STATE_IS_NETWORK))
     {
+
+char* host;
+loadingURI->GetHost(&host);
+if (PL_strcmp(host, "xslt.alexa.com") == 0) {
+  return 0;
+}
+
         // starting to load a webpage
         PR_FREEIF(mLastPSMStatus); mLastPSMStatus = nsnull;

Aha, the xslt.alexa.com site is for the what's-related panel in the sidebar.  
And I just happened to have that panel open.  If I close that panel, then no 
accesses are made to that site and my ad-hoc patch is not needed.

The one thing I haven't yet figured out is why do we have this problem with the 
what's-related site after doing a skin-switch but not before.

Wasn't the new what's related panel supposed to go away for both N6.0 branch and
Mozilla trunk?
Patching the chrome registry in this way is unacceptable.  I can't approve this
patch.

Other problems with this patch include:
(1) You register on all windows not just a browser window.
(2) The security button may not be in every window, so you're initializing the
security module with a null button for those other windows.
(3) This makes the assumption that there was a secure browser UI attached.  This
only happens to be the case for our browser windows I think.


No problem.  The identical patch can be applied to nsBrowserInstance.cpp 
instead.  I'm attaching the new patch.
Sorry, ignore patch to nsBrowserInstance.cpp -- it doesn't work.  Back to the 
drawing board.
OK, here comes the corrected patch.  Sorry for posting the previous one -- my 
testing of it was flawed so I thought it worked.
a=hyatt on the last patch.

There is yet another problem here.  Namely the routine in which we are adding 
this patch does not get called when clicking on a link -- it is called only when 
you enter a new url in the url bar.  See bug 58416 which I just filed on that 
problem.  So other things do not work correctly after a skin switch (such as the 
back button as described in bug 58416) until you enter a new url.  Same now is 
true for the lock icon after the patch described here is made.

So to completely fix this current bug, we should also fix bug 58416.
Depends on: 58416
For PDT's sake, I should give arguments as to why I think this bug is a 
pull-off-the-wire bug.  After the user does a skin switch, we will tell him that 
future sites that he visits are secure when they aren't and/or vice versa.  
Furthermore we will not give him the pop-up notifications that he expects when 
he submits data to an insecure site.  Both of these could have legal 
ramifications.
This has a A=, but not a R=. We need a reviewer to get to rtm+.
Whiteboard: [rtm need info] → [rtm need info] a=hyatt, NEED R=
I'm fully aware of that.  It's hard to find people on a Sunday, but I did reach 
dveditz and he's reviewing it right now.
Attaching a revised patch that incorporate comments received from dveditz.  No 
logic has changed since the previous patch  -- just a slight change in coding 
style to improve readability.
a=hyatt again

Regarding the patch that , I would really like to see us reuse the
window.AddEventListener() method for reinitalization when a theme changes. What
I mean by this is when we switch themes, we post an event to all window
eventListener with a different verb (like "theme-changed" or something).

We hook into the eventListener like this:

http://lxr.mozilla.org/seamonkey/source/netwerk/resources/content/securityUI.js#23

Is there a verb that we can use to receive notification that the theme changed
(hyatt, vidur, please comment)

If there is no such verb that we can use, I would will reluctantly give an r= on
this patch. In the future, I will rip out the eventListener code all together
and just have the security module be initialized in the nsBrowserInstance.
I have said this before.  I'll post it again.  This situation can occur outside 
of a theme switch.  If a window is taken to display: none and back, you'll be in 
the same boat.  You have to design these systems such that they can deal with a 
docshell being recreated.  I'm not going to create a special skin-switching 
event.

One solution that would deal with auto-re-initializing code would be to use XBL. 
 XBL fires bindingattached and bindingdetached events when the bindings get 
taken away and restored (either through a skin switch or for other reasons).
Whiteboard: [rtm need info] a=hyatt, NEED R= → [rtm+] a=hyatt, r=dougt
Filed tasked level bug 58468 to move start/init code to nsBrowserInstance.cpp.

Regarding the patch inline, I really would like to see a better solution than
blocking a paticular site.  Any idea why passing content instead of the parent
window does not work?  Hyatt could you help morse with this?
Whiteboard: [rtm+] a=hyatt, r=dougt → [rtm need info] a=hyatt, NEED R=
I'm retracting my a=.  The wrong DOM window is being passed into Init.  It 
should be the content window.  Call the GetContentWindow method of 
nsBrowserInstance and pass that window into the init method of the secure module 
instead.
Whiteboard: [rtm need info] a=hyatt, NEED R= → [rtm need info] NEED R= AND A=
Dave, I previously tried using the content instead of the parent window and it 
didn't work.  See my patch of 10/20/00 12:08.

I wasn't doing exactly what you just suggested but rather was using the content 
that was obtained from mDOMWindow->Get_content.  So now I tried to do it just 
like you indicated in your last comment, nameley:

  nsIDOMWindowInternal* content2;
  GetContentWindow(&content2);

and using content2.  But that also didn't work.  The only way I was able to get 
the listener to be called after the skin-switch was to use mDOMWindow.

Do you have any other suggestions?
I found the problem with my previous use of the content window.  I was using the 
content window not only when calling the Init() routine but also for the 
GetDocument() call.

So now I have yet another patch which I'm about to attach.  And this one doesn't 
have the negative interaction from the what's-related sidebar.  hyatt and dougt, 
please review.
a=hyatt (again).  :)

Just a question. Are we leaking the old secure browser UI object on the skin switch?
Paul, I don't know.  Dougt, can you answer Paul's question?
Whiteboard: [rtm need info] NEED R= AND A= → [rtm need info] a=hyatt, r=
r=dougt.
Whiteboard: [rtm need info] a=hyatt, r= → [rtm+] a=hyatt, r=dougt
Putting into candidate limbo.
Please check this into the trunk asap.
Anticipate that (time and quality permitting), we may soon double plus this bug
to land on the RTM branch.  Please update this bug with any regression info.
Fix has now been checked into trunk.
Whiteboard: [rtm+] a=hyatt, r=dougt → [rtm++] a=hyatt, r=dougt
rtm++, please checkin ASAP so we can build today.
I just tested this with the 2000110108 trunk build.

1) Went to a secure site.
2) Switched skins
3) Went to an insecure site (either following a link or clicking on bookmark or
typing the url in the urlbar).  In all cases, no alert.  But the lock icon
changed to unlocked.
4) Hit the back button.
5) Went to an insecure site again (any of the methods in item 3 or hitting the
forward button).  In this case I got an alert.

After that, everything seems to function correctly.

But there is still item 3 in which I went from a secure site to an insecure one
without an alert....
Fixed checked into branch.

There are still problems.  Namely bug 58416.  Also possibly the lack of alert 
noted above.  

So bug is not being closed.  It's now in pchen's hands.
But now it should go off the rtm radar?
Whiteboard: [rtm++] a=hyatt, r=dougt → [rtm++] a=hyatt, r=dougt[fixed in branch]
Keywords: correctness, rtm
Whiteboard: [rtm++] a=hyatt, r=dougt[fixed in branch]
If there are more problems, I please file NEW bugs. Closing out this bug as fixed.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Boris, if you do file a new bug report (and please do), reference this one as 
well as bug 58416.
Also please copy me on the new bug report (but do not assign it to me).
OK, I've opened bug 58898 on the lack of an alert.
QA Contact: blakeross → pmac
Unable to verify this bug on Linux yet because it's currenty broken
on switching themes. Will verify this bug later.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: