137193 - Debug Mozilla aborts when JSObject.call("func", args) and args is zero-length array

Status: RESOLVED FIXED

(Core Graveyard :: Java: Live Connect, defect, P3)

Description

[Xiaobin Lu]

From Bugzilla Helper:
User-Agent: Mozilla/4.79C-CCK-MCD  [en] (X11; U; SunOS 5.8 sun4u)
BuildID:    20020412



Reproducible: Always
Steps to Reproduce:
1. Open browser
2. Launch the testcase, it will crash right away
3.

Actual Results:  Crash

Expected Results:  Function normal

Comment 1

[Xiaobin Lu]

Attachment: Check the agrc before calling JS_Malloc

Comment 2

[Xiaobin Lu]

Please review the patch!

Comment 3

[Phil Schwartau]

Formally confirming bug; cc'ing more reviewers for this patch -


> 2. Launch the testcase, it will crash right away

Xiaobin, I don't see the URL for this anywhere. Could you post 
that in the URL field above, or attach it to this bug? Thanks -

Comment 4

[Xiaobin Lu]

/**
* <applet code=navmeth_Test.class height=500 width=500>
* </applet>
*/
import java.awt.*;
import java.applet.*;
import java.io.*;
import java.net.*;
import netscape.javascript.*;

public class navmeth_Test extends TestMain  {


    public void	start()	 {

    testname = "navmeth_Test";
	try  {
	    JSObject win = JSObject.getWindow(this);
	    JSObject nav = (JSObject)win.getMember("navigator");
            System.out.println("The navigator object is ..." + nav);

	    try	{
		t.append("\nTesting Navigator4 Properties.... \n");
		
		t.append("\nTesting Navigator Functions.... \n");
		call(nav, "javaEnabled", new String[] {	});
		//	call(nav, "refresh", new String[] { });
	        //call(nav, "taintEnabled", new String[] { });
		//      call(nav, "test", new String[] { });
	    }
	    catch (Throwable  e)	 {
   	      this.handlException(e);
	      errors=errors+1;
          System.out.println("exception occured");
		  e.printStackTrace();
	    } 


	 }
	  catch (Throwable e ) {
   	   this.handlException(e);
	   errors=errors+1;
	   t.append("\n Exception Occured\n");
	  }
	  t.append("\n Checking the STATUS....\n");
      System.out.println("Checking the status");
      this.checkStatus();
	  t.append("\n Test" +" " + testname + ":" + teststatus);
   }


}

// TestMain.java
/**
*/
import java.awt.*;
import java.applet.*;
import java.io.*;
import java.lang.*;
import java.util.*;
import java.net.*;
import netscape.javascript.*;


public class TestMain extends Applet  {
      TextArea t;
      public String teststatus;   // testcase pass/fail status
      public static int errors;   // number of unit test errors
      private String TAB = "  ";   // format string
      public String testname;     // testcase name
      //public String logfile;     // logfilename name
      long filelength;
      FileWriter fw;
      Date today = new Date( System.currentTimeMillis());

    public void init()  {
    setLayout(new BorderLayout());
    t = new TextArea();
    add(t, BorderLayout.CENTER);
    show();
    }


    public Object getMember(JSObject jobj, String member)  {
	try  {
	
	    t.append("Getting: " + jobj.toString() + "." + member + ": ");
            t.append("\nStarting getMember Function .... \n");
	    Object s = jobj.getMember(member);
	    if (s != null) {
		t.append(s.toString());
		if (s.toString().indexOf("\n") == -1)
		    t.append("\n");
	    }
	    else    {
		t.append("null\n");
	    }

	    return s;
	}
	catch (Throwable e)  {
		handlException(e);
		errors = errors + 1;
	    t.append(e.toString() + "\n");
	    return null;
	}
    }

    public void	setMember(JSObject jobj, String	member,	Object value)  {
	try  {
		t.append("\nStarting setMember Function .... \n");
	    t.append("Setting: " + jobj.toString() + "." + member + "= " +
value.toString());
	    jobj.setMember(member, value);
	    getMember(jobj, member);
	}
	catch (Throwable e)  {
		handlException(e);
		errors = errors+1;
	    e.printStackTrace();
	    t.append(e.toString() + "\n");
	}
    }

    public void	call(JSObject jobj, String method, Object[] args)  {
	try  {
		t.append("\nStarting call Function .... \n");
	    t.append("Calling: " + jobj.toString() + "." + method + ": ");
	    Object s = jobj.call(method, args);
	    if (s != null) {
		t.append(s.toString());
		if (s.toString().indexOf("\n") == -1)
		    t.append("\n");
	    }
	    else    {
		t.append("null\n");
	    }
	}
	catch (Throwable e)  {
		handlException(e);
		errors = errors+1;
	    e.printStackTrace();
	    t.append(e.toString() + "\n");
	}
    }
      
	public void writefile() {
      try {

			String os;
                        RandomAccessFile filename;
			os = System.getProperty("os.name");
			String path = "c:\\tmp\\ReportFile" ;
			String upath = "/tmp/ReportFile";
			if (os.indexOf("Windows") != -1){
                          filename = new RandomAccessFile(path,"rw");
                          System.out.println("Writing report to :"+path);
			}else
			{
                          filename = new RandomAccessFile(upath,"rw");
                          System.out.println("Writing report to :"+upath);
			}
            //FileWriter fw = new FileWriter(filename);
            //BufferedWriter bw = new BufferedWriter(fw);
            //PrintWriter out = new PrintWriter(bw);
		    //out.println("STATUS:"+TAB+testname+TAB+TAB+teststatus+TAB);
            //bw.close();
            //fw.close();
	    filelength = filename.length();
		if (filelength == 0){
	
filename.writeBytes("STATUS"+TAB+":"+TAB+testname+TAB+TAB+teststatus+TAB+TAB+today.toString()+"\n");
		}else
		 {
	    filename.seek(filelength);
	
filename.writeBytes("STATUS"+TAB+":"+TAB+testname+TAB+TAB+teststatus+TAB+TAB+today.toString()+"\n");
		 }

		 filename.close();
         }catch(IOException e) {; }
	   
	}
    public void writeReport() {
     //writefile();
	 System.out.println("STATUS:"+TAB+testname+TAB+teststatus+TAB);
     
	 }

     /* check testcase pass/fail status
    */
    public void checkStatus()
    {
       if ( errors == 0 ) {
           teststatus = "PASSED";
       } else {
          teststatus = "FAILED";
       }
       writeReport();
    }

  /* deals with exception, produces stack trace, writes test report
  */
    
   public void handlException(Throwable e)
    {
        System.out.println("Exception caught!");
        teststatus = "EXCEPTION";
        e.printStackTrace();
        //writeReport();

    }

}

Comment 5

[Phil Schwartau]

Xiaobin: thanks!

When I tried the testcase on WinNT, this is the output I saw on the HTML page:

Exception Occured
Checking the STATUS....
Test navmeth_Test:FAILED


And this is the output from the Java Console:

Exception caught!
netscape.javascript.JSException
	at netscape.javascript.JSObject.getWindow(Unknown Source)
	at navmeth_Test.start(navmeth_Test.java:14)
	at sun.applet.AppletPanel.run(Unknown Source)
	at java.lang.Thread.run(Unknown Source)
Checking the status
STATUS:  navmeth_Test  FAILED  



But the browser stays up; I don't crash. Does that sound right to you?


I'm using 

    File name: D:\moz-BIN\2002041106\bin\plugins\NPOJI600.dll
    Java Plug-in 1.3.0_01 for Netscape Navigator (DLL Helper)

    Mime Type Description Suffixes Enabled
    application/x-java-vm Java Virtual Machine for Netscape 6.x

Comment 6

[Xiaobin Lu]

In Solaris, it will crash the system since malloc will crash when the passed in
size is 0.

Comment 7

[Xiaobin Lu]

Joe, can you get my fix reviewed and super reviewed, after that you can check in
that fix. Thanks!

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Are you really saying that Solaris' malloc doesn't handle a size of 0?  I 
thought ANSI (C and C++ both) required that zero-sized allocations work, and 
return a non-NULL pointer if possible (typically by bumping 0 size to be 1).

If Solaris' malloc is really that fragile, I think we should be wrapping it 
with some #ifdef SOLARIS (or #ifdef MALLOC_HATES_ZERO, if this losing is more 
widespread) code.

Comment 9

[Xiaobin Lu]

I made a bad comment. Actually I should say it is because the function 
JS_malloc defined in jsapi.c make an assertion when the size is 0. And in the 
debugging mode, it will cause crash. 

Comment 10

[Joe Chou]

Xiao Bin, I'll try the test case and the patch now.

Comment 11

[Joe Chou]

I did some testing and tracing on the test case, and here is what I've found
out: the browser was aborted, instead of crashing.
Currently, in JS_malloc() there is an assertion if number of bytes to be
allocated is 0. In that assertion (JS_Assert(), see below), after printing a
message saying for what reason the assertion occurred, if Windows or OS2,
DebugBreak() was called. Else if non-Mac (i.e., Unix, etc.), abort() was called
(as in this test on Solaris and Linux, no crash though). For Mac, do nothing and
keeps going.
The real problem here seems to be in JS_Assert()'s over-react for Unix. If
non-debug build keeps going, why should debug be aborted? If debug in other OS
can keep going, why Unix be aborted?  This has been a pain in Unix debuging. I
think it is time to correct it: after printing the assertion message, just keep
going.

Attached patch for fixing JS_Assert().

Comment 12

[Joe Chou]

Accepting the bug.

Comment 13

[Joe Chou]

Attachment: Patch to skip abort after assertion in debug

Comment 14

[timeless]

I was under the impression that JS Asserts were intentionally fatal. Which 
means that if you have a case where you don't want a fatal assert, you should 
not use them.

Comment 15

[Mike Kaply [:mkaply]]

For the record, the official stance on malloc(0) is undefined.

Here is an example comment from one reference:

Upon successful completion with size not equal to 0, malloc() returns a pointer 
to the allocated space. If size is 0, either a null pointer or a unique
pointer that can be successfully passed to free() will be returned. Otherwise, 
it returns a null pointer and sets errno to indicate the error. 

This is typical of most docs.

So we should avoid mallocing 0.

OS/2 has this problem, so we have fixed many malloc(0) issues in the past.

We also found that in many cases, the malloc(0) were causing lots of unnecessary 
code to be executed, so in the end, working around malloc(0) issues caused less 
code to be executed.

Comment 16

[Joe Chou]

My responses to the two comments above:
1) JS_Assert() supposed to be fatel (comment #14):
   I don't think this assertion should be fatel. Just because some web page has
   some error (try to allocate 0 bytes, etc.), we should not abort the browser,
   instead, we should flag the error and move on. Aborting here seems an over 
   kill. I like the way Mac is handling right now.
   On the other hand, if it should be fatal, then it should exit for all
   platforms, not just Unix. Plus, should the commercial builds also exit at the
   assertion (currently not)? If not, why should the debug builds?
   Again, I think the abort here is an over kill, which has been a pain while 
   debugging. Many times, I have to comment the abort call out to resume the 
   debugging.

2) malloc (0) (comment #15):
   Actually, JS_Malloc() has a way to handle this case. If size 0, it will first 
   call JS_Assert(), then allocate size 1. The following code will not use the
   pointer if size is 0, or argc is 0. So the current code has a way to deal 
   with malloc(0). It may not be the ideal way, but it wouldn't crash.

Comment 17

[Joe Chou]

Xiaobin, why this is a blocker? It seems to be only affecting debug builds,
aborted, but not crashed.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 79372 [details] [diff] [review]
Patch to skip abort after assertion in debug

Sorry, no.  Assertion failures mean there is a bug in some code that should be
fixed.	The assertion in JS_malloc is telling you to look at the caller of
JS_malloc, and figure out how to teach it to avoid wasting cycles allocating a
zero net length hunk of memory.

/be

Comment 19

[Xiaobin Lu]

Exactly as Brendan said, this is why I added a check before calling JS_Malloc,
+        if (argc != 0)
+            argv = (jsval*)JS_malloc(cx, argc * sizeof(jsval));

This bug is basically a logic error in liveconnect code, so fix the code there 
is reasonable. BTW, the reason I filed this bug is because I find that it is 
really annoying to see crash when I have a debug build even though it is not 
crash in release build.
     

Comment 20

[Patrick C. Beard]

r=beard on guarding against passing 0 to JS_malloc. Can we see a patch, please?

Comment 21

[Xiaobin Lu]

cvs server: Diffing .
Index: nsCLiveconnect.cpp
===================================================================
RCS file: /cvsroot/mozilla/js/src/liveconnect/nsCLiveconnect.cpp,v
retrieving revision 1.25
diff -u -r1.25 nsCLiveconnect.cpp
--- nsCLiveconnect.cpp  19 Mar 2002 04:28:47 -0000      1.25
+++ nsCLiveconnect.cpp  12 Apr 2002 22:12:00 -0000
@@ -436,7 +436,8 @@
     /* Allocate space for JS arguments */
     if (java_args) {
         argc = jEnv->GetArrayLength(java_args);
-        argv = (jsval*)JS_malloc(cx, argc * sizeof(jsval));
+        if (argc != 0)
+            argv = (jsval*)JS_malloc(cx, argc * sizeof(jsval));
     } else {
         argc = 0;
         argv = 0;

Please see the patch above.

Comment 22

[Olivier Cahagne]

what's the status on this patch ?

Comment 23

[Phil Schwartau]

Formally reassigning to Patrick to get this back on the radar screen.
Joe is no longer at Sun, I believe - (please correct me if I'm wrong!)

Comment 24

[saari (gone)]

->peterl

Comment 25

[Peter Lubczynski]

cc:ing OJI module owner

Comment 26

[Kyle Yuan]

Comment on attachment 78988 [details] [diff] [review]
Check the agrc before calling JS_Malloc

I'm still seeing this problem using recent trunk build. The fix is
straightforward, and got 
r=beard in comment. Brendan, mind to sr=?

Comment 27

[Brendan Eich [:brendan]]

JS_malloc no longer asserts when passed 0, it instead allocates 1 byte via
malloc. So I don't think this patch is necessarly; it seems to leave argv
uninitialized in the argc == 0 case, which isn't good.

What's the current crash stack backtrace?

/be

Comment 28

[Kyle Yuan]

I see the assertion is still there:
http://lxr.mozilla.org/seamonkey/source/js/src/jsapi.c#1436

And as before, mozilla aborts, no crashs. I changed the summary.

Comment 29

[Brendan Eich [:brendan]]

Ok, so in a non-DEBUG build, there's no crash or related bug whatsoever?

/be

Comment 30

[timeless]

Attachment: set argv

i'm fairly certain the old code crashed for an oom case.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 139720 [details] [diff] [review]
set argv

Drive-by sr=me.

/be

Comment 32

[Kyle Yuan]

Comment on attachment 139720 [details] [diff] [review]
set argv

r=kyle

Comment 33

[Kyle Yuan]

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