118373 - escape(null) causes cookies to not be recognized

Status: RESOLVED FIXED

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

Description

[Andrew Steele]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7) Gecko/20011221
BuildID:    2001122106

The site (www.theissue.org) sets two cookies on the first visit to identify
whether the users preference is to have sound played or not.

In Netscape 4.7x, 6.2 and IE 6 this works correctly.  On second and subsequent
visits the browser loads either the with-sound or no-sound site pages based on
the last cookie set.

With Mozilla 0.9.7 the cookies is not recognised and Mozilla loads the default
page which takes the visitor through the initial set-up.

The cookie appears to be written into the cookies.txt file OK but is not
interpreted by Mozilla.

Reproducible: Always
Steps to Reproduce:
1.Go to http://www.theissue.org
2.Site loads default page offering sound or no-sound
3.Select no-sound for simplicity
4.Close browser return to the URL and the initial selection pages load again

Actual Results:  As described...

Expected Results:  Interpreted the cookie and based on the initial selection
by-passed the initial page and gone straight to the no-sound and with-sound
default page.

I'm happy for any gentle chiding if I have mis-coded the cookie stuff...

As mentioned though, it all works OK in IE6, 5.5 and Netscape 4.7x and 6.2

Comment 1

[Boris Zbarsky [:bzbarsky]]

site is doing:

  document.cookie = sound + "=" + soundset + "; expires=" + now.toGMTString();

I bet this is bug 118266

Comment 2

[Stephen P. Morse]

I don't think this is bug 118266, but I could be wrong.

My initial test indicated that the cookie.txt file in n4 had this cookie set 
with a value of "null" (characters "null" actually appear in file) whereas in 
mozilla the value was the empty string.  I suspect therein lies the problem 
although I was unable to follow-up on this because my browser broke for other 
reasons.  As soon as get my tree fixed, I intend to continue pursuing this.

Comment 3

[Stephen P. Morse]

Yes, I was right.  It's the value of "null" that is causing the problem.  I 
prooved that to myself by forcing the cookie module to return the string "null" 
for the cookie value when the value itself is the empty string.  Attaching a 
patch to demonstrate that.

This is not a proposed patch to fix the problem but rather a proof of concept as 
to what the problem is.  Need to determine if this is an evangelism issue on the 
site for not recognizing the cookies that it itself set (it indeed set a cookie 
with no value).  Also need to look at the 4.x code and see why/how it forces the 
value of "null" into the cookie.

Comment 4

[Stephen P. Morse]

Attachment: proof of concept -- not a proposed fix

Comment 5

[Stephen P. Morse]

Looked at the code for 4.x.  There is nothing in the cookie module itself that 
forces a value of "null" into a cookie with an empty value.  I no longer have a 
4.x debug environment so I can't trace where the value of "null" is coming from, 
but since this is a javascript cookie I would venture a guess that js in 4.x was 
returning the value of "null" whereas in mozilla it is returning the value of 
"".

So it seems like we have three choices:

1. If a cookie is being set with a value of "", replace that with "null".
2. Same as one but only for javascript cookies
3. Tell site to accept their own cookies having a value of ""

Solution 2 is safe in that it won't break existing sites because it exactly 
mimics the 4.x behavior (if my theory about where the "null" is coming from is 
correct).  However it seems very strange to treat js cookies one way and http 
cookies another.

Solution 1 is uniform at least, but it could break some sites that set http 
cookies of "".

Comment 6

[Boris Zbarsky [:bzbarsky]]

Which code exactly is setting the cookie to null?

Comment 7

[Stephen P. Morse]

jst: Funny you should ask.  That's just what I'm investigating right now.  Hope 
to have an answer soon.

Comment 8

[Stephen P. Morse]

Oops, I didn't mean to put "jst:" on my previous comment.  I meant to put 
"bzbarsky:" there instead.

Comment 9

[Stephen P. Morse]

OK, I think I know what's happening and I believe it is a js error.

Attaching a simplified test case.  Consists of two files, theissue.html and 
welcome.html.  Put both files in same directory and bring up theissue.html in 
the browser.

Will explain what's going on after I attach the simplified test case.

Comment 10

[Stephen P. Morse]

Attachment: theissue.html

Comment 11

[Stephen P. Morse]

Attachment: welcome.html

Comment 12

[Stephen P. Morse]

Well html files are hard to see in attachments.  So I'll also put these two 
files inline right here.

************ theussue.html *********************

<HTML>
  <HEAD>
    <SCRIPT LANGUAGE="JavaScript">

      function setCookie(name, value) {
        dump("setCookie: {"+name+"} = {"+value+"}\n");
        document.cookie = name + "=" + escape(value);
      }

      function getCookie(name) {
        var prefix = name + "=";
        var cookieStartIndex = document.cookie.indexOf(prefix);
        if (cookieStartIndex == -1) {
          dump("getCookie: no cookie for {"+name+"} found\n");
          return null;
        }
        var cookieEndIndex =
          document.cookie.indexOf(";", cookieStartIndex + prefix.length);
        if (cookieEndIndex == -1) {
          cookieEndIndex = document.cookie.length;
        }
        var value = 
          document.cookie.substring
            (cookieStartIndex + prefix.length, cookieEndIndex);
        dump("getCookie: {"+name+"} = {"+unescape(value)+"}\n");
        return unescape(value);
      }

      var visits = getCookie("newuser")
      if (!visits) {
        dump("site believes that no newuser cookie is set\n");
        location = "welcome.html"
        setCookie("newuser", visits)
      }
    </SCRIPT>
  </HEAD>
  <BODY>
  </BODY>
</HTML>

***************** welcome.html ************************

<HTML>
  <BODY>
    Welcome
  </BODY>
</HTML>

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.. are we being bitten by our correct support for null DOMStrings?  What 
should null.ToString() be?

Comment 14

[Stephen P. Morse]

Well I don't yet understand it, but let me post the results of running my 
simplified test case on both nav4 and on mozilla:

1. Mozilla:

   First visit to theissue.html:

   getCookie: no cookie for {newuser} found
   site believes that no newuser cookie is set
   setCookie: {newuser} = {null}

   Subsequent visits to theissue.html:

   getCookie: {newuser} = {}
   site believes that no newuser cookie is set
   setCookie: {newuser} = {}

2. Nav4

   First visit to theissue.html:

   getCookie: no cookie for {newuser} found
   site believes that no newuser cookie is set
   setCookie: {newuser} = {null}

   Subsequent visits to theissue.html:

   getCookie: no cookie for {newuser} found
   site believes that no newuser cookie is set
   setCookie: {newuser} = {null}

Comment 15

[Stephen P. Morse]

3. IE

   First visit to theissue.html:

   getCookie: no cookie for {newuser} found
   site believes that no newuser cookie is set
   setCookie: {newuser} = {null}


   Subsequent visits to theissue.html:

   getCookie: {newuser} = {null}

Comment 16

[Stephen P. Morse]

And one more bit of information.  If the escape/unescape is removed from the 
simpified test case, then the results for Mozilla, Nav4, and IE are all 
identical -- they are the results that was previously obtained for IE.

Comment 17

[Stephen P. Morse]

OK, here's where the difference lies.  In Nav 4 and in IE

   escape(null) = "null"

wheras in mozilla

   escape(null) = ""

Here's a still more simplified test program to demonstrate this:

  <html>
    <head>
      <script>
        alert("escape(null)={"+escape(null)+"}");
      </script>
    </head>
    <body>
    </body>
  <html>

Comment 18

[Stephen P. Morse]

And for my final proof, I modified my local copy of the http://www.theissue.com 
website by removing the escape in the setcookie routine, and everything worked 
fine.

So now I'm convinced that this is not a cookie problem but rather a javascript 
one per my observation in comment #17.  Reassigning.

(Now that I understand this better, I retract the three choices I stated in 
comment #5.  None of them were valid.)

Comment 19

[Stephen P. Morse]

Reassigning to javascript engine component.

Comment 20

[Stephen P. Morse]

Modifying summary from

   cookies not recognized

to

   escape(null) causes cookies to not be recognized

Comment 21

[Stephen P. Morse]

Just for the record, I'm attaching three files that make up my local copy of 
the http://www.the issue.org website.  These files are:

   theissue.html (starting file)
   welcome.html
   sound.html
   page2.html

The escape that I spoke about in comment 18 is in the setCookie routine of 
theissue.html and the identical routine in sound.html.

Note that when using these files, make sure to check of the box saying you want 
sound.  That will cause the sound.html file to be loaded in.  If you check off 
the no-sound box, it will attempt to fetch a different file which I have not 
included here.

Comment 22

[Stephen P. Morse]

Attachment: full version of theissue.html

Comment 23

[Stephen P. Morse]

Attachment: full version of welcome.html

Comment 24

[Stephen P. Morse]

Attachment: full version of sound.html

Comment 25

[Stephen P. Morse]

Attachment: full version of page2.html

Comment 26

[Stephen P. Morse]

Sorry, I lied in comment #21.  It was four files, not three.

Comment 27

[Phil Schwartau]

From Comment #17 above: 

> OK, here's where the difference lies.  In Nav 4 and in IE
>    escape(null) = "null"
> whereas in Mozilla
>    escape(null) = ""


Confirming this behavior. Note that in the browser, the DOM escape()
function supersedes the JavaScript Engine escape() function. In fact,
in the current standalone JS Engine, we show the Nav4/IE behavior:

            js> escape(null);
            null

Whereas in a current Mozilla build, the following javascript: URL 
shows that in the DOM, escape(null)=== the empty string, as Stehen says:

            javascript: alert(escape(null) === '');


Therefore reassigning to DOM Level 0 component - 

Comment 28

[Fabian Guisset]

Boris was right, this is beause we pass the new null string to
nsGlobalWindow::Escape().
I think something as simple as
if (isNullDOMString(aStr)) {
  setDOMStringToNull(aReturn);
}
would do the job (and would avoid constructing all the encoder stuff we
currently build in this case).

Comment 29

[Andrew Steele]

As always a most helpful and thorough contribution from everyone.  I've asked
the author to take note of the comments and adjust the code which has resolved
the problem for the particular site.  Thus the live site should very shortly
function OK.  This won't, though, be because the bug has been fixed but because
the page code has been changed :-)

I hope that the discussion has been helpful for future
implementations/development - I've certainly found it instructive.

Comment 30

[Stephen P. Morse]

Even if the site changes their code to work around the problem, we can still 
reproduce it using the captured files form the site that I attached in this 
report.

Comment 31

[Johnny Stenback (:jst)]

Pushing to mozilla1.0

Comment 32

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 33

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 34

[Boris Zbarsky [:bzbarsky]]

OK, so in IE and NS4 escape(null) == "null" (the string, not a null value).  Do
we want to emulate this behavior?

Comment 35

[Fabian Guisset]

The JS spec as the following to stay about escape():
1. Call ToString(string)
2. Compute the number of characters in Result(1)
3. Let R be the empty string
4. Let k be 0
5. If k equals Result(2), return R

So per spec, we are doing the right thing. I searched for some uses of
escape(null) on the web and couldn't find any, so I guess if this bug doesn't
block the site anymore, we should leave the behavior as is.
Just IMHO, of course.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Um.. What is ToString(null)?  Is it ""?

Comment 37

[Fabian Guisset]

does it matter? the number of chars in it is 0...

Comment 38

[Boris Zbarsky [:bzbarsky]]

As long as ToString(null) is not "null"  ;)

Ok, I'm fine with just marking this wontfix.

Comment 39

[Fabian Guisset]

Marking WONTFIX, please feel free to reopen if you disagree.

Comment 40

[Johnny Stenback (:jst)]

I don't know the EcmaScript spec in enough detail to know what ToString(null)
is, but I know JS_ValueToString(null) in Mozilla gives the string "null"...

Comment 41

[Phil Schwartau]

Ref: http://www.mozilla.org/js/language/E262-3.pdf

I don't know if the EcmaScript spec is relevant here, since in the browser
escape() is superseded by the DOM escape() (right?). But FWIW, here is the
EcmaScript specification for |escape(null)|:

B.2.1 escape(string)
When the escape() is called with argument string, the following steps are taken:

1. Call ToString(string)
        etc.


9.8 ToString
The operator ToString converts its argument to a value of type String
according to the following table:

Input Type      Result
Undefined     "undefined"
Null            "null"
        etc.


So in pure, browser-independent EcmaScript, escape(null) === "null"

Comment 42

[Boris Zbarsky [:bzbarsky]]

Reopening, then.  Because both IE and NS4 return "null" while we return "".

Comment 43

[Boris Zbarsky [:bzbarsky]]

We now use the JS engine's escape(), so this is fixed (by the patch in bug 44272).