306810 - Location bar has wrong URL after 302 redirect for link opened in a new tab

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[kmike]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050902 SeaMonkey/1.1a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050902 SeaMonkey/1.1a

After clicking on links which redirect you to other pages with HTTP 302 Found,
URL bar contains the previous page URL instead of new one. Found it on google
results page - they sometimes contain redirects to the actual results.

Reproducible: Always

Steps to Reproduce:
1. Click on
http://www.speedtrader.com/dirinc/click.php?url=http://www.commodity-trading-software.com&id=746


Actual Results:  
URL bar contains
http://www.speedtrader.com/dirinc/click.php?url=http://www.commodity-trading-software.com&id=746


Expected Results:  
URL bar contains http://www.commodity-trading-software.com/

Note that a page info (ctrl-I) shows a right URL, not a previous one, so it is a
location bar which is confused about current URL.

My previous Linux build from 2005-07-13 doesn't exhibit this problem.

This may have something to do with bug 302575, though I'm not sure.

Comment 1

[kmike]

After some more investigation, the culprit is found: XUL.mfasl. After deleting
it this bug is WFM.

Comment 2

[kmike]

Ack, it isn't WFM, even in a pristine new profile. Updating summary and URL,
here're steps to reproduce:

1. Do a google search for "bookmarklets":
http://www.google.com/search?hl=en&ie=UTF-8&q=bookmarklets&btnG=Google+Search
2. Right click on a first result ("Bookmarklets Home Page - free tools for power
surfing"), select "Open link in a new tab"
3. Switch to a new tab, if you don't have it already focused
4. Observe 
"http://www.google.com/url?sa=t&ct=res&cd=1&url=http%3A//www.bookmarklets.com/&ei=3TkYQ63zFp3O4QHH8J3SDA"
in the location bar

Expected results:
http://www.bookmarklets.com/ in the location bar.

Some notes: pressing "reload" will fix URL. Selecting "open in a new window" as
well as simple clicking on a link to open in the same window doesn't trigger the
bug, only "open link in a new tab" does.

Comment 3

[kmike]

Downloaded 
ftp://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/2005-09-01-09-trunk/seamonkey-1.1a.en-US.linux-i686.tar.gz,

it shows the same bug. Testcase #1 works as well when using "open in a new tab".

Comment 4

[Bruno 'Aqualon' Escherl]

Confirming the bug with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1)
Gecko/20050902 SeaMonkey/1.1a and Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.9a1) Gecko/20050901 SeaMonkey/1.1a

No problem with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1)
Gecko/20050901 Firefox/1.6a1 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050902 Firefox/1.6a1

Seems the refresh of the location bar is forgotten after redirect.

Bruno

Comment 5

[Bruno 'Aqualon' Escherl]

It's a regression (can someone set the keyword please?) between Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050831 SeaMonkey/1.1a and
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050901
SeaMonkey/1.1a

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-31+10%3A09%3A00&maxdate=2005-09-01+09%3A20%3A00&cvsroot=%2Fcvsroot

Probably regression from the fix for bug 302575.

Bruno

Comment 6

[Samuel Sidler (old account; do not CC)]

Confirming based on comments 4 and 5. Adding regression keyword. Changing
hardware/OS to all.

Comment 7

[Bruno 'Aqualon' Escherl]

@@ -141,7 +155,13 @@
               catch (e) {
               }
             }
-            this.webNavigation.loadURI(aURI, aFlags, aReferrerURI, null, null);
+            try {
+              this.userTypedClear++;
+              this.webNavigation.loadURI(aURI, aFlags, aReferrerURI, null, null);
+            } finally {
+              if (this.userTypedClear)
+                this.userTypedClear--;
+            }
           ]]>
         </body>
       </method>

This part of the suite patch is the reason for the regression. Backing it out
makes it work, but causes Bug 302575 again.

Comment 8

[[e-mail]]

Attachment: Proposed patch

This problem basically affected loads that had to stop an existing load.
The stop wasn't originally a problem because userTypedClear was already 0.
Before: 0 (-1 stop) +1 from start = 1 therefore OK to clear
After: 0 +1 before loadURI -1 stop +1 start -1 after loadURI = 0 not OK
So to fix this I propose to make start/stop more important i.e. += 2.
This will only affect the way start/stop interacts with faked clear i.e.
New: 0 +1 before loadURI -1 (-2) stop +2 start -1 after loadURI = 1 so OK

Comment 9

[Bruno 'Aqualon' Escherl]

I tested the patch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050901 SeaMonkey/1.1a

This bug gets fixed and Bug 302575 keeps fixed too. Haven't seen any problem
with it so far.

Bruno

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194782 [details] [diff] [review]
Proposed patch

r=bzbarsky, I guess, but please document this very clearly!

Comment 11

[Darin Fisher]

Comment on attachment 194782 [details] [diff] [review]
Proposed patch

Please document the heck out of this.  sr=darin

Comment 12

[[e-mail]]

Draft comment:

<!--
This field tracks the location bar state. The value that the user typed
in to the location bar may not be changed while this field is zero.
However invoking a load will tempoarily increase this field to allow
the location bar to be updated to the new URL.

Case 1: Anchor scroll
  The user appends the anchor to the URL. This sets the location bar
  into typed state, and disables changes to the location bar. The user
  then requests the scroll. loadURIWithFlags temporarily increases the
  flag by 1 so that the anchor scroll's location change resets the
  location bar state.

Case 2: Interrupted load
  The user types in and submits the URL. This triggers an asynchronous
  network load which increases the flag by 2. (The temporary increase
  from loadURIWithFlags is not noticable in this case.) When the load
  is interrupted the flag returns to zero, and the location bar stays
  in typed state.

Case 3: New load
  This works like case 2, but as the load is not interrupted the
  location changes while the flag is still 2 thus resetting the
  location bar state.

Case 4: Corrected load
  This is a combination of case 2 and case 3, except that the original
  load is interrupted by the new load. Normally cancelling and starting
  a new load would reset the flag to 0 and then increase it to 2 again.
  However both actions occur as a consequence of the loadURIWithFlags
  invocation, which adds its temporary increase in to the mix. Since
  the new URL would have been typed in the flag would have been reset
  before loadURIWithFlags incremented it. The interruption resets the
  flag to 0 and increases it to 2. Although loadURIWithFlags will
  decrement the flag it remains at 1 thus allowing the location bar
  state to be reset when the new load changes the location.
  This case also applies when loading into a new browser, as this
  interruptes the default load of about:blank.
-->
<field name="userTypedClear">
  1
</field>

Comment 13

[baffclan]

cannot reproduce with SeaMonkey/2005092105-trunk/WinXP.
wfm/fixed?

Comment 14

[Darin Fisher]

s/noticable/noticeable/ ;-)

Comment 15

[[e-mail]]

Comment checked in with two other typos fixed too.

Comment 16

[[e-mail]]

SeaMonkey-only portion of patch checked in to the 1.8 branch.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-181) <tabbrowser.xml> [Checkin: Comment 22]

'approval-seamonkey1.1a=?':
Attachment 194782 [details] [diff] landed on (Trunk and) MOZILLA_1_8_BRANCH,
but this simple enhanced comment never made it to the branch: synchronizing.

Comment 18

[[e-mail]]

Comment on attachment 222627 [details] [diff] [review]
(Bv1-181) <tabbrowser.xml>
[Checkin: Comment 22]

Personally I don't think it's worth synchronising a comment, but you could always ask jag for a second opinion.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 222627 [details] [diff] [review]
(Bv1-181) <tabbrowser.xml>
[Checkin: Comment 22]

(In reply to comment #18)
> (From update of attachment 222627 [details] [diff] [review] [edit])
> Personally I don't think it's worth synchronising a comment, but you could
> always ask jag for a second opinion.

Not mandatory, for sure;
but I thought it would be good to sync', considering the trunk migration to the Toolkit file...

Comment 20

[jag (Peter Annema)]

Comment on attachment 222627 [details] [diff] [review]
(Bv1-181) <tabbrowser.xml>
[Checkin: Comment 22]

I say let's check this in. Cost to us is virtually nothing (right?) and it might clear this up to someone who's only got access to the 1.8 branch source (unlikely, but ...)

Comment 21

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 222627 [details] [diff] [review]
(Bv1-181) <tabbrowser.xml>
[Checkin: Comment 22]

a=me for 1.1a

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 222627 [details] [diff] [review]
(Bv1-181) <tabbrowser.xml>
[Checkin: Comment 22]


Checkin: {
2006-06-23 14:26	neil%parkwaycc.co.uk 	mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml 	1.126.2.27 	MOZILLA_1_8_BRANCH
}

Comment 23

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060623 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH, between these two builds.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060624 SeaMonkey/1.1a] (nightly) (W98SE)