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.

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)

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…

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

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

Seen again for http://ur1.ca/3iwi9 → https://campus.ftacademy.org/login/index.php Can we get this bug escalated somewhat?

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.

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

(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...

There's bug 311007

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

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

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

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

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...

Blair: this is similar to bug 568410.

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

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?

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.

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

(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.

(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.

(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+.

(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+.

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.

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.

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.

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

(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.

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.

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

(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.

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?

Comment on attachment 578297 [details] [diff] [review] Firefox ver. Are you going to remove the aRequest check in bug 673752?

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.

(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.

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.

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?

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."

Ok, whatever, I just kept the current comment. http://hg.mozilla.org/integration/mozilla-inbound/rev/007e6ebef90e

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:///".

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

Yes, that's right.

This should work also when |aRequest| is null. i.e. redirecting to... protocol not found: <foo:bar> invalid URI : <about:foo>

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

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

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.

replacing the wontfix from last triage.

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