283201 - Entering/leaving security warnings when staying on https

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Darin Fisher]

I cannot reproduce this bug on the trunk.  It only seems to happen on the branch.

Comment 2

[Kai Engert [:KaiE:]]

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.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: proposal v1

Comment 4

[Kai Engert [:KaiE:]]

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));

Comment 5

[Darin Fisher]

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?)

Comment 6

[Darin Fisher]

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.

Comment 7

[Darin Fisher]

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.

Comment 8

[Darin Fisher]

Attachment: v2 patch

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.

Comment 9

[Darin Fisher]

*** Bug 283226 has been marked as a duplicate of this bug. ***

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 175241 [details] [diff] [review]
v2 patch

Yeah, that matches the trunk now. Good catch! r+sr=jst

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 175241 [details] [diff] [review]
v2 patch

a=asa for branch fixing.

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 175241 [details] [diff] [review]
v2 patch

and 1.7.6 too.

Comment 13

[Darin Fisher]

fixed1.7.6, fixed-aviary1.0.1

Comment 14

[Jay Patel [:jay]]

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.