213257 - unable to activate JS_HAS_FILE_OBJECT support in js interpreter

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[A. Guru]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4b) Gecko/20030516 Mozilla Firebird/0.6

This is for js-1.5-rc5a.

I am trying to activate support in the standalone
javascript interpreter.  I modified the Makefile.ref, linked with
libnspr4 and made sure that the file stuff was compiled in by using the
/usr/bin/strings command on the resulting binary to look for "File" strings.

However, when running the interpreter, I am unable to do this

        js> f = new File
        1: ReferenceError: File is not defined

Moreover, the stats('atom') command does not list anything related to
files.  There is no trace of the standard streams "input", "output", and
"error".


Reproducible: Always

Steps to Reproduce:
1.  Modify Makefile.ref
2.  make -f Makefile.ref JS_HAS_FILE_OBJECT=1
2a.  compile jsfile.c with -I/usr/include/mozilla-1.0.2/nspr
2b.  link js with -lnspr4
3.  Execute Linux_All_DBG.OBJ/js
4.  Try
        f = new File
        stats('atom')
5.  Execute
        strings Linux_All_DBG.OBJ/js | grep File
    to verify that file support in compiled in




I have a patched Red Hat 7.3 system with mozilla*-1.0.2-2.7.3.i386.rpm.

Comment 1

[A. Guru]

Attachment: patch to properly activate JS_HAS_FILE_OBJECT in Makefile.ref

This is needed since the code uses
  #if JS_HAS_FILE_OBJECT
not
  #ifdef JS_HAS_FILE_OBJECT

Comment 2

[A. Guru]

Attachment: patch that solves problem

The additional lines in Makefile.ref need to be generalized.  I've put there
what was necessary on my Red Hat 7.3 system.

The bug in jsfile.c needs to be fixed anyway, even if the rest of the patch is
not applied.

It is unclear to me why js_InitFileClass() was made to take three arguments,
contrary to other Init functions that take only two.  The js_InitFileClass2()
wrapper solves the problem.

Comment 3

[Phil Schwartau]

Confirming and cc'ing JS developers.
Recall we have these other open bugs on the File object:

bug 132949 "Uninitialized pointer used in function call in jsfile.c"
bug 181324 "jsfile.c buggy on Windows"

Comment 4

[A. Guru]

Attachment: supplementary jsfile.c patch

Consider the case where the caller provides a path made only of space
characters.

Comment 5

[timeless]

Comment on attachment 128179 [details] [diff] [review]
patch that solves problem

>--- jsfile.c.orig-1.5-rc5a	Thu Apr  3 22:00:55 2003
>+++ jsfile.c	Mon Jul 21 16:47:50 2003
>@@ -375,7 +375,7 @@ js_fileDirectoryName(JSContext *cx, cons
not enough context, i needed to see the whole function :(
>         }
> 
>         /* add terminating separator */
>-        index = strlen(result)-1;
>+        index = strlen(result);
>         result[index] = FILESEPARATOR;
>         result[index+1] = '\0';
>     } else{

you can do this, but you could clean it up a bit - i wonder if there's some bug
w/ that cleaning, or if i have it in another tree somewhere.
(for reference the rest of the segment is reproduced here)
362	    result = (char*)JS_malloc(cx, index+4); 
buffer has space for 3 extra chars and a null
363	    if (!result)  return NULL; 
364 #if defined(XP_WIN) || defined(XP_OS2) 
365	    if (drive!='\0') { 
366		result[0] = toupper(drive); 
first char
367		result[1] = ':'; 
second char
368		strncpy(&result[2], pathname, index); 
369			    result[index+3] = '\0'; 
fourth char (null)
370	    }else 
371 #endif 
372	    { 
373		strncpy(result, pathname, index); 
374			    result[index] = '\0'; 
375	    } 
376 
377	    /* add terminating separator */ 
378	    index = strlen(result)-1; 
379	    result[index] = FILESEPARATOR; 
potentially the third char (except it's part of the original string's length)
380	    result[index+1] = '\0'; 
fourth char (null)

There are certain things i'd do, like twiddling index

< 369
> 369 index += 3;
- 374
< 376
> 376 result[index]='\0'; /* only set null character in one place */
< 377
> 377 /* set terminating separator to primary separator - why are we doing this? */
- 378
< 379
> 379 result[index-1]=FILESEPARATOR;
- 380

Comment 6

[A. Guru]

There is another bug.  See bug id 213447.
(The patch there has even less context. :-)

Comment 7

[timeless]

oh bah. modify my changes to say 2 instead of 3 and roll them in here, we don't
need an extra bug for that.  at 2 it might be time to fix the bounds on the
JS_malloc (which are always too high but especially on drive letterless volumes,
but i'm not sure if we mind the extra bytes).

Comment 8

[A. Guru]

Attachment: patch to summarize jsfile.c:js_fileDirectoryName() fix

Is this ok?

Comment 9

[timeless]

Comment on attachment 128469 [details] [diff] [review]
patch to summarize jsfile.c:js_fileDirectoryName() fix

i think so, but i wasn't sure about the braces following the else/#endif.

Comment 10

[bill]

If this fix works, it would be nice if it were included in a stable release.
It is not in http://ftp.mozilla.org/pub/mozilla.org/js/js-1.5.tar.gz (which is the only spidermonkey tar ball I can find).
Cf problem 335652

Thanks
Bill

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 128469 [details] [diff] [review]
patch to summarize jsfile.c:js_fileDirectoryName() fix

jsfile.c is a folly; avoid.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 128469 [details] [diff] [review]
patch to summarize jsfile.c:js_fileDirectoryName() fix

I don't think this is needed anymore.

Comment 13

[Robert Sayre]

WONTFIX per comment 11 and comment 12