Closed Bug 283201 Opened 20 years ago Closed 20 years ago

Entering/leaving security warnings when staying on https

Categories

(Core Graveyard :: Security: UI, defect)

Other Branch
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dveditz, Assigned: darin.moz)

References

Details

(Keywords: fixed-aviary1.0.1, fixed1.7.6, regression, Whiteboard: [sg:fix])

Attachments

(2 files)

Regression from bug 258048, surfing from secure site to secure site (e.g. click
the "next" link in a list of bugzilla bugs) you get the warning dialogs for
leaving/entering secure state. Rather than reopen ancient bug 154084 I'm filing
this to address the regression as a clean state.
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1+
Whiteboard: [sg:fix]
I cannot reproduce this bug on the trunk.  It only seems to happen on the branch.
nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState
if (!channel)
  mNewToplevelSecurityState = nsIWebProgressListener::STATE_IS_INSECURE;

That's switching the state and causing the additional transition warnings.
But we must change the root of the problem.

The OnLocationChange method is being called with additional request objects.
I have no idea what kind of requests are being passed in there,
but obviously, it must be items that don't have a channel.

I propose we re-work the patch in bug 258048.

Before the patch, calling OnLocationChange only triggered state changes for
wyciwig requests.

Instead of doing it for everything, let's do it only if: it's wyciwyg or it has
a channel.

So, I propose to revert the patch in bug 258048, and apply the patch I will
attach in a minute.
Attached patch proposal v1Splinter Review
The patch looks big, but all it does is:

- revert 258048
- add the following small change:
--- src/nsSecureBrowserUIImpl.cpp-before-2580248        2005-02-23
00:09:44.601874208 +0100
+++ src/nsSecureBrowserUIImpl.cpp       2005-02-23 00:09:50.256014648 +0100
@@ -1138,9 +1138,12 @@
     mIsViewSource = vs;
   }

+  nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
+  PRBool hasChannel = !!channel;
+
   mCurrentURI = aLocation;

-  if (isWyciwyg) {
+  if (isWyciwyg || hasChannel) {
     nsCOMPtr<nsIDOMWindow> windowForProgress;
     aWebProgress->GetDOMWindow(getter_AddRefs(windowForProgress));
Kaie:

Welcome back, and thanks for the patch!!!

A couple quick questions:

 * Why wouldn't yciwyg requests have a channel?

 * Why doesn't this bug show up on the trunk?  (Is it that we sometimes don't
have a channel on the branch, but always do on the trunk?  What is your theory
about that?)
Status: NEW → ASSIGNED
If aRequest is null for a URI that should have a channel, then we have a pretty
big problem.  I think we need to ensure that it is non-null in those cases.
Ok, It appears that I simply failed to merge part of jst's patch for bug 277564:

-    FireOnLocationChange(this, nsnull, aURI);
-
-    return NS_OK; 
+    FireOnLocationChange(this, aRequest, aURI);
 }

On the branches, nsnull is still passed :-(

I'm updating my 1.0.1 firefox build to verify that fixing that fixes this bug as
well as bug 283226, which is most likely a duplicate.
Attached patch v2 patchSplinter Review
Ok, it turns out that I was right.  I just failed to merge jst's patch
properly.  Here's the missing piece from nsDocShell::SetCurrentURI.  I verified
that this patch fixes this bug as well as the other.
Attachment #175241 - Flags: superreview?(jst)
Attachment #175241 - Flags: review?(jst)
*** Bug 283226 has been marked as a duplicate of this bug. ***
Comment on attachment 175241 [details] [diff] [review]
v2 patch

Yeah, that matches the trunk now. Good catch! r+sr=jst
Attachment #175241 - Flags: superreview?(jst)
Attachment #175241 - Flags: superreview+
Attachment #175241 - Flags: review?(jst)
Attachment #175241 - Flags: review+
Comment on attachment 175241 [details] [diff] [review]
v2 patch

a=asa for branch fixing.
Attachment #175241 - Flags: approval-aviary1.0.1+
Comment on attachment 175241 [details] [diff] [review]
v2 patch

and 1.7.6 too.
Attachment #175241 - Flags: approval1.7.6+
fixed1.7.6, fixed-aviary1.0.1
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Verified Fixed with 2/23 builds for Aviary 1.0.1, Mozilla 1.7.6, and both
Trunks.   Going from mozilla.org to bugzilla only displays the security dialog
once (with the security.warn_* prefs set to true).  Going through a list of bugs
no longer pops up the dialog each time you go to a new one.
Status: RESOLVED → VERIFIED
Group: security
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: