Mads Kiilerich Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20110103 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20110103 Firefox/3.6.13

Entering the url http://rhodecode.org makes Firefox show a fine "This Connection is Untrusted" page. That doesn't make sense for a http url and will make people why try to understand what is going on even more confused.

What happens is that http://rhodecode.org redirects to https://rhodecode.org , so Firefox shows the content from https://rhodecode.org but still shows http://rhodecode.org as url. I would expect it to first fully redirect (and update the url field), then process the certificate stuff.

Reproducible: Always




Chromium behaves as I would expect.

	Tim (fmdeveloper)
	Comment 1 • 14 years ago

Can confirm issue on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13 ID:20101203075014 and trunk -> Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110105 Firefox/4.0b9pre ID:20110105030550

(Chrome and Safari update the location bar then show the error; IE and Opera act as Firefox currently does)

	John Drinkwater (:beta)
	Comment 2 • 14 years ago

I came to report this issue with the URL http://is.gd/iFMbSX redirecting to secure.youtipit.org but the URL isn’t updated.

Important bug imo. Could be used to trick a user to accept an invalid cert…

	John Drinkwater (:beta)
	Comment 3 • 14 years ago

Clicking on the security drop‐down (favicon) forces the URL and favicon to update to be correct.

	John Drinkwater (:beta)
	Comment 4 • 14 years ago

Feedback from sysKin in irc://irc.mozilla.org/firefox that Windows nightly does not exhibit this behaviour. Linux Minefield for today does.

	John Drinkwater (:beta)
	Comment 5 • 13 years ago

Seen again for http://ur1.ca/3iwi9 → https://campus.ftacademy.org/login/index.php

Can we get this bug escalated somewhat?

	Christopher Blizzard (:blizzard)
	Comment 6 • 13 years ago

Marked as security-sensitive until the security team gets to look at this.  They can mark as + if they can reproduce and think it's something we should track for Firefox 5.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 7 • 13 years ago

I can reproduce when loading http://ur1.ca/3iwi9 in a background tab (but not when loading it in a foreground tab).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 13 years ago

(In reply to comment #3)
> Clicking on the security drop‐down (favicon) forces the URL and favicon to
> update to be correct.

The relevant bit here is the call to gURLBar.handleRevert(); in handleIdentityButtonEvent. The problem is that userTypedValue is set to the URL that we are opening, and the load of the error page doesn't trigger an onLocationChange that causes us to update it. I thought we maybe had bugs on this already...

	Dão Gottwald [:dao]
	Comment 9 • 13 years ago

There's bug 311007

	O. Atsushi (Torisugari) Assignee
	Comment 10 • 13 years ago

http://goo.gl/cb1Rr
?

(In reply to comment #8)
> and the load of the error page doesn't trigger an onLocationChange
> that causes us to update it. I thought we maybe had bugs on this
> already...

1.
DocShell calls onLocationChange (twice, not intentionally IMO) BEFORE start loading <aboutCertError.xhtml>.

2.
DocShell does not call onLocationChange AFTER loading <aboutCertError.xhtml>, as a result of bug 514232.

In any case, docShell does not tell whether or not it is loading an error page or not explicitly.  And, bug 311007 would not fix this issue, directly. We do have a channel (with a bad cert).

	O. Atsushi (Torisugari) Assignee
	Comment 12 • 13 years ago

goo.gl/sajSx

The problem is that error pages do not use network.

On a normal page load, the final (STATE_STOP | STATE_NETWORK) is called after onLocationChange. On the other hand, on an error page, (STATE_STOP | STATE_NETWORK) is called before onLocationChange. As a result, when onLocationChange is called, |userTypedClear| is 0 and we can't clear |userTypedValue| in tabbrowser.xml

	O. Atsushi (Torisugari) Assignee
	Comment 13 • 13 years ago

I think |userTypedClear| is too complicated to maintain. How about switching to a boolean value from it?

	Justin Dolske [:Dolske]
	Comment 14 • 13 years ago

Oops! Didn't notice a patch had already appeared here, bouncing it over to the patch owner. :)

	O. Atsushi (Torisugari) Assignee
	Comment 15 • 13 years ago

Frankly speaking, I don't want to be assigned because what I'm suggesting is relatively a radical change, compared to the symptom of this bug. It will take a long time. I have no good idea how to cancel session-restore's deleting user-input, for example.

This is a security bug. The sooner the better, isn't it? I simply think the race-condition should be removed, in the long run...

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 16 • 13 years ago

Blair: this is similar to bug 568410.

	O. Atsushi (Torisugari) Assignee
	Comment 17 • 13 years ago

Removing |userTypedClear| completely.

I can't reproduce bug 231393 even without userTypedClear, we don't need an extra flag like the last patch.

	O. Atsushi (Torisugari) Assignee
	Comment 18 • 13 years ago

I can reproduce this bug with SeaMonkey. And this is a little related to bug 435932.

My plan is:
1. 
Remove userTypedClear.

2. 
Instead, introduce a new boolean isLoadingUserTyped, which is set true by browser.loadURIWithFlags(...), and set false by docshell's onLocationChange. The only user of this value is session-store, which re-sends a request to the userTypedValue's URI only when session-history do not remember userTypedValue's URI. I think this is lean-and-mean, for docshell handles session-history things at the same time firing onLocationChange. So either browser's userTypedValue or session-history-entry remembers that URI.

3.
docshell's onLocationChange always set null to userTypedValue.

  3.5
  Unless session-store is restoring the tab. This is a
  little dirty hack. I'm not too sure this is a good
  behavior; session-restore remembers  |userTypedValue|,
  while session-history's back()/forword() forgets
  |userTypedValue| .

neil, you have some ideas about browser.userTypedClear?

	neil@parkwaycc.co.uk
	Comment 19 • 13 years ago

There is a large comment in browser.xml explaining how userTypedClear improves the experience for users by preventing unrelated page loads from altering the URLbar while the user is typing a new URL into it. I don't see how your suggestion addresses this.

	neil@parkwaycc.co.uk
	Comment 20 • 13 years ago

This forces the location bar to update when an error page loads.

	O. Atsushi (Torisugari) Assignee
	Comment 21 • 13 years ago

(In reply to neil@parkwaycc.co.uk from comment #19)
> There is a large comment in browser.xml explaining how userTypedClear
> improves the experience for users by preventing unrelated page loads from
> altering the URLbar while the user is typing a new URL into it.

Can onLocationChange is called when the user is typing? In Case 1, 3, 4, we should reset URL bar anyway. "Case 2: Interrupted load" seems what I overlooked. And it also seems causing this bug. According to Case 2, we should not reset userTypedValue when user's request was interrupted by network errors including the bad-cert error. Well, I'm not sure what I'm talking about...

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>--- a/suite/browser/tabbrowser.xml
>+++ b/suite/browser/tabbrowser.xml
>@@ -577,8 +577,9 @@
>                   this.mTabBrowser.forwardBrowserGroup = [];
>                 }
> 
>-                // The document loaded correctly, clear the value if we should
>-                if (this.mBrowser.userTypedClear > 0)
>+                // Clear the value if this was a user document or an error page.
>+                if (this.mBrowser.userTypedClear > 0 ||
>+                    !Components.isSuccessCode(aRequest.status))
>                   this.mBrowser.userTypedValue = null;
>               }

We often calls onLocationChange where |aRequest| is null. I'll investigate nsIRequest::status really corresponds to network error pages.

	neil@parkwaycc.co.uk
	Comment 22 • 13 years ago

(In reply to O. Atsushi (Torisugari) from comment #21)
> Can onLocationChange is called when the user is typing?
Yes, for instance a page that automatically refreshes itself.

> We often calls onLocationChange where |aRequest| is null.
Sorry, the code is actually inside an if (aRequest && ...) block but I would have needed an extra 10 lines or so of context to show it.

	O. Atsushi (Torisugari) Assignee
	Comment 23 • 13 years ago

(In reply to neil@parkwaycc.co.uk from comment #22)
> (In reply to O. Atsushi (Torisugari) from comment #21)
> > Can onLocationChange is called when the user is typing?
> Yes, for instance a page that automatically refreshes itself.
In that case, the current version of Firefox refreshes URLbar as well. userTypedClear does not look useful.

> > We often calls onLocationChange where |aRequest| is null.
> Sorry, the code is actually inside an if (aRequest && ...) block but I would
> have needed an extra 10 lines or so of context to show it.
And I'm sorry too. Then it seems OK to me. If I were a reviewer, r+.

	neil@parkwaycc.co.uk
	Comment 24 • 13 years ago

(In reply to O. Atsushi (Torisugari) from comment #23)
> (In reply to comment #22)
> > (In reply to O. Atsushi (Torisugari) from comment #21)
> > > Can onLocationChange is called when the user is typing?
> > Yes, for instance a page that automatically refreshes itself.
> In that case, the current version of Firefox refreshes URLbar as well.
Well, it depends on whether you typed during the "active" zone in between the network start and the location change or not.

> If I were a reviewer, r+.
As SeaMonkey module owner, I can accept your r+.

	O. Atsushi (Torisugari) Assignee
	Comment 25 • 13 years ago

Comment on attachment 576901 [details] [diff] [review]
SeaMonkey workaround

Review of attachment 576901 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to neil@parkwaycc.co.uk from comment #24)
> Well, it depends on whether you typed during the "active" zone in between
> the network start and the location change or not.

Thanks, now I see the logic. But it seems difficult for me (I hope it's not just me), depending on web page's file size.

	Dão Gottwald [:dao]
	Comment 26 • 13 years ago

Comment on attachment 576409 [details] [diff] [review]
a new approach v2

It's not clear to me whether this still needs review or whether the SeaMonkey workaround applies to Firefox.

	O. Atsushi (Torisugari) Assignee
	Comment 27 • 13 years ago

In my opinion, Firefox should also apply neil's to close this bug.

Apart from this bug, I still think userTypedClear is evil, ... or a little too tricky. (e.g. bug 435932). And I'd like to know what you and Gavin think about userTypedClear.

	Dão Gottwald [:dao]
	Comment 28 • 13 years ago

I don't like userTypedClear and always forget how it works.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 29 • 13 years ago

(In reply to Dão Gottwald [:dao] from comment #28)
> I don't like userTypedClear and always forget how it works.

Same here. But I'm not sure there are any suitable simpler alternatives - I'd certainly be happy to consider any proposals, but that's probably best done in a new bug.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 30 • 13 years ago

Torisugari, can you create a Firefox version of Neil's SeaMonkey patch? It would be really excellent to get some test coverage for this, too.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 33 • 13 years ago

Comment on attachment 578297 [details] [diff] [review]
Firefox ver.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>                 // The document loaded correctly, clear the value if we should
>-                if (this.mBrowser.userTypedClear > 0)
>+                if (this.mBrowser.userTypedClear > 0 ||
>+                    (aRequest && !Components.isSuccessCode(aRequest.status)))

I suppose it might not hurt to check, but this particular progress listener shouldn't ever get called with aRequest == null, right? AFAIK that only happens for the pseudo-WPLs that tabbrowser holds on to (i.e. those called via the _callProgressListeners in updateCurrentBrowser).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 34 • 13 years ago

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33)
> I suppose it might not hurt to check, but this particular progress listener
> shouldn't ever get called with aRequest == null, right?

Oh, nevermind - just saw bug 673752.

	Dão Gottwald [:dao]
	Comment 35 • 13 years ago

Comment on attachment 578298 [details] [diff] [review]
Test (mochitest-browser-chrome)

There are several exceptions thrown during this test due to the progress listener not implementing onSecurityChange, onStateChange, etc.
I'd recommend using addTabsProgressListener, for which you don't need to implement all of nsIWebProgressListener. See browser_bug521216.js for instance.

Also, I can reproduce this bug by loading http://goo.gl/cb1Rr in a foreground tab. Maybe the test should cover this too?

	Dão Gottwald [:dao]
	Comment 36 • 13 years ago

Comment on attachment 578297 [details] [diff] [review]
Firefox ver.

Are you going to remove the aRequest check in bug 673752?

	O. Atsushi (Torisugari) Assignee
	Comment 37 • 13 years ago

Thanks.

(In reply to Dão Gottwald [:dao] from comment #36)
> Are you going to remove the aRequest check in bug 673752?

No. aRequest is very often null, basically when docshell does not use the network (nor cached data). Typically, that's an internal hyperlink, such as <a href="#foo">foo</a>. Other important cases are sessionstore's successful restore and <about:blank>. Therefore I'm a little curious how SeaMonkey handles these null-channel onLocationChanges.

Anyway, every error page has a non-null channel for the time being, more accurately until bug 312680 is fixed. Bug 312680 is probably one of blockers of bug 616843. So in the future, we will reproduce this bug on NS_ERROR_UNKNOWN_PROTOCOL's error page, but that's not a serious security error compared to BAD_CERT's. 

In short, we should do something to userTypedClear or elsewhere.

	O. Atsushi (Torisugari) Assignee
	Comment 38 • 13 years ago

(In reply to Dão Gottwald [:dao] from comment #35)
> There are several exceptions thrown during this test due to the progress
> listener not implementing onSecurityChange, onStateChange, etc.
> I'd recommend using addTabsProgressListener, for which you don't need to
> implement all of nsIWebProgressListener. See browser_bug521216.js for
> instance.

Thank you, I've totally forgot how <browser/>.addProgressListener works.

And I can reproduce foreground's. Added it.

	O. Atsushi (Torisugari) Assignee
	Comment 39 • 13 years ago

Then the checkin comment for mozilla-central would be

"Bug 623155 - Clear userTypedValue on an error page, patch by neil, r=dao"

or something like that. Anyway, please don't forget to mention the original author. Thanks in advance.

	Dão Gottwald [:dao]
	Comment 40 • 13 years ago

Neil's original patch changed "The document loaded correctly, clear the value if we should" to "Clear the value if this was a user document or an error page", which I like, except that the notion of a user document is a new one to me. Can this be ironed out before we land this patch?

	O. Atsushi (Torisugari) Assignee
	Comment 41 • 13 years ago

OK, though I'm the last person to talk with, about English comment lines.  "The document loaded correctly, clear the value if we should" is fine for me, BTW. An error page is correct too.


Anyway,

"was a user document" -> "was a URI set by user's action"

?

"Clear the value if it was set by user's action or this is an error page"

But it contains a lie slightly, for all userTypedValue are set by user's action.

Or the ideal behavior is:
"Clear the value as long as the URLbar's input-field is not focused."

The actual behavior would be:
"Clear the value if there's no user's action during loading the document."

	Dão Gottwald [:dao]
	Comment 42 • 13 years ago

Ok, whatever, I just kept the current comment.

http://hg.mozilla.org/integration/mozilla-inbound/rev/007e6ebef90e

	O. Atsushi (Torisugari) Assignee
	Comment 43 • 13 years ago

As is anticipated, probalby this bug came back, because of bug 312680 's checkin. We need a new way which does not depend on nsIRequest (nsIChannel).

What about SeaMonkey's?

Steps to reproduce:
Use an unknown protocol URI instead, e.g. "foo:///".

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 44 • 12 years ago

Torisugari: does bug 478927 being fixed help address this bug?

	O. Atsushi (Torisugari) Assignee
	Comment 45 • 12 years ago

Yes, that's right.

	O. Atsushi (Torisugari) Assignee
	Comment 46 • 12 years ago

This should work also when |aRequest| is null.

i.e. redirecting to...
protocol not found: <foo:bar>
invalid URI       : <about:foo>

	Dão Gottwald [:dao]
	Comment 47 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/3eab6ea25342

	Dão Gottwald [:dao]
	Comment 48 • 12 years ago

http://hg.mozilla.org/mozilla-central/rev/3eab6ea25342

	Al Billings [:abillings - ex-MoCo]
	Comment 49 • 12 years ago

Using the sample URLs above, even opening in background, I'm not seeing a difference in behavior post-fix on trunk and in Firefox 12 (such as with http://goo.gl/cb1Rr).

	O. Atsushi (Torisugari) Assignee
	Comment 50 • 12 years ago

(In reply to Al Billings [:abillings] from comment #49)
> Using the sample URLs above, even opening in background, I'm not seeing a
> difference in behavior post-fix on trunk and in Firefox 12 (such as with
> http://goo.gl/cb1Rr).

Though I'm not too sure what "status-firefox-esr10" means, whoever wants to test the difference needs:

1. Firefox 13 or later. (per bug 312680)
2. A website which does 3xx redirection to <foo:bar> etc.

As far as about:certerror is concerned, the reported spoof had been fixed by the earlier patch (Firefox 11?). So I guess whether or not we should do something to Firefox 10 ESR would depend on the comparison between Firefox 10 and Firefox 11. Not 12 nor 15.
According to 
http://mxr.mozilla.org/mozilla-esr10/source/browser/base/content/tabbrowser.xml
, surely the old patch has not been applied.

The new patch is based on the interface change (bug 478927 and bug 311007, Firefox 15 and Firefox 11 respectively). So if we are talking about the new patch, it's almost impossible to apply it to Firefox 10. But there's some room to discuss the old patch.

I mean
the old patch: attachment 578297 [details] [diff] [review] (comment #31)
the new patch: attachment 622010 [details] [diff] [review] (comment #46)

Is this comment clear enough?

	Al Billings [:abillings - ex-MoCo]
	Comment 51 • 12 years ago

The comment isn't really clear as it appears that this bug has morphed from the issue for which it was originally open. In those instances, it would be better if we made new bugs for new issues rather than changing the focus of existing bugs.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 52 • 12 years ago

replacing the wontfix from last triage.

	Robert Kaiser
	Comment 53 • 12 years ago

Adding Philip as he's working on bug 782033 (SeaMonkey version of this one).