9472 - window.location.search not set

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[richmond]

window.location.search is not set when a HTTP 302 redirect occurs to a page
with a search string which is how Messenger Express server passes arguments to
the client application

Comment 1

[vidur (gone)]

The original URL specified is not valid. I did find a place on Netcenter where a
HTTP 302 redirect takes you to a page with a search component (a ? in the URL).
It seems like netlib is stripping out or somehow losing the search component. As
a result, querying it through JavaScript returns the wrong answer.

Gagan, I think this is you.

Comment 2

[Gagan]

have a dup. a dup that Andreas fixed.
so this should be fixed. Dr.Seuss. is a dup too.
Cc'ing Andreas and marking fixed.

Comment 3

[Prashant Desale]

Tested with todays builds [10-19-09]. Working fine, hence marking verified

Comment 4

[Andreas Otte]

This is the bug I was waiting for. It contradicts the patch I have done for bug
14946. Will change that fix to only not transfer params. I wonder when we will
have a site that needs params transfered?

Comment 5

[Andreas Otte]

Ricks fix for bug 16418 broke this one again. The redirection now moves you from
http://home.netscape.com/netcenter/personalize/index.html?cp=hom09pmyn to
http://my.netscape.com/

I don't think this is what should happen.

Comment 6

[Andreas Otte]

It also happens when I put back in the query part, so this may not be the right
url to test this.

Comment 7

[leger]

Clearing FIXED resolution due to reopen of this bug.

Comment 8

[Warren Harris]

Moving to m15.

Comment 9

[Steve Lamm]

28069 is marked PDT+ for beta1. This bug causes links in the sidebar to load the
wrong page.

Comment 10

[Steve Lamm]

Check out 28069 for more information (especially my last comments) and an easy 
way to reproduce the bug.

Comment 11

[leger]

Please give us an ETA date for fix in Status Whiteboard.  Thanks!

Comment 12

[rpotts (gone)]

Removing PDT+ since bug #28069 (which is PDT+) seems to work...

Comment 13

[Steve Lamm]

Bug 28069 still happens (I ran it on a debug Windows build from today). Here is
another way to reproduce it,

1. Add "Reuters News" to your sidebar
2. Open that panel and click on one of the headlines.

"my.netscape.com" gets loaded instead of the headline.

Comment 14

[vidur (gone)]

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

Comment 15

[vidur (gone)]

Bug 28730 also depends on this and it is a PDT+.

Comment 16

[kevinyen]

Adding myself to cc:
-kevin

Comment 17

[leger]

Putting on PDT+ radar for beta1. 

Comment 18

[rpotts (gone)]

hey gagan,

This is yet another instance of the WebShell needing to implement
nsIHTTPEventSink.  When the WebShell gets an OnRedirect(...) it should update
the document URL.

This *should* fix Window.location too.

I'll let you decide it it is really just a dup or not :-)

Comment 19

[Norris Boyd]

I've got changes in my tree that have nsWebShell implement nsIHTTPEventSink and 
when the WebShell gets an OnRedirect(...) it updates the WebShell's mURL member 
(see bug 29541 for more information and the proposed patch). 

Is this what rpotts suggests? Or is there additional work needed to set the 
document's URL?

Comment 20

[rpotts (gone)]

hey norris,

This sounds like the same fix...  You should talk with gagan, because he has a 
whole herd of bugs related to nsIHTTPEventSink...

Comment 21

[Gagan]

yes this is a definite dup of bug 27048 and norris we need to merge in our 
changes...

*** This bug has been marked as a duplicate of 27048 ***

Comment 22

[Gagan]

this bug is no longer a dup of 27048 but possibly a dup of bug 30783. 
Essentially window.location (and hence window.location.search) are reading the 
previous (incorrect) URL and not the redirected one... someone in js/dom land 
would have to comment on where it picks the window.location from, if its from 
DocShell then this should work, but if we are looking at the location bar in the 
UI then this is definitely a dup of 30783. 

I am clearing the resolution of dup... and assiging back to vidur.

Comment 23

[Gagan]

->vidur

Comment 24

[Gagan]

off to norris who recently fixed GetURL of webshell to return the redirected URL 
. If we are calling GetURL on the webshell (which vidur convinces me that we 
are) then this should work correctly... otherwise something is messed up in the 
webshell's GetURL call which is not returning the redirected URL. 

Comment 25

[racham]

Adding steve and myself to the cc list.

Comment 26

[Norris Boyd]

The page http://home.netscape.com/netcenter/personalize/index.html?cp=hom09pmyn 
has the following HTML:

<SCRIPT LANGUAGE="JavaScript1.1">
var str = parseInt(10000*Math.random());
document.location.replace("http://my.netscape.com/?s="+str);
</SCRIPT>
<META HTTP-EQUIV="Refresh" CONTENT="0; URL=http://my.netscape.com/">

Looking at this in the debugger, it appears that location.replace is called with 
the intended URI and that load is begun, but that the http refresh comes along 
and overrides it. If I skip over the refresh code in the debugger, I get the URI 
with the search string loaded. 

So perhaps we need some way to cancel pending refreshes when we execute 
location.replace()?

Comment 27

[Norris Boyd]

Actually, I see there is already code in nsWebShell::CancelRefreshURITimers to 
cancel pending refreshes. The problem is that the HTTPContentSink has not yet 
encountered the META tag, and it looks like it continues processing even after 
the call to location.replace().

What do people think of fixing this by stopping the content sink from 
processing when location.replace is called?

Comment 28

[daver]

adding Must Fix to status whiteboard.  Activation needs to work, somehow, for 
beta 1.   A workaround is fine.  Do you need more engineering help?

Comment 29

[kevinyen]

Sorry for the late response.  Occupied by press demo firedrills in last 36 hrs.

In a nutshell, Activation is in good shape but: (1) there remains uncertainty 
about work needed to fix bugs, (2) it is time we signal HIGH URGENCY with 
Netcenter and AOL/P development teams we've been working with.  BTW, we've been 
trying to, but it may be time to call in the enforcer (Daver) for this.

I'll update ASAP.

thx,
kevin

Comment 30

[Norris Boyd]

Yes, help would be appreciated. I'm far afield from any code I've written.

Comment 31

[Norris Boyd]

Well, I could hack on it, but I'm not sure what else I might break. I haven't 
had any offers of help... Should I just reassign back to gagan? I'm willing to 
help someone who knows the networking code with the DOM side of it.

Comment 32

[Jim Roskind]

I just added Warren (Network) and Vidur (who is leaving (left?) for sabbatical)
(DOM) to the CC list.  Out of desperation, I'm also adding Jband (JS
cleverness)and gagan ('cause norris muttered his name) to the CC list.

This bug is pretty critical to the Netcenter activation story.  We're working
with Netcenter to see if they can work around this bug... but getting traction
on this would be the thing to do.

Any suggestions from the extended CC list about who should handle this??

Comment 33

[Norris Boyd]

It appears now that this bug does not need a fix to get activation working. 
Waiting for complete test from Netcenter to completely verify. 

Comment 34

[racham]

It is realized that the actual problems were in javascript sources on activation 
servers, as things started working fine once activation team fixed them. Once 
they reached the place where they called window.location.search, the client is 
doing the right thing (in the activation case). I have already removed bug 
27797's dependency on this one.

Comment 35

[Jim Roskind]

At what point can we close this bug?  Alternatively, we could have a more
descriptive bug that tracks issues at Netcenter, since the comments suggest that
window.location.search is not an issue.

Comment 36

[Norris Boyd]

Based on the comments above, I'm changing the summary from "[Activation] 
window.location.search not set" to "meta refresh overrides pending 
location.replace" and removing the PDT+ designation. If PDT agrees and sets 
PDT-, I'll remove the beta keyword.

Comment 37

[John G. Myers]

Please open a new bug instead of changing the summary.  The original issue, that 
window.location.search was not set when a HTTP 302 redirect occurs, needs to be 
verified.

Comment 38

[leger]

Putting on PDT- radar for beta1.

Comment 39

[Norris Boyd]

What is the original bug? Is there a test case?

Comment 40

[John G. Myers]

For the original bug, see the first few bug comments.

The original testcase is in bug 8310, but that testcase is hampered by bugs 
outside of Mozilla.  I don't have the expertise to generate a minimal testcase, 
but the testcase in the 1999-11-01 02:39 may suffice.

Comment 41

[Norris Boyd]

Remove beta1 keyword.

Comment 42

[Gagan]

norris: I think the issue is just deciding on what take precedence between a 
<SCRIPT> tag and a <META refresh> If the plan is to use the first occurence of 
either then we should just abort the other one. 

Comment 43

[John G. Myers]

The <SCRIPT> tag versus <META refresh> issue is a separate bug.  Undoing the 
Summary change of 2000-03-13 20:25

Comment 44

[Norris Boyd]

Undoing the URL since it doesn't correspond to the summary. Opened 32049 for the 
meta refresh / location replace problem.

Forward to owner of DOM component for the redirect problem.

Comment 45

[Johnny Stenback (:jst)]

I don't yet have a clear picture of the problem here but if I did understand
this correctly then could this patch be something along the lines that we'd
want here?

Index: dom/src/base/nsLocation.cpp
===================================================================
RCS file: /cvsroot/mozilla/dom/src/base/nsLocation.cpp,v
retrieving revision 1.50
diff -u -r1.50 nsLocation.cpp
--- nsLocation.cpp      2000/03/30 22:38:19     1.50
+++ nsLocation.cpp      2000/03/31 22:21:43
@@ -381,6 +381,24 @@
     loadInfo->SetReferrer(referrer);
     loadInfo->SetReplaceSessionHistorySlot(aReplace);

+    nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(mDocShell));
+
+    if (webNav) {
+      nsCOMPtr<nsIDOMDocument> domDoc;
+
+      result = webNav->GetDocument(getter_AddRefs(domDoc));
+
+      if (NS_SUCCEEDED(result) && domDoc) {
+        nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
+
+        if (doc) {
+          doc->StopDocumentLoad();
+        }
+      }
+    }
+
+    mDocShell->StopLoad();
+
     return mDocShell->LoadURI(newUrl, loadInfo);
   }


(This patch is not tested at all, it does compile tho)

How can I test if this bug is fixed or not?

Comment 46

[Johnny Stenback (:jst)]

Moving to M16

Comment 47

[Prashant Desale]

Adding nsbeta2 to keywords.

Comment 48

[leger]

Putting on [nsbeta2+][6/01] radar.  This work must be done by 06/01 or we may 
pull this for PR2.

Comment 49

[John G. Myers]

The original bug 8310 now works.  Test case is http://we-gotmail.red.iplanet.com 
with userid "x" password "x".  I suspect this can be closed now.

Comment 50

[Johnny Stenback (:jst)]

Based on jgmyers comment I'm closing this bug. I honestly have no idea what the
problem is here so if you do reopen this bug, please supply a short clear
description of the real problem here, and please also attach a testcase showing
the problem (please don't point point to any urls not accessible from outside
the Netscape firewall since I work from outside the firewall).

Comment 51

[Prashant Desale]

Verified with 2000-02-22-08 using testcase provided by John G. Myers.