The static type argument to toObject is effectively never used since it always assumes that instances of String, Number and Boolean represent primitive JS values and handled via ScriptRuntime.newObject and in the rest of cases static type was not checked by WrapFactory.
Such optimization wins very little with modern JVMs if cast succeeds and produces very big overhead if cast fails. Moreover, it may prevent jits from doing more aggressive optimizations and makes class files bigger.
The change also made code in many places smaller since insanceof check take ensure that object is not null as well and with ClassCastException such check had to be done explicitly.
1. Disable invoker optimization for methods with variable number of arguments since currently to call optimized invoker a new argument array has to be allocated in any case which makes the optimization irrelevant.
2. Never modify elements of the args array in constructor, instead avoid allocation of the new argument array iff all js argument can be passed to java without type conversion.
I added Context.getApplicationClassLoader() that is now used in all
cases as a parent loader for generated classes and as the default class loader
for NativeJavaPackage. The default implementation tries to use
Thread.getContextClassLoader, but only when it is available and if Rhino
classes is available through it. Otherwise the loader for Context instance is
used. In this way if Rhino is loaded through a custom loader, it will be used,
and if Rhino classes are placed in lib/ext, Thread.getContextClassLoader still
give the application loader.
And if this default policy would not work in a particular application,
Context.getApplicationClassLoader() can be overridden to in that application.
use IdFunction.initAsConstructor to initialize Error constructors in
NativeGlobal.init and remove setFunctionType and corresponding getFunctionType in IdFunction and use a simple private boolean field there to mark functions that can be called as constructors since NativeGlobal.init was the only place that used that.
Use toObject() in ScriptRuntime.delete to convert non-Scriptable delete target to Object which required to pass Context and scope to the method and update Interpreter and optimizer/Codegen accordingly.
if (val != null && val != Undefined.instance && val instanceof Scriptable) ...
by code to generate:
if (val instanceof Scriptable && val != Undefined.instance) ...
since (val instanceof Scriptable) => (val != null)
I removed deprecated since 1.5R3 omj.ClassOutput and moved some of code from omj/ClassNameHelper.java to omj/optimizer/OptClassNameHelper so if one does not need the optimizer package, the jar will be smaller.
Initialize invoker in FunctionObject constructor, not during first call and catch possible SecurityException. In this way initialization will happen without script code on java stack which may not have permissions to create class loaders.
2. Hashtable is replaced by ObjToIntMap or UintMap to minimize memory usage.
3. Converting of strings to utf8 encoding is coded explicitly to avoid overhead of creating many objects.
2. Removal of Codegen.superClassSlashName field since Codegen.superSlashName since it is never used as a part of a signature and ClassFileWriter converts . into / in type name automatically.
Attempts to access/modify properties of null or undefined are explicitly checked to include in error messages the property name so it would be possible on error in x.y.z to know if it is x or y that was undefined or null.
Inspired by suggestion from Russell Gold.
I also added explicit flags to Parser: languageVersion and allowMemberExprAsFunctionName and set them from Context. In this way Parser can be used without Context which is useful for debugging.
1. It is not passed as a parameter to Interpreter/Codegen, instead Codegen access it directly when necessary.
2. ClassNameHelper.reset method is removed as inherently thread unsafe and data that should be used during compilation of single script is stored in Codegen itself.
3. Instead of a special DefaultClassRepository null is used to indicate that generated classes should not be stored and JavaAdapter is modified to take ClassRepository as a parameter, not ClassNameHelper.
PreorderNodeIterator iter = new PreorderNodeIterator();
for (iter.start(tree); !iter.done(); iter.next()) {
Node node = iter.getCurrent();
...
}
instead of
PreorderNodeIterator iter = tree.getPreorderIterator();
Node node;
while ((node = iter.nextNode()) != null) {
}
to allow for more flexible usage and added PreorderNodeIterator.nextSkipSubtree to skip iteration of the last visited node subtree which allows to have simple code in Optimizer.buildStatementList when iterating over statements.
The bug caused by a missed check in StmtNodeIterator.nextNode for a possible null result of findFirstInterestingNode inside the search loop which made search to stop preliminary with non-empty stack.
The changes fixe this and integrate StmtNodeIterator into
Optimizer.buildStatementList as StmtNodeIterator was used only by
buildStatementList and the new version is simpler.
The reason for the bug was that omj/optimizer/Optimizer.java when optimizing code for this[name] (see GETELEM switch, line 665) assumed a number context for GETELEM index node unconditionally which is wrong.
The fix uses number context only if [] argument is known for sure to be a number.
The bug was caused by a double call to Codegen.addNumberConstant, the first
time correctly from Codegen.visitLiteral and the second time wrongfully from
the loop in emitConstantDudeInitializers where loop index should be used
instead of calling addNumberConstant. As addNumberConstant would return the
same index for same numbers, the bug surfaces only with NaN as
addNumberConstant does not recognizes already added NaN. The bug also visible
only with optimization set to 1 or higher since only then constant folding can
produce NaN literal.
The fix removes the second call to addNumberConstant and uses
ScriptRuntime.NaNobj for NaNs.
The reason for the bug is that emitDirectConstructor generates code to call
setPrototype twice instead of setPrototype/setParentScope pair during new JS
object construction. The fix replaces that setup by a single call to
BaseFunction.createObject which is used by Interpreter as well.
Integration of LineBuffer into TokenStream code which now uses a special buffer for unreading of several chars to follow SM more closely. In this way there is no problem with a possible backtracking of 3 chars on failed attempt to match <!-- at the last minus.
TokenStream is also modified to accept a string with a source directly which avoids the need to construct intermediate StringReader in Context and allows to remove DebugReader class which is replaced by a simple function to read all Reader data into string.
Codegen.visitRegularCall should not try to apply the simple call optimization
when firstArgDone is true indicating directly called function. The patch also
replaces generation of code to call new Object[0] by loading the
ScripRuntime.emptyArgs field.
I just noticed that the changes introduced with
v1.29 of Main.java broke the ability to do hot
reloads of scripts. To be more explicit, the script
is actually reloaded but the source in the debugger
is not updated to reflect the newly loaded code.
...
Attached is a patch that restores the original behavior.
The refactorings are preserved but the handling of
previously loaded SourceInfo objects is restored and the
check for previously loaded ScriptItem instances
removed.
Have you considered adding a "Go" method to Main.java with
public visibility (same behavior as pressing the "Go" button in the debugger UI).
This would be a big help in a system where the debugger has been
embedded. Being able to close the debugger and ensure that any
breakpoints were removed and any blocked threads notified would
be a nice feature. Without this, closing the debugger can either
a) halt the application or b) destroy the debugger leaving blocked
threads in a permanent wait state. Note that the debugger is
not actually destroyed in this case because the waiting threads
prevent it from being wholly GCed.
This looks like a simple case of using the Hashtable key
instead of the value...
public void clearAllBreakpoints() {
// Igor - Use of keys() is inappropriate here. It produces
// a ClassCastException on the assignment below. The
// keys are String instances, not SourceInfo instances...
//
//Enumeration e = sourceNames.keys();
Enumeration e = sourceNames.elements();
...
}
For that I added new method createClasssLoader to Context, which by default returns new instance of DefiningClassLoader and changed the code to use this method instead of creating DefiningClassLoader directly. I moved DefiningClassLoader to org.mozilla.javascript package so core Rhino classes would not depend on org.mozilla.classfile package. I also changed SecurityController.createClasssLoader to take additional parentLoader argument to explicitly specify which class loader should be parent for generated code.
From my mail to Norris Boyd:
When considering http://bugzilla.mozilla.org/show_bug.cgi?id=166530 I realized that my 2 years old suggestion to use Thread.getContextClassLoader() in org.mozilla.classfile.DefiningClassLoader was wrong, as it does not follow class loader chain pattern Rhino embeddings can use. Moreover, it is wrong to use Thread.getContextClassLoader() when searching for Rhino classes as if Rhino is available via the system class loader and an application uses its copy from another loader, Thread.getContextClassLoader() would return incompatible class while simple Class.forName() would do proper job of loading the requested class from a loader of Class.forName() caller.
The only place where Thread.getContextClassLoader() can be useful is when searching for classes in NativeJavaPackage, but even there with a new option to use Package with an explicit class loader argument it is not necessary as one can write in a script
Packages(java.lang.Thread.contextClassLoader) to get necessary behavior.
The new SecurityController in its current form does not allow to define more then one generated class class in the same class loader effectively preventing to use optimizer which needs to define classes that refer each other and should be defined in the same loader.
To fix this I replaced the defineClass method in SecurityController by
public GeneratedClassLoader createClassLoader(Object securityDomain);
which returns instance of the new GeneratedClassLoader interface which can be used to define several classes. I also made DefiningClassLoader to implement this interface to simplify code in JavaAdapter.java and optimizer/Codegen.java.
The change removes the need to have instances of NativeFunction representing nested functions constructed before they are needed as a part of script execution.
I made ScriptRuntime.escapeString to escape \ and remove code to escape single quote ' as it is unreachable due to if (' ' <= c && c <= '~' && c != '"' && c != '\\') check as ' should not be escaped.
Patch from Marcus Crafter:
...
After speaking with Christopher Oliver, the problem seems to be a general JDK
1.4 bug that caches selected values in JComboBox, even after removeAllItems() is
called. Since its a general defect Christopher and I thought we'd report and get
it fixed in the main CVS tree.
...
Now caching of the last access slot covers GetterSlot instances as well and getter is always called for such slots
2. Fixing a potential race condition in setBySetter when a setter slot becomes an ordinary slot in response to a setter returning a value.
During execution of setBySetter a different thread can see initial null value in slot.value instead of the result of setter call as it is possible that JVM will first update slot.flags and only then slot.value for that thread. The fix replaces the old getter slot by an ordinary one under synchronized block for that I added new getSlotPosition method and updated the rest of code accordingly.
It allows to implement the DebuggableScript interface only by omj/InterpreterData instead of 2 identical implementations by InterpretedFunction and InterpretedScript.
Allow to use char sequences exceeding 64K when storing source for decompilation
The current 64K limit for string literals comes from omj/Parser.java where it constructs the internal script presentation for future decompilation. The patch extends this form to allow string sequences with more then 64K characters and modifes decompilation code in omj/NativeFunction.java accordingly.
TokenStream.java: third octal digit is a part of the octal escape in strings only if the result is <= 0377.
resources/Messages.properties: removal of unused msg.oct.esc.too.large
The change also allows with simple modifications of Context.getCurrentContext and Context.setThreadContext to use java.lang.ThreadLocal from JDK 1.2 to remove any synchronization on global data structures during Context.enter/Context.exit/Context.getCurrentContext.
#155289 - String.prototype.XXX.length has some wrong values
#155291 - RegExp properties should be DontEnum
Plus fix for matching against RegEXp captures with undefined value.
I keep getting syntax errors with no line numbers as well.
That happens when I use Context.compileReader(..) to compile the script. The
DefaultErrorReporter will throw an exception with only the message and not the
line it happened on.
It is of course easy to workaround using your own error reporter, but I've
attached a patch to add on the line and source name so the DefaultErrorReporter
gives the similar output as EcmaError if that is wanted.
> Norris Boyd wrote:
>
> Igor Bukaniv wrote:
> >
> > I am curios, why there is a need to have a special JSObject support in Rhino? Was it used for anything? The implementation in the ICEbrowser does not use it as in rare cases where conversion from JSObject to/from JS type may be needed (like calling JSObject.getWindow from a script), it seems that WrapHandler (or similar modifications to pre Rhino 1.5R2 sources) and Wrapper are enough to cover all the cases.
> Yes, we should probably just remove the JSObject code. We added it early on when Rhino was first written and we thought we might need JSObject compatibility with the JS + Java implementation in Navigator 4.x. That's not important now, so we should just remove this code (which likely doesn't work at this point anyway).
given the following object :
----------------------------------------------
function SomeObject() {}
SomeObject.prototype.exec = function() {
var local = this.someField;
}
----------------------------------------------
i create an 'instance', set a field and call the exec method :
----------------------------------------------
var someField = "global field value";
var anInstance = new SomeObject();
anInstance.someField = "instance field value";
anInstance.exec();
----------------------------------------------
then the local variable 'local' in the exec() method is assigned the value
of the global 'someField' variable instead of the instance field value.
the problem seems to be in the ScriptRuntime.callOrNewSpecial() method,
which is called, because the parser treats the name 'exec' specially. in
this method the exec() method gets called with
return call(cx, fun, thisArg, args, scope);
where the 'thisArg' parameter really is the global this value instead of
the dynamic this value, which is in the jsThis variable and which would be
the one needed...
is it legitimate to replace the above call in callOrNewSpecial() with the
following line :
return call(cx, fun, jsThis, args, scope);
this seems to only happen for methods named 'exec', which are identified as
special in the NodeTransformer.isSpecialCallName() method.
any help is appreciated. thank you very much for your time.
kind regards,
felix
The attached patch adds support for debugging eval and Function code transparently. It changes omj.NativeGlobal and omj.BaseFunction to embed line number of origin of eval and Function scripts into source name and pass 1 as base line for script code. In this way a debugger implementation can treat eval and Function code in the same way as scripts loaded from some url while giving more information about error location in case of an error in eval code as the error source would contain both line number of eval origin and line number in eval code itself.
I chose to embed line numbers via patterns like
sourcefile#<line-number>(eval)
sourcefile#<line-number>(Function)
just to be able to to pass the constructed name to URL constructor if the original sourcefile is a valid URL but it is pretty arbitrary.
I have noticed that attempting to call a java method like this:
public void foo(String foo, Serializable bar)
{
// un-important details
}
from script using foo("foo", "bar"); fails because the second argument
is not deemed coercable to Serializable. A preliminary look at the
coercion code shows that no check is made in this case with
isAssignableFrom().
The to type is only tested against StringClass and ObjectClass (non
primitive case).
(See NativeJavaObject.getConversionWeight())
I attach the patch to move away setting/quering for breakpoints from the Rhino core to application as a debugger implementation can check if a particular line has a breakpoint or not. The changes to omj/tools/debugger takes more then few lines I initially thought but they are mostly caused by refactoring to implement different view to set/query breakpoints.
The patch replaces getLineNumbers, placeBreakpoint and removeBreakpoint in DebuggableScript by getFirstLine, getEndLine and getInstructionLines where the last function fills a boolean array to indicate which script lines can ever occur in DebugFrame.onLineChange. These are read-only functions so InterpeterData are never mdofied by the debugger.
omj/tools/debugger/Main uses this information to check whether it is possible to place breakpoint at a particular line, and if possible, it sets to true entry at the boolean breakpoint array. In this way testing for break in onLineChange is simple and fast as it just needs to check if breakpoint array holds true at the given line number position.
1. Targeting of labels with 0x80000000 biscuits is moved from LabelTable to ClassFileWriter as this is classfile specific and is not necessary in Interpreter.
2. LabelTable allows for pc > Short.MAX_VALUE as this restriction is classfile specific. The only requirement is for jump offsets to stay within short range.
3. LabelTable is made private member of Interpreter and ClassFileWriter instead of being classes' superclass to avoid API leakage that forced optimizer.Codegen to declare few utility methods public as they got the same signature as LabelTable methods visible throw Interpreter inheritance.
1. Replacing omj.debug.Debugger.enterFrame() by omj.debug.Debugger.getFrame() and omj.debug.DebugFrame.onEnter() to allow to return null from omj.debug.Debugger.getFrame to enable full optimization with debugger set if it is not interested in monitoring a particular frame
2. Changing type for the source argument in omj.debug.Debugger.handleCompilationDone from StringBuffer to String as Debugger instances should not be able to modify source even by chance.
The patch uses the special NOT_FOUND value to flag deleted indexes. It also
make sure that original array object passed to Function.call is not modified,
as all changes goes to cloned copy. It is not necessary for the fix, but it is
the only place in the current Rhino that can alter Object[] array passed to
Function.call and I think it is better to remove this exceptional case.
igor%mir2.org
nboyd%atg.com
rogerl%netscape.com