Bug 130949 (lockicon)

[meta] lock icon issue tracking bug

NEW
Unassigned

Status

()

defect
P3
normal
18 years ago
4 months ago

People

(Reporter: ssaux, Unassigned)

Tracking

(Depends on 7 bugs, Blocks 1 bug, {meta})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-m][psm-padlock])

Attachments

(6 obsolete attachments)

 
adding dependent bugs.
Depends on: 124688, 130794, 130946
Keywords: nsbeta1+
Priority: -- → P1
Target Milestone: --- → 2.2
cc Momoi and Arun
Depends on: 130394
Posted patch Patch (obsolete) — Splinter Review
This new code works good for me.
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.
adding patch review
Keywords: patch, review
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.

Attachment #75125 - Attachment is obsolete: true
I take my statement back, that I have a fix fox bug 130394 already. I will
comment there.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #75395 - Flags: needs-work+
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #75395 - Attachment is obsolete: true
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. 
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.
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
Attachment #75597 - Flags: review+
Comment on attachment 75597 [details] [diff] [review]
Patch

Patch looks good to me. d=ddrinan.
That should be r=ddrinan.
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;
+    }
+  }
Attachment #75597 - Attachment is obsolete: true
Attachment #76186 - Flags: superreview+
Attachment #76186 - Flags: review+
Attachment #76186 - Flags: approval+
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?
Attachment #76186 - Flags: needs-work+
>   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!
Attachment #76186 - Attachment is obsolete: true
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.
Attachment #76256 - Flags: superreview+
Attachment #76256 - Flags: review+
Attachment #76256 - Flags: approval+
Yeah, that's what I meant, my shortcut for SchemeIs was a little too short.

And yeah, I like the new GetTooltipText.
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.
Attachment #76256 - Flags: needs-work+
Posted patch Updated patch (obsolete) — Splinter Review
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?
Attachment #76256 - Attachment is obsolete: true
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.
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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.
> "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?
I filed bug 134977 for the doc loader behaviour causing unexpected progress
events to show up.
Comment on attachment 76286 [details] [diff] [review]
Updated patch

Obsoleting patch, because it has been checked in.
Attachment #76286 - Attachment is obsolete: true
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.
Status: RESOLVED → REOPENED
Depends on: 134977, 135007, 135178
Keywords: nsbeta1+, patch, review
Resolution: FIXED → ---
Depends on: 127210
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.
Depends on: 139522
I'm adding dependencies for two more bugs describing the current lock icon
behaviour.
Depends on: 140836, 140837
Depends on: 139948
Depends on: 144056
Depends on: 148610
Depends on: 150863
Depends on: 145730
Alias: lockicon
Keywords: meta
Depends on: 154084
Depends on: 154240
Depends on: 119969
Depends on: 128641
Depends on: 139730
Depends on: 155760
Depends on: 156758
Depends on: 177330
No longer depends on: 177330
Depends on: 184246
Depends on: 165301
Depends on: 191212
lowering severity, since this is a meta bug
Severity: critical → normal
Depends on: 196827
Mass reassign ssaux bugs to nobody
Assignee: ssaux → nobody
Status: REOPENED → NEW
Depends on: 255242
Depends on: 64367
Depends on: 258048
Depends on: 257308
Depends on: 253121
Depends on: 238566
Depends on: 268483
Depends on: 262689
Depends on: 253288
Depends on: 277564
Depends on: 278003
Depends on: 280461
Depends on: 276720
Depends on: 283733
Product: PSM → Core
Depends on: 286810
Depends on: 287637
Depends on: 219502
Depends on: 271194
Depends on: 300613
Depends on: 308496
Depends on: 305284
Depends on: 305282
Depends on: 307027
Depends on: 312363
Whiteboard: [kerh-m]
Depends on: 317380
Depends on: 319313
Depends on: 329869
Depends on: 333440
changing obsolete psm* target to --- (unspecified)
Target Milestone: psm2.2 → ---
Depends on: 254745
Depends on: 349209
Depends on: 358438
Depends on: 335801
Depends on: newlock
Depends on: 369503
QA Contact: junruh → ui
Depends on: 401317
Depends on: 383369
Depends on: 418354
Depends on: 418355
Depends on: 390321
Version: psm2.2 → 1.0 Branch
Depends on: 445773
Depends on: 450912
Depends on: 451420
Depends on: 455367
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.
Depends on: 482245, 506008, 62178, 492358, 337897
Depends on: 509409
Depends on: 552286
Depends on: 552789
Depends on: 558551
Whiteboard: [kerh-m] → [kerh-m][psm-padlock]
Depends on: 527733
Depends on: 707620
Depends on: 724247
Depends on: CVE-2012-0479
Depends on: 613935
Depends on: 947079
Depends on: 804974
Depends on: 1070967
Blocks: 1029832
Blocks: 63413
Component: Security: UI → Site Identity and Permission Panels
Flags: approval+
Product: Core → Firefox
Version: 1.0 Branch → unspecified
Priority: P1 → --
Summary: lock icon issue tracking bug → [meta] lock icon issue tracking bug
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.