130949 - (lockicon) [meta] lock icon issue tracking bug

Status: RESOLVED INACTIVE

(Firefox :: Site Identity, defect, P3)

Description

[Stephane Saux]

Comment 1

[Stephane Saux]

adding dependent bugs.

Comment 2

[Stephane Saux]

cc Momoi and Arun

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch

This new code works good for me.

Comment 4

[Kai Engert (:KaiE:)]

The patch I just attached fixes the following bugs for me:
- 124688
- 130394
- 130946

I tested that it also works fine with
- 125807

Regarding
- 130794
I have the impression it is not caused by the security lock icon code, but by
some general XUL/chrome problems on the SUN platform.

Comment 5

[Stephane Saux]

adding patch review

Comment 6

[Kai Engert (:KaiE:)]

While that patch fixes things for me, we can't use this patch yet, because it is
in conflict with the patch from bug 101723 (which is already approved for checkin).

Feel free to have a look at my patch, if you're interested. But it might be
better to wait until I have the patch that works together with the fix from 101723.

Comment 7

[Kai Engert (:KaiE:)]

I take my statement back, that I have a fix fox bug 130394 already. I will
comment there.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 75395 [details] [diff] [review]
Patch

This patch does not care correctly for pages, that use frames, and some frames
are https, some http.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch

Finally I found a simple solution for frame loading.
I saw that all progress events for frame document have the same state bits as
the real toplevel frameset document.
The solution is to look at the DOM window inside the web progress event, that
states for which window this progress is for. If that window is not our
toplevel window, the progress is treated as belonged it to a subdocument, even
if it has the toplevel document state flags set.

Comment 11

[John Unruh]

The latest test build from kaie appears to work perfectly. The mixed content 
page correctly shows a mixed lock icon now - https://pki.mcom.com/frames.html. I 
will search bugzilla for lock icon problems and test with those now. If I 
still see a problem, I will add a comment to the bug and mention it here also. 

Comment 12

[Kai Engert (:KaiE:)]

David, Terry, can you please review the patch?
It seems to work.
I have a lot of trace output code in the patch, but it should be only active in
debug mode.

Comment 13

[rpotts (gone)]

hey kai,
this patch looks good to me.  

my only suggestion is to use nsIURI::SchemeIs(...) rather than calling
nsIURI::GetSpec(...) and doing a strncmp(...).

-- rick

Comment 14

[rpotts (gone)]

Comment on attachment 75597 [details] [diff] [review]
Patch

sr=[e-mail]

Comment 15

[David P. Drinan]

Comment on attachment 75597 [details] [diff] [review]
Patch

Patch looks good to me. d=ddrinan.

Comment 16

[David P. Drinan]

That should be r=ddrinan.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Updated patch, minor change as suggested by Rick

I tested that it still works.
The new code is:

+  nsCAutoString scheme;
+  if (mCurrentURI)
+  {
+    mCurrentURI->GetScheme(scheme);
+
+    PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
+	    ("SecureUI:%p: OnLocationChange: scheme is: %s\n", this,
+	     scheme.get()));
+
+    if (!strcmp(scheme.get(), "view-source"))
+    {
+      mIsViewSource = PR_TRUE;
+    }
+  }

Comment 18

[jag (Peter Annema)]

Comment on attachment 76186 [details] [diff] [review]
Updated patch, minor change as suggested by Rick

>Index: mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,v
>retrieving revision 1.5
>diff -u -d -r1.5 nsSecureBrowserUIImpl.cpp
>--- mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp	20 Mar 2002 09:57:25 -0000	1.5
>+++ mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp	26 Mar 2002 12:49:43 -0000
>@@ -160,14 +161,52 @@
> NS_IMETHODIMP
> nsSecureBrowserUIImpl::GetTooltipText(nsAString& aText)
> {
>-  aText = mTooltipText;
>+  if (mInfoTooltip.Length())

  if (!mInfoTooltip.IsEmpty())

says better what you really mean.

>+  {
>+    aText = mInfoTooltip;
>+  }
>+  else
>+  {
>+    nsAutoString tooltiptext;
>+    GetBundleString(NS_LITERAL_STRING("SecurityButtonTooltipText").get(),
>+                    tooltiptext);

You could get this string straight into mInfoTooltip (and save having to look
it up again the next time someone tries to get the tooltip text) if you change
GetBundleString to take a nsAString for the out param.

>+
>+    aText = tooltiptext;
>+  }
>+
>   return NS_OK;
> }
> 
>@@ -408,6 +964,52 @@
>                                         nsIURI* aLocation)
> {
>   mCurrentURI = aLocation;
>+
>+#if 0
>+
>+  PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+         ("SecureUI:%p: OnLocationChange\n", this));
>+
>+  // I would like to understand why this never worked for view source events.
>+
>+  nsresult res;
>+  nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest, &res));
>+  if (channel)
>+  {
>+    PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+           ("SecureUI:%p: OnLocationChange: got channel\n", this));
>+    nsCOMPtr<nsIHttpChannel> httpRequest(do_QueryInterface(channel));
>+    if (httpRequest) {
>+      PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+             ("SecureUI:%p: OnLocationChange: got http\n", this));
>+
>+      nsCOMPtr<nsIViewSourceChannel> viewsource(do_QueryInterface(httpRequest));
>+      if (viewsource) {
>+        mIsViewSource = PR_TRUE;
>+
>+        PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+               ("SecureUI:%p: OnLocationChange: don't want view source requests\n", this));
>+        return NS_OK;
>+      }
>+    }
>+  }
>+#endif
>+
>+  nsCAutoString scheme;
>+  if (mCurrentURI)
>+  {
>+    mCurrentURI->GetScheme(scheme);
>+
>+    PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
>+           ("SecureUI:%p: OnLocationChange: scheme is: %s\n", this,
>+            scheme.get()));
>+
>+    if (!strcmp(scheme.get(), "view-source"))
>+    {
>+      mIsViewSource = PR_TRUE;
>+    }
>+  }
>+
>   return NS_OK;
> }
> 

So you can simplify this to:

  if (mCurrentURI)
  {
    mCurrentURI->SchemeIs("view-source", &mIsViewSource);
  }

At the "cost" of not doing that PR_LOG. If you need/want the PR_LOG, I would
put that in #ifdef:

  if (mCurrentURI)
  {
    mCurrentURI->SchemeIs("view-source", &mIsViewSource);

#ifdef DEBUG /*or whatever */
    nsCAutoString scheme;
    mCurrentURI->GetScheme(scheme);

    PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
	   ("SecureUI:%p: OnLocationChange: scheme is: %s\n", this,
	    scheme.get()));
#endif
  }

Note that even if you keep the old code, I would put the nsCAutoString
declaration inside the |if|.

Could it be the QI method doesn't work because a nsIViewSourceChannel wraps the
http channel instead of being an interface on the same object?

Comment 19

[Kai Engert (:KaiE:)]

>   if (!mInfoTooltip.IsEmpty())
> 
> says better what you really mean.

changed


>>+  {
>>+    nsAutoString tooltiptext;
>>+    GetBundleString(NS_LITERAL_STRING("SecurityButtonTooltipText").get(),
>>+                    tooltiptext);
>>+
>>+    aText = tooltiptext;
>>+  }
> 
> You could get this string straight into mInfoTooltip (and save having to look
> it up again the next time someone tries to get the tooltip text) if you change
> GetBundleString to take a nsAString for the out param.

Your suggestion has two parts.
I agree on the first, that we can avoid the extra temporary string by changing
the out parameter type. Done.

Your second suggestion is, I should only fetch the string once from the bundle,
and after that, keep it around in memory.
I disagree. Note that the contents of mInfoTooltip change dynamically.

mInfoTooltip only stores the dynamic security tooltip text, but it never stores
the default tooltip text.

If I want to reset the security tooltip text to "standard == insecure == what's
in the bundle", then I erase the contents of mInfoTooltip, and this will drive
the behaviour of GetTooltipText when it get's called back.

I have to look up the default string anyway whenever the security state changes.

Because, if I wanted to avoid looking up the default string again, I'd have to
use another variable that always caches the default string in memory, and I
wanted to save having to store that string in RAM for every open browser window,
while it is not needed.

My assumption is that nsSecureBrowserUIImpl::GetTooltipText will only be called
when the security state changes, and that should be seldom enough to justify not
to cache the default string in memory.

Ok, I could change both places, where mInfoTooltip is currently truncated to
zero length, and add the code that retrieves the default tooltip, but assuming
that GetTooltipText is really only called on state change, it shouldn't matter.


> So you can simplify this to:
> 
>   if (mCurrentURI)
>   {
>     mCurrentURI->SchemeIs("view-source", &mIsViewSource);
> 
>   [SNIP]
> 
> Note that even if you keep the old code, I would put the nsCAutoString
> declaration inside the |if|.

I understand your suggestion, but agree only partially.

Your code would always set mIsViewSource, it would set it even back to PR_FALSE,
even if it had been PR_TRUE before.

The current patch never changes it back to PR_FALSE. My plan was that once I see
a "view-source" in a window, I know that all security warnings must be prevented
in it.

This is necessary, because my testing showed that in view-source windows we also
receive file:/// requests for some style sheet, and that would change the
mIsViewSource back and turn the warnings back on.

But thanks for the suggestion to avoid copying the scheme and using the
shortcut, I therefore changed the code to:

  if (mCurrentURI)
  {
    PRBool vs;
    
    if (NS_SUCCEEDED(mCurrentURI->SchemeIs("view-source", vs)) && vs)
    {
      PR_LOG(gSecureDocLog, PR_LOG_DEBUG,
             ("SecureUI:%p: OnLocationChange: view-source\n", this));

      mIsViewSource = PR_TRUE;
    }
  }


> Could it be the QI method doesn't work because a nsIViewSourceChannel wraps the
> http channel instead of being an interface on the same object?

That sounds like a good explanation, thanks.
Now that we found the answer, I'll remove that dead
  #if 0
block from the code (that contained my question you just answered), because I
believe doing the simple string compare is cheaper than three QIs anyway.

Thanks!

Comment 20

[Kai Engert (:KaiE:)]

Attachment: New patch incorporating Jag's suggestions

Comment 21

[Kai Engert (:KaiE:)]

Comment on attachment 76256 [details] [diff] [review]
New patch incorporating Jag's suggestions

I'm feeling bold and are transfering r, sr and a to the latest patch.

Comment 22

[jag (Peter Annema)]

Yeah, that's what I meant, my shortcut for SchemeIs was a little too short.

And yeah, I like the new GetTooltipText.

Comment 23

[Kai Engert (:KaiE:)]

Oh well, I was feeling anxious, did more testing, and actually found two more
issues.

1.) Variable mIsViewSource never got initialized, and that led to random event
filtering.

2.) When going from https to ftp, the lock icon stayed closed. (But this had
always been that way, just tested with an old version.) I fixed it by doing a QI
to nsIFTPChannel in addition, and evaluating those progress events, too.

When I worked on 2.), I optimized the code.
The past version and my patch QIed to all interfaces all the time.
I optimized that to first QI to nsIHttpChannel. Only if that fails, we QI to the
next potential channel type, etc., and finally bail out, if we didn't find any
channel type of interest.
This will save us many QI calls.

Comment 24

[Kai Engert (:KaiE:)]

Attachment: Updated patch

Has the minor changes as described in previous comment:
- inits mIsViewSource
- adds testing for nsIFTPChannel as a relevant channel type for state tracking
- this required making security/manager/boot dependent on necko2

Now I'm unsure. Do we need another review / approval cycle?

Comment 25

[Kai Engert (:KaiE:)]

Patch checked in, and I'm tempted to say FIXED.

After I checked it in, I did some additional paranoia testing.
I found a strange behaviour that made me believe my patch could have another
bug. However, after doing some research, I think it seems to be an inconsistency
in document loading.

First, here is what I see.

Go to http://www.bouldernews.com/

That's a tricky page that Jud suggested me to look at.
I tested several links on that site, and if one is patient and waits until
everything has loaded, and only then is bold enough to click a link, everything
worked perfectly for me.

However, I did some testing, clicking on links, while page loading has not been
completed.

That's actually easy to do. 
Go to http://www.bouldernews.com/ 
Most content will load rather quickly.
But at the end, the progress bar in the lower right corner is still active, and
nothing seems to happen.
(If you wait 30 seconds, it will eventually load).

But try to not wait that long. I.e., while the page has mostly loaded, but the
progress is still "active", click on that area in the upper right corner that
says "subscribe".

When I do that, the new page will load, and I will see a MIXED security warning,
and the lock icon will indicate the broken state.

But that seems wrong, because:

If you don't click early, but wait until the home page has loaded, wait until no
more activity is shown, and then click on the "subscribe" area, the new page
will be reported as SECURE and the lock icon will be closed.

I traced what is happening here.

In both scenarios (click early or wait), the total number of start / stop events
matches.

Also, regardless whether you wait or click early, the internal tracking count of
the lock icon code reaches the "all zero" state, which means that in both
scenarious both page loadings are separate transactions.

But I saw, when you click the link early, the transaction for loading the
destination document is not limited to content of the destination document.

After the START event for the destination event was received, I saw additional
start events for other documents, one of them is:

http://adcenter.scripps.com/html.ng/site=BOU&size=120x90&sitesection=general&keyword=front&pagepos=4

I looked at the source that is transfered for the subscribe document, it does
not contain any reference to the above URL.

I looked at the source for the home page, and it indeed references that URL, it
is trying to load that in an <iframe>.

I would like to conclude by suspecting that there is a bug in the document
loader, that causes those <iframe> documents to get requested as part of the new
document loading, although it does not make sense that it does.

And that explains the mixed lock icon display in the "click early" scenario. The
tracking code recognizes that some http documents get loaded, while the toplevel
document is https.

Comment 26

[Kai Engert (:KaiE:)]

I think this is fixed, assuming the guess in my previous comment is correct, and
my obervations are really caused by a wrong behaviour in doc loader.

Comment 27

[Katsuhiko Momoi]

In comment #25, kaie wrote:

"I would like to conclude by suspecting that there is a bug in the document
loader, that causes those <iframe> documents to get requested as part of the new
document loading, although it does not make sense that it does."

Will you be filing a bug on this? We need to fix anthing that make it look 
like Mozilla's lock icon behavior is broken.

Comment 28

[Kai Engert (:KaiE:)]

> "I would like to conclude by suspecting that there is a bug in the document
> loader, that causes those <iframe> documents to get requested as part of the new
> document loading, although it does not make sense that it does."
> 
> Will you be filing a bug on this? We need to fix anthing that make it look 
> like Mozilla's lock icon behavior is broken.

I will, but I thought I mention it here first.
Rick, do you confirm what I described is indeed a bug?

Comment 29

[Kai Engert (:KaiE:)]

I filed bug 134977 for the doc loader behaviour causing unexpected progress
events to show up.

Comment 30

[Kai Engert (:KaiE:)]

Comment on attachment 76286 [details] [diff] [review]
Updated patch

Obsoleting patch, because it has been checked in.

Comment 31

[Kai Engert (:KaiE:)]

Reopening, because this is a tracking bug, and we can use it to track the other,
yet unresolved issues / enhancement requests.

Added bug 135007, bug 134977, and bug 135178 to the list of dependencies.

Removing keywords.

Comment 32

[Kai Engert (:KaiE:)]

We have a serious regression, see bug 139522.
My first impression is, the security code is helpless, because for some windows,
we do not receive any state change notifications.

Comment 33

[Kai Engert (:KaiE:)]

I'm adding dependencies for two more bugs describing the current lock icon
behaviour.

Comment 34

[Kai Engert (:KaiE:)]

lowering severity, since this is a meta bug

Comment 35

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 36

[Kai Engert (:KaiE:)]

changing obsolete psm* target to --- (unspecified)

Comment 37

[Honza Bambas (:mayhemer)]

Adding bugs 506008, 62178, 358438, 492358, 337897 to the dependency list, some of them are not directly related but touches the code of security state detection.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

closing as we haven't been using it for a while