Paul Grimshaw Reporter
	Description • 18 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1) Gecko/20061010 Firefox/2.0

When you visit a site that has not got a valid certificate and the padlock in the address bar is open and "Contains Unauthenticated Content". If you go somewhere else and then click back, the bar is now yellow and locked padlock suggesting site ok. This is incorrect.

Reproducible: Always

Steps to Reproduce:
1. Go to https://www.madmash.com/ - Unlocked Padlock in Bar
2. Go to http://news.bbc.co.uk or whatever
3. Click BACK button, Locked padlock in bar.

Actual Results:  
Padlock locked.

Expected Results:  
Padlock still unlocked.

	Daniel Veditz [:dveditz]
	Comment 1 • 18 years ago

I don't get an "open" padlock, I get a slashed one indicating "mixed" content. When i go to an SSL site and then hit back I get an initial padlock that switches to mixed content when some of the non-SSL images render.

Non-SSL sites have no padlock at all, there should never be an "unlocked" icon ever in Firefox.

	Paul Grimshaw Reporter
	Comment 2 • 18 years ago

apologies - dveditz@cruzio.com is correct.

	Gervase Markham [:gerv]
	Comment 3 • 18 years ago

Marking INVALID based on comment #2.

Gerv

	Daniel Veditz [:dveditz]
	Comment 4 • 18 years ago

whoa, pardner! We clarified some terminology, but both the reporter and I agree on the symptoms, and the symptoms are a problem:

1) on a mixed content site (we have plans to kill this state)
2) go elsewhere, hit back
3) same content as 1, but no longer showing the "mixed" state.

I'm fairly sure this is because we switch into mixed state based on loading insecure content. When we hit back, however, we now load the content from the bfcache -- we don't "load" the insecure content because we already loaded and incorporated it into the saved (cached) content.

Not sure whether to blame security UI or the bfcache code (we should save the security state in the cache, probably). Another approach is to move forward with the eventual plan to kill off mixed content, and then this case doesn't even come up.

I don't think either fix is realistic for 1.8.1.1

	Kai Engert (:KaiE:)
	Comment 5 • 18 years ago

Confirming. Dan, thanks for your analysis.

	Kai Engert (:KaiE:)
	Comment 6 • 18 years ago

(In reply to comment #4)
> Not sure whether to blame security UI or the bfcache code (we should save the
> security state in the cache, probably). 

This is a regression that was introduced by changes outside of the security UI code.
In my testing, the lock icon tracking code does no longer receive events about loading the inner content of the page. None for JS, none for CSS.

As you raise the blame question: I'd blame the change that introduced new document loading, without ensuring that the web progress notification messages still worked as they were before.


> Another approach is to move forward
> with the eventual plan to kill off mixed content, and then this case doesn't
> even come up.

This will not be sufficient. Even in that scenario we'll need to be able to detect that insecure content and block / hide it.

	Kai Engert (:KaiE:)
	Comment 8 • 18 years ago

For testing I used:
- go to https://kuix.de/misc/test17/index3.php
- go to about:blank
- go back

I used NSPR_LOG_MODULES="nsSecureBrowserUI:5" to capture the events we get on initial and second loading of that page.

From the attached files you can see, the second time the security UI tracking code gets much less tracking information.

We used to get the same amount of notifications in both scenarios.

	Kai Engert (:KaiE:)
	Comment 9 • 18 years ago

This regression probably has been caused by changed around bug 292971.

With the new caching, and the new lack of progress notifications, it appears, we never go into the mixed-mode state when navigating through history.

Looking at the checkins made to file nsSecureBrowserUI.h, you had initially attempted to store the security state and restore the state when navigating. But you backed it out without any solution. Was this an oversight?

I suspect it was not an oversight. In bug 292971 comment 8 Brian said:
> Reverted the changes to the PasswordManager and SecureBrowserUI classes that
> were needed to account for lack of WebProgressListener notifications.

Did you believe the notifications are completely working?

While we get some notifications for the topmost document, we do not seem to get any events for the inner contents, which would be necessary to detect the mixed state.

	Daniel Veditz [:dveditz]
	Comment 10 • 18 years ago

raising severity to moderate, malicious injected code could easily use this to hide the fact that it's insecure.
 1 - load page in "mixed" state due to evil code
 2 - evil code loads a page under its control (either its own
     or any random site with an XSS flaw)
 3 - the controlled page issues a back()
 4 - profit! (original hacked page now looks secure)

Who owns bfcache now?

	Kai Engert (:KaiE:)
	Comment 11 • 18 years ago

In this patch you added nsSecureBrowserUIImpl::CaptureState and TransitionToState to SecureBrowserUI.h:
https://bugzilla.mozilla.org/attachment.cgi?id=182539&action=edit
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=bryner%25brianryner.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-05-04+00%3A00&maxdate=2005-05-04+23%3A59&cvsroot=%2Fcvsroot

But in this patch, you backed it out:
https://bugzilla.mozilla.org/attachment.cgi?id=186170&action=edit
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=bryner%25brianryner.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-15+00%3A00&maxdate=2005-06-15+23%3A59&cvsroot=%2Fcvsroot

	Kai Engert (:KaiE:)
	Comment 12 • 17 years ago

I think it's unfortunate this functional regression in security has never been addressed. The original code should have been backed out.

I was reminded about this bug when I heard about new bug 390168.

As of today, I can not reproduce this one. But it seems likely to be hidden by the new bug 390168.

	Kai Engert (:KaiE:)
	Comment 13 • 17 years ago

let's re-test this bug after the new regression bug 390168 got fixed.

	Kai Engert (:KaiE:)
	Comment 14 • 17 years ago

I found a simpler test which proves we still have this bug.

- go to https://kuix.de/misc/test17/insecure-script-and-css.php

- notice mixed lock icon
  (why does Firefox show a yellow bar? SeaMonkey does not, which is better)

- go to some other page, click the link

- click back

You get a secure lock icon.
Bug is still confirmed.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 17 years ago

Seems pointless to do EV if we're going to have bugs like this that let sites circumvent it...

	Damon Sicore (:damons)
	Comment 17 • 17 years ago

Marking as blocking.  Can still reproduce this on trunk.

	Brandon Sterne (:bsterne)
	Comment 18 • 17 years ago

Still reproducible on trunk.  Have we been able to make any progress toward a fix?

	Johnathan Nightingale [:johnath]
	Comment 19 • 17 years ago

Kai, any chance that 402574 offers any insight here?  It's certainly pretty undesirable behaviour.  :)

	Kai Engert (:KaiE:)
	Comment 20 • 17 years ago

(In reply to comment #19)
> Kai, any chance that 402574 offers any insight here?

No. Bug 402574 was a trivial bug in state tracking.

But this bug/regression exists, because the networking backend no longer sends the notification the security tracking relies on.

See comment 8 and comment 9.

	Kai Engert (:KaiE:)
	Comment 21 • 17 years ago

I'm replacing the old test
  ( https://kuix.de/misc/test17/insecure-script-and-css.php )
with a more impressive one.

Go to https://kuix.de/misc/test17/358438.php

For a short second the page is shown with mixed content.
Then it goes to fully secure.

	Kai Engert (:KaiE:)
	Comment 22 • 17 years ago

I notice, when going back, the web progress notification contains a new flag, STATE_RESTORING.

Is there any way PSM could stick the "mixed content" information into the cache?

Or alternatively, is there any way that PSM could iterate over the element objects that are associated to the cached page, and extract security information for each of them, and go to mixed mode if that fails?

(Well, the second proposal might not work, because we'd fail to extract security information for images, probably. Because we don't detect insecure images on page loading, going back would report more pages (correctly) as having a mixed content.)

	Adam Barth
	Comment 23 • 17 years ago

If you see mixed content for a given origin, you might as well continue to show the mixed content indication because if an attacker substitutes a malicious script in place of the insecure script, he can script/control any page in that origin.  See Bug 403590.

If you remember the mixed content information per origin, there isn't any need to iterate over the objects again, and back/forward will work correctly.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 17 years ago

> Is there any way PSM could stick the "mixed content" information into the
> cache?

Do you have a cache entry descriptor?  You can store arbitrary metadata on those...

Alternately, could you store something on the security info?

	Kai Engert (:KaiE:)
	Comment 25 • 17 years ago

I still haven't been able to focus on this bug.

I'm adding Honza to the CC list, who has recently started to work on PSM and told me he is looking for priority bugs he could help with. I discussed this bug with him, and he is willing to give it a try.

Honza, this might be tricky to solve, so thanks a lot for your attempt!

Answering Boris' comment 24, I'm not aware of cache entry descriptors (to the bfcache), no idea where I could get them from.

You also asked whether I could store something in the security info. Do you think the security info gets serialized into the bfcache? I assume it does not. Well, at least the combined state gets calculated by nsSecureBrowserUIImpl each time you go backward/forward.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 17 years ago

The security info hangs off the document channel, doesn't it?

	Honza Bambas (:mayhemer) Assignee
	Comment 27 • 17 years ago

Bad news: I cannot read the cache entry when the page is being restored from bfcache because the cache entry inside the channel was already closed and released. It is loaded (and available) only when the channel is open for network access between OnStart and OnStopRequest. But during fast back load the channel is passed just in inactive form for compatibility. See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp#5358 

I can load the cache entry directly from the cache session (but it is IMHO little bit misplaced to be called from the security UI).

Other way is to cache this inside of the security UI mapped to the URI and hold this during life time of the session - same as bfcache life time is. It is probably better way then storing this to the cache entry that is (even more) persistent among sessions.

BTW: I would like to ask why the particular notification for subcontent were actually removed? Is there any concrete reference why this were done?

FYI: This bug is irreproducible with Firebug 1.1.0b10. From the security UI logs it seems the bfcache is turned off when Firebug is enabled although they claim bfcache is untouched: http://www.getfirebug.com/relnotes.html, Beta 9, #2. I will check this with Firebug developers.

FYI2: This bug is irreproducible with this URL: https://www1.mail.volny.cz/
I will check why, maybe it can help us.

For now I am going to dive deeper to the bfcache code and see if we can get some notification that will not break anything but give the complete information.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 17 years ago

> Other way is to cache this inside of the security UI

Or in the channel's securityinfo, right?

> I would like to ask why the particular notification for subcontent were
> actually removed?

Because the whole point of bfcache is that the subcontent is not actually loaded again.  Which means there is nothing to send the notifications...

> see if we can get some notification that will not break anything

That would be much much more difficult than caching the "final" security state in the security info, I suspect.

	Honza Bambas (:mayhemer) Assignee
	Comment 29 • 17 years ago

This is first working draft. I changed nsITransportSecurityInfo interface to make the securityState attribute mutable. I override the value when notifying UI and next time on reload this new "final" value is read from there. 

I am not sure this is the right way, but it seems to me this is the most correct way to store the info, anyway.

Also I am not sure where to throw away (and if even to) reference to a top level security info. I have tested there is no leak when reference is released just this way.

Tested with test cases at comment 0, comment 8 and comment 21.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 17 years ago

I don't really know most of this code well enough to review.

I do think that the public API method should only allow downgrading the security state, as a safety precaution.  Right now it seems to also allow upgrading it.

	Daniel Veditz [:dveditz]
	Comment 31 • 17 years ago

not limited to 1.8 branch

	Honza Bambas (:mayhemer) Assignee
	Comment 32 • 17 years ago

I added check to allow only downgrade.

- When state is SECURED we may set only *exactly same strength* of SECURED. Other options are BROKEN or INSECURE, security strength bits will then be discarded.
- When state is BROKEN then just BROKEN and INSECURE might be set. Security strength bits will be discarded.
- When state is INSECURE then just INSECURE might be set. Security strength bits will be discarded.

	Honza Bambas (:mayhemer) Assignee
	Comment 33 • 17 years ago

Maybe more human-readable logic of the restriction code.

	Paul F Markfort
	Comment 34 • 17 years ago

I think you are on the right path.

Your logic seems simple and straight forward.

However; it looks like you didn't discard the security bits for:
case nsIWebProgressListener::STATE_IS_INSECURE:
(unless that isn't needed or is already handled in other code)

	Honza Bambas (:mayhemer) Assignee
	Comment 35 • 17 years ago

(In reply to comment #34)
> I think you are on the right path.
> 
> Your logic seems simple and straight forward.
> 
> However; it looks like you didn't discard the security bits for:
> case nsIWebProgressListener::STATE_IS_INSECURE:
> (unless that isn't needed or is already handled in other code)
> 

I expect that in that case there are already no level flags set. The only place the flags could be set is through SetSecurityState module-only public method. If we really want to be clear and do not trust our self :) then the check should additionally be made right there.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 17 years ago

If it's guaranteed to be true, it's probably worth an assertion....

	Kai Engert (:KaiE:)
	Comment 38 • 17 years ago

Comment on attachment 310045 [details] [diff] [review]
Final fix 1.2

Honza, good work!

I must confess, it took me quite a while to understand WHY exactly this patch is working.

I'm learning:
- you rely on the fact that the channels associated to the load requests store the original security state bits. I guess this works because the cache serializes the state, I remember such code got added last year.

- instead of storing a downgraded state only in the temporary state of nsSecureBrowserUI, you permanently store a downgraded state for the toplevel document in the channel.


I have several comments to improve the patch, in the hope it still works as intended.

I notice that fix v1.2 causes us to call the downgrade function many times. I think we should limit it to contexts where a downgrade is possible. (We should not call it each time we get a location change notification for a page element.)


I also propose some more comments, the use of locks for the member variables, and to simplify the downgrade function itself.


>Index: minimo/components/ssl/nsNativeSSL.cpp
> 
>+NS_IMETHODIMP
>+nsWINCESSLSocketInfo::DowngradeSecurityState(PRUint32 aNewStatus)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}

It seems that your implementation of DowngradeSecurityState depends only on
the parameter and on member variable mSecurityState, which is available
in minimo, too. So for completeness, once we're done, we should consider
to simply duplicate that code in the minimo sources, too.


>Index: netwerk/socket/base/nsITransportSecurityInfo.idl
>===================================================================
> 
>-[scriptable, uuid(0d0a6b62-d4a9-402e-a197-6bc6e358fec9)]
>+[scriptable, uuid(F0B4F0A9-02F9-41d8-9658-88814EA819A3)]
> interface nsITransportSecurityInfo : nsISupports {
>     readonly attribute unsigned long    securityState;
>     readonly attribute wstring          shortSecurityDescription;
>     readonly attribute wstring          errorMessage;
>+
>+    void downgradeSecurityState(in unsigned long aNewState);
> };


I think the function to downgrade the state must be able to update all state members, including error message and description.

With the test case, and using "mouse over" the broken lock icon, I still get the message "authenticated by <CA>". When we downgrade to mixed state, we should update the information strings to the usual "contains unauthenticated content" popup message.

We could postpone this fix to a later time, however, we should make sure we at least allow the interface to support it. I propose we allow 3 parameters to the function, and allow "zero length strings" to indicate "keep strings unchanged" (for now)


>+  PRUint32 securityState = mSecurityState & securityStateMask;
>+  PRUint32 newState = aNewState & securityStateMask;
>+
>+  switch (securityState)
>+  {
>+    case nsIWebProgressListener::STATE_IS_SECURE:
>+      if (newState == nsIWebProgressListener::STATE_IS_SECURE) {

Now you know that both securityState and newState are equal to STATE_IS_SECURE, so the following check will never be true, right?


>+        if (aNewState != mSecurityState)
>+          return NS_ERROR_NOT_AVAILABLE;


I think we don't need to return a failure in this function, unless you know a good reason I propose to always return NS_OK.

	Kai Engert (:KaiE:)
	Comment 40 • 17 years ago

Comment on attachment 310321 [details] [diff] [review]
Patch v2

Honza, what do you think about my changes to your patch?

	Kai Engert (:KaiE:)
	Comment 41 • 17 years ago

I was curious whether the same fix works on the 1.8 branch in Firefox 2, too.
It was necessary to backport the patch.
It seems to work.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 42 • 17 years ago

(In reply to comment #38)
> - you rely on the fact that the channels associated to the load requests store
> the original security state bits. I guess this works because the cache
> serializes the state, I remember such code got added last year.

bfcache is not the same as the normal cache. also, before that serialization fix last year, we didn't store SSL pages on disk, and in memory we just kept the original securityState object, so modifications to it will be kept.

	Honza Bambas (:mayhemer) Assignee
	Comment 43 • 17 years ago

Comment on attachment 310321 [details] [diff] [review]
Patch v2

(In reply to comment #38)
> (From update of attachment 310045 [details] [diff] [review])
> - you rely on the fact that the channels associated to the load requests store
> the original security state bits. I guess this works because the cache
> serializes the state, I remember such code got added last year.
> 
> - instead of storing a downgraded state only in the temporary state of
> nsSecureBrowserUI, you permanently store a downgraded state for the toplevel
> document in the channel.
> 

Exactly. And with the bfcache assistance the channel is held and passed to callbacks during restore. Channel then keeps flags as long as bfcache holds the page (exactly what we need).

> I notice that fix v1.2 causes us to call the downgrade function many times. I
> think we should limit it to contexts where a downgrade is possible. (We should
> not call it each time we get a location change notification for a page
> element.)
> 

Agree but IMHO it is not a big performance problem.

> 
> I also propose some more comments, the use of locks for the member variables,
> and to simplify the downgrade function itself.
> 

I forget about any synchronization, right :)

> 
> >Index: minimo/components/ssl/nsNativeSSL.cpp
> > 
> >+NS_IMETHODIMP
> >+nsWINCESSLSocketInfo::DowngradeSecurityState(PRUint32 aNewStatus)
> >+{
> >+  return NS_ERROR_NOT_IMPLEMENTED;
> >+}
> 
> It seems that your implementation of DowngradeSecurityState depends only on
> the parameter and on member variable mSecurityState, which is available
> in minimo, too. So for completeness, once we're done, we should consider
> to simply duplicate that code in the minimo sources, too.
> 

Yes, agree.

> 
> >Index: netwerk/socket/base/nsITransportSecurityInfo.idl
> >===================================================================
> > 
> >-[scriptable, uuid(0d0a6b62-d4a9-402e-a197-6bc6e358fec9)]
> >+[scriptable, uuid(F0B4F0A9-02F9-41d8-9658-88814EA819A3)]
> > interface nsITransportSecurityInfo : nsISupports {
> >     readonly attribute unsigned long    securityState;
> >     readonly attribute wstring          shortSecurityDescription;
> >     readonly attribute wstring          errorMessage;
> >+
> >+    void downgradeSecurityState(in unsigned long aNewState);
> > };
> 
> 
> I think the function to downgrade the state must be able to update all state
> members, including error message and description.
> 
> With the test case, and using "mouse over" the broken lock icon, I still get
> the message "authenticated by <CA>". When we downgrade to mixed state, we
> should update the information strings to the usual "contains unauthenticated
> content" popup message.
> 

Thanks for this hit! It is very important. The problem isn't raising because we don't clear the message. It is build by nsSecureBrowserUIImpl::GetTooltipText that returns "SecurityButtonMixedContentTooltipText" text when state is exactly lis_mixed_security. 

BUT: on restore (when navigating back) the state is lis_broken_security because the top level document is 'insecure'. Then security state gets to lis_broken_security instead of lis_mixed_security, see nsSecureBrowserUIImpl::UpdateMyFlags, if (mNewToplevelSecurityState & STATE_IS_BROKEN) branch.

Thus, this whole patch and concept is probably completely wrong because we produce incorrect security state on restore.

Personally I tend to cache the state in nsSecureBrowserUIImpl using hash table. It is probably the only way we can do it correctly w/o any magic to fit into the current code, change API and so on.

Not sure other comments are relevant anymore...

> We could postpone this fix to a later time, however, we should make sure we at
> least allow the interface to support it. I propose we allow 3 parameters to the
> function, and allow "zero length strings" to indicate "keep strings unchanged"
> (for now)
> 
> 
> >+  PRUint32 securityState = mSecurityState & securityStateMask;
> >+  PRUint32 newState = aNewState & securityStateMask;
> >+
> >+  switch (securityState)
> >+  {
> >+    case nsIWebProgressListener::STATE_IS_SECURE:
> >+      if (newState == nsIWebProgressListener::STATE_IS_SECURE) {
> 
> Now you know that both securityState and newState are equal to STATE_IS_SECURE,
> so the following check will never be true, right?
> 

I don't want to allow change of security level flags. aNewState and mSecurityState might differ. Question is what to do in that case - ignore the difference or throw error.

However, downgrading the security level should probably also be allowed. When some parts of the page are loaded securely but with lower level (key < 90 bits) then the whole page info should be displayed with this level, shouldn't be? It's the same when some part (even a small picture) is not loaded from a secure site - we get BROKEN state then.

> 
> >+        if (aNewState != mSecurityState)
> >+          return NS_ERROR_NOT_AVAILABLE;
> 
> 
> I think we don't need to return a failure in this function, unless you know a
> good reason I propose to always return NS_OK.
> 

The change in DowngradeSecurityState you provide (which is more clear then my original way :)) allows set of security level flags even the IS_SECURE flag is not set. This IMO should not be allowed. Thus we should probably use mSecurityState = newState (the masked one) or better, call SetSecurityState(aNewState) instead.

	Honza Bambas (:mayhemer) Assignee
	Comment 44 • 17 years ago

I store the number of secure/non-secure/broken-secure sub-documents in the channel's security info using new interface dedicated to it. I also reflect the nsIWebProgressListener::STATE_RESTORING flag to really restore only when restoring and save only on full reload.

We might thing of a better place to store the numbers (currently in nsSecureBrowserUIImpl::UpdateSubrequestMembers). Should be stored on finish of page load but I am not able to find the correct place.

	Honza Bambas (:mayhemer) Assignee
	Comment 45 • 17 years ago

Same as the previous one but added missing interface.

	Kai Engert (:KaiE:)
	Comment 46 • 17 years ago

Comment on attachment 311621 [details] [diff] [review]
v3

Honza, thanks a lot, this patch was very inspiring, and you're on the right track towards the solution.

I'm sorry that it took me so long to come back to this bug, but reviewing this patch was not trivial, I wanted to make sure I really understand it.

Unfortunately, when I review complicated code, I often find that I better write the new myself, instead of describing proposed changes :-/ Sorry.

So, I'll again attach a new patch.

I'm giving you r- for this patch for two major reasons:

- I'm worried that your proposed use of mRestoring is dangerous.

I can't predict the order of calls on OnStateChange and onLocationChange, but you try to transport the "restoring" state info the other function.

I think we should save and restored the state at the same time where your code decides to save the state.

- Your proposed patch would be "expensive" at runtime. You proposed to update the counts for every element that gets loaded, which can be a lot, and we'd do 4 XPCOM calls for each such change. I think it's worth it to optimize and do it only when changing between pages.

	Kai Engert (:KaiE:)
	Comment 47 • 17 years ago

I want to demonstrate the steps that brought me to the new patch.
FYI, you produced your v3 patch while bug 420187 was checked in, but it was soon after backed out.
This required me to merge.
So, here is a version of v3 that still applies to the most recent trunk.

	Kai Engert (:KaiE:)
	Comment 48 • 17 years ago

So, bugzilla is unable to diff "v3" against "merged v3", but a tool like "tkdiff" should help you compare the files.

Now, this "merged v3 rearranged" is the same as "merged v3", but has the order of files changed. You can use it to compare it to the "v4" that I'll attach next.

	Kai Engert (:KaiE:)
	Comment 49 • 17 years ago

This is my new proposed fix.
However, I'll attach a version that is easier to review.

	Kai Engert (:KaiE:)
	Comment 50 • 17 years ago

Honza, what do you think?

	Honza Bambas (:mayhemer) Assignee
	Comment 51 • 17 years ago

Comment on attachment 314808 [details] [diff] [review]
patch v4, not showing whitespace changes

Yes, this is quit good! I tested this again will all STRs and it works perfectly. 

Please, just take a look at indenting in nsSecureBrowserUIImpl::OnStateChange.

	Kai Engert (:KaiE:)
	Comment 52 • 17 years ago

(In reply to comment #51)
> 
> Please, just take a look at indenting in nsSecureBrowserUIImpl::OnStateChange.

The whitespace fixes are already included in attachment 314807 [details] [diff] [review].

	Kai Engert (:KaiE:)
	Comment 53 • 17 years ago

The fix for this bug overlaps with the fix for bug 383369 (which is a better fix for bug 335801).

This patch is a merged version of bug 358438 attachment 314807 [details] [diff] [review] (patch v4) and bug 383369 attachment 315028 [details] [diff] [review] (patch v1).

In my testing, this merged patch fixes all of bug 335801, bug 383369, bug 358438 (this one). I've tested using the URLs listed in those bugs, which are:
http://kuix.de/misc/test335801/
http://kuix.de/misc/test383369/
https://kuix.de/misc/test17/358438.php

Now I'm going to work on an updated patch for bug 420187, which will overlap as well :-(

	Robert Relyea
	Comment 54 • 17 years ago

Comment on attachment 314808 [details] [diff] [review]
patch v4, not showing whitespace changes

r+ rrelyea

	Kai Engert (:KaiE:)
	Comment 55 • 17 years ago

I've checked in a merged version of the latest patch together with the fix for bug 420187.

Marking fixed.

Honza, thanks a lot for your help!

	Johnathan Nightingale [:johnath]
	Comment 56 • 16 years ago

This bug can be opened now, can it not?  The fix is now in every gecko version we support.

	Honza Bambas (:mayhemer) Assignee
	Comment 57 • 16 years ago

If we still support 1.8.1 then it's not landed there.

So far landed:
http://hg.mozilla.org/mozilla-central/rev/7b1bfc82b813
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b1bfc82b813
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-04-11+21%3A47&maxdate=2008-04-11+21%3A47&cvsroot=%2Fcvsroot

	Paul F Markfort
	Comment 58 • 15 years ago

Although this is fixed in FIREFOX.

My original bug report was for Seamonkey.

As of SeaMonkey 1.1.17 (Windows) - it is still not fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 59 • 15 years ago

Paul FM, see comment 57 (and check the Gecko version in your Seamonkey build).

	Paul F Markfort
	Comment 60 • 15 years ago

Gecko/20090605 Installed with Seamonkey 1.1.17 (zip file install).

I run Firefox on the same machine.


I just want to make sure that this bug fix gets into the next (currently alpha) version of Seamonkey.

	Boris Zbarsky [:bzbarsky]
	Comment 61 • 15 years ago

> Gecko/20090605

I really did mean the version, not the build date.  The part after "rv:".

> I just want to make sure that this bug fix gets into the next (currently alpha)

If it's shipping off of Gecko 1.9.0 or later (which I believe it is), it'll pick this fix up automatically.