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

RESOLVED FIXED in Future

Status

P3
critical
RESOLVED FIXED
17 years ago
8 years ago

People

(Reporter: xiaobin.lu, Assigned: timeless)

Tracking

({crash})

Trunk
Future
Sun
Solaris
crash

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Comment 1

17 years ago
Created attachment 78988 [details] [diff] [review]
Check the agrc before calling JS_Malloc
(Reporter)

Comment 2

17 years ago
Please review the patch!

Comment 3

17 years ago
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 -
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, patch
Summary: Mozilla crashes when JSObject.call("func", agr) amd agr = null → Mozilla crashes when JSObject.call("func", agr) and agr = null
(Reporter)

Comment 4

17 years ago
/**
* <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

17 years ago
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
(Reporter)

Comment 6

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

(Reporter)

Comment 7

17 years ago
Joe, can you get my fix reviewed and super reviewed, after that you can check in
that fix. Thanks!
Assignee: rogerl → joe.chou
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.
(Reporter)

Comment 9

17 years ago
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

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

Comment 11

17 years ago
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

17 years ago
Accepting the bug.
Status: NEW → ASSIGNED

Comment 13

17 years ago
Created attachment 79372 [details] [diff] [review]
Patch to skip abort after assertion in debug
Attachment #78988 - Attachment is obsolete: true
(Assignee)

Comment 14

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Xiaobin, why this is a blocker? It seems to be only affecting debug builds,
aborted, but not crashed.
(Reporter)

Updated

17 years ago
Severity: blocker → critical
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
Attachment #79372 - Flags: needs-work+
(Reporter)

Comment 19

17 years ago
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

17 years ago
r=beard on guarding against passing 0 to JS_malloc. Can we see a patch, please?
(Reporter)

Comment 21

17 years ago
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

16 years ago
what's the status on this patch ?

Comment 23

16 years ago
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!)
Assignee: joe.chou → beard
Status: ASSIGNED → NEW

Comment 24

16 years ago
->peterl
Assignee: beard → peterl

Comment 25

16 years ago
cc:ing OJI module owner
Priority: -- → P3
Target Milestone: --- → Future

Updated

15 years ago
Attachment #79372 - Attachment is obsolete: true

Comment 26

15 years ago
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=?
Attachment #78988 - Attachment is obsolete: false
Attachment #78988 - Flags: superreview?(brendan)
Attachment #78988 - Flags: review+
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

15 years ago
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.
Summary: Mozilla crashes when JSObject.call("func", agr) and agr = null → Mozilla aborts when JSObject.call("func", agr) and agr is zero-length array
Ok, so in a non-DEBUG build, there's no crash or related bug whatsoever?

/be
(Assignee)

Comment 30

15 years ago
Created attachment 139720 [details] [diff] [review]
set argv

i'm fairly certain the old code crashed for an oom case.
Attachment #78988 - Attachment is obsolete: true
Comment on attachment 139720 [details] [diff] [review]
set argv

Drive-by sr=me.

/be
Attachment #139720 - Flags: superreview+

Comment 32

15 years ago
Comment on attachment 139720 [details] [diff] [review]
set argv

r=kyle
Attachment #139720 - Flags: review+

Updated

15 years ago
Attachment #78988 - Flags: superreview?(brendan)
(Assignee)

Updated

15 years ago
Assignee: peterl-bugs → timeless
Summary: Mozilla aborts when JSObject.call("func", agr) and agr is zero-length array → Debug Mozilla aborts when JSObject.call("func", args) and args is zero-length array
(Assignee)

Updated

15 years ago
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 33

15 years ago
*** Bug 108369 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

8 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.