Closed Bug 374918 Opened 18 years ago Closed 16 years ago

String primitive prototype wrongly resolved when used with many top scopes

Categories

(Rhino Graveyard :: Core, defect)

Product:
Rhino Graveyard
Component:
Core
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mguillemot, Assigned: norrisboyd)

Details

Attachments

(2 files, 3 obsolete files)

Java test class
785 bytes, text/x-cvsweb-markup
Details
Patch fixing primitive prototype problem as currently occuring in HtmlUnit
12.34 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2) Gecko/20060601 Firefox/2.0.0.2 (Ubuntu-edgy) Build Identifier: the protoype of a string primitive is always resolved to the one in the context's top call scope rather than to the one in the executing code's top scope. The provided unit test illustrates the problem. I suppose that ScriptRuntime.toObjectOrNull shouldn't use getTopCallScope(cx) but look at cx.lastInterpreterFrame.scope. The change can't be done directly as CallFrame is a private class of Interpreter. If it's ok to make changes like removing the private modifier of CallFrame I can look at providing a patch. Reproducible: Always Steps to Reproduce: public void testStringPrimitivePrototypeScope() { Context cx = Context.enter(); Scriptable scope1 = cx.initStandardObjects(); Scriptable scope2 = cx.initStandardObjects(); String str2 = "function f() { String.prototype.foo = 'from 2'; \n" + "var s1 = new String('s1');\n" + "if (s1.foo != 'from 2') throw 's1 got: ' + s1.foo;\n" // works + "var s2 = 's2';\n" + "if (s2.foo != 'from 2') throw 's2 got: ' + s2.foo;\n" // fails + "}"; cx.evaluateString(scope2, str2, "source2", 1, null); scope1.put("scope2", scope1, scope2); String str1 = "String.prototype.foo = 'from 1'; scope2.f()"; cx.evaluateString(scope1, str1, "source1", 1, null); Context.exit(); }
Yep, it seems right -- ScriptRuntime.toObjectOrNull() should have a "Scriptable scope" parameter that it passes to ScriptRuntime.toObject() instead of defaulting to getTopCallScope(cx). Problem is, ScriptRuntime.toObjectOrNull() is called from no less than 18 locations, so reviewing the possible side-effects of the fix will take a while. Will ask Norris if he can comment on it...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → 1.6R6
Sorry to take a while to respond. I dug up the original change that introduced the topCallScope changes: -------- date: 2004/08/09 11:00:46; author: igor%mir2.org; state: Exp; lines: +1 -4 Top call scope tracking changes: Since E4X implementation needs to know the activation scope for tracking of default namespaces, previously an elaborated schema was added to set/restore the activation scope which relied on the fact that scrip and function with activation record should always call special entry/exit functions. But that does not work for functions without activation records since they never call any special entry/exit pairs. So if application call such function directly, the function would not store its top scope anywhere and the E4X subsystem would not be able to get E4X library object. The patch fixes with introduction of 2 functions, hasTopCall and doTopCall to ScriptRuntime and adding the following code prefix to each implementation of Callable.call that can start execution of script code: public Object call(Context cx, Scriptable scope, Scriptable thisObj, Object[] args) throws JavaScriptException { // Prefix start if (!ScriptRuntime.hasTopCall(cx)) { return ScriptRuntime.doTopCall(this, cx, scope, thisObj, args); } // Prefix end ... In this way there is always registered top scope during script execution and the previous elaborated schema became unnecessary so I reverted that part to almost pre-E4x state. ---------- This change log brings up the question of whether changing ScriptRuntime.toObjectOrNull would break some of the E4X implementation. Frankly I don't know much about E4X so I can't comment. The problem with the proposed change of using cx.lastInterpreterFrame.scope is that it doesn't work for compiled mode. It's not clear, Attila, if you were intending to use lastInterpreterFrame or not. I frankly didn't intend for two sets of scopes initialized with initStandardObjects to ever be mixed in the same JS object graph. It's certainly non-ECMA since those objects are defined to be global by ECMA. What's the use case?
(In reply to comment #2) > The problem with the proposed change of using cx.lastInterpreterFrame.scope is > that it doesn't work for compiled mode. It's not clear, Attila, if you were > intending to use lastInterpreterFrame or not. No - that's what the submitter of the bug proposed, but since that wouldn't work in compiled code I specifically intended to change the signature of toObjectOrNull() so it works uniformly regardless of execution mode. > > I frankly didn't intend for two sets of scopes initialized with > initStandardObjects to ever be mixed in the same JS object graph. It's > certainly non-ECMA since those objects are defined to be global by ECMA. What's > the use case? > Not sure. You're right in saying this is quite unintended use of the API, so we might want to avoid jumping the gun. Marc, can you provide us with a real-world use case for this?
In HtmlUnit we simulate the browser(s) behaviour and in this case you have one scope per (html) page but one page can be accessed from an other one (for instance frames). The provided unit case illustrates such a problem we had making the JavaScript generated by Google WebToolkit working with HtmlUnit.
This seems like an important case. Attila's approach should work, but it does look like a lot of plumbing to get all the scope values passed around appropriately. Looking at the E4X default namespace implementation, I see ScriptRuntime.setDefaultNamespace and searchDefaultNamespace. From reading this code I think the problem of multiple top-level scopes would affect this code as well.
Just out of curiosity, anyone has any idea how does this work in browsers with cross-page scripting within a frame? I.e. Mark, how would typical browsers (I always have to smile at this, since I'm using a *very* nontypical one - Opera on Mac OS X :-) ) run a browser-adapted version of your above testcase?
You mean how they handle the scopes? No idea but this is a current practice for js frameworks to play with these multiple scopes (even in Opera). With multiple scopes I have an other issue with eval that I will first bring to the mailing list.
I took another look at this. I don't see any way to fix this short of redoing a lot of deep plumbing that also has impacts on E4X stuff I don't understand well. I think we should punt on this for the soon-to-be-released Rhino 1.6R6 and look at it again when we have more time to shake out bugs.
Does the time come where you have "more time to shake out bugs"? ;-)
Attached file js1_7/regress/regress-414553.js (obsolete) —
Proposed regression test
Comment on attachment 300079 [details] js1_7/regress/regress-414553.js Attached to wrong bug, sorry.
Attachment #300079 - Attachment is obsolete: true
FYI, this appears to apply to Number.prototype, as well.
Any progress on this issue? It seems that our workaround (a hack) doesn't work anymore with current Rhino from CVS, meaning that we'll have problem to benefit from the progress made in Rhino.
This patch fixes this bug and contains unit tests demonstrating it both in compiled and interpreted modes. I guess that other places than the ones fixed should make usage of the new methods to get object properties that correctly resolve primitive prototypes, but I'm not able to judge it. The test suite junit-all causes a lot of error when I run it (with or without this patch), therefore I can't get any information from it.
Can a Rhino committer apply this patch (if it considered as good) or post a comment (if there is a problem with it)?
In fact the problem is more important than I found initially and my patch fixes only one case. Following example shows the problem: final Context cx = ContextFactory.getGlobal().enterContext(); final Scriptable scope1 = cx.initStandardObjects(); final Scriptable scope2 = cx.initStandardObjects(); String scriptScope1 = "String.prototype.foo = function() { return 'from 2'; }\n" + "var s2 = 's2';\n"; String scriptScope2 = "var s2Foo = scope2.s2.foo();\n" + "if (s2Foo != 'from 2') throw 's2 got: ' + s2Foo;\n"; // fails cx.evaluateString(scope2, scriptScope2, "source2", 1, null); scope1.put("scope2", scope1, scope2); cx.evaluateString(scope1, scriptScope1, "source1", 1, null); Context.exit(); The JS code fails because no function foo can be found in s2. My previous patch doesn't help here because the current scope when the function is called is still scope1. This means that a String (or any primitive) can't be saved in a Slot just as java.lang.String but must be wrapped to save the (top?) scope of its creation. I can work on a patch for that, but I'd like first feedback from some Rhino committer.
Attachment #338310 - Attachment is patch: true
Attached file Java test class —
Attaching a simplified version of your test code. As I expected, String and number prototypes are consistent across scopes when wrapped in JS objects via String() and Number constructors. So I guess a viable strategy for implementing cross-scope safety would be to introduce a mode where primitives are always wrapped in objects.
Primitive aren't objects and therefore wrapping them in JS objects would introduce a lot of failures. Your test (in fact it's not a unit test) doesn't test exactly the same thing and only test for the default optimization level. In fact the number of cases to handle is far higher than illustrated by this test: primitive have to be wrapped as well before to get assigned to a property, before to be transmitted as arg to a function, when returned by a function, in array constructor, in object constructor, ... I'll attach the test cases with which I'm working.
The attached tests cover different cases where the scope need to be saved together with the primitive value. The last 3 tests pass with current code base and are just here to ensure that modification to primitives don't break basic funtionalities. Note that it is important to consider special numbers 0 and 1 as they are handled differently than other values.
Comment on attachment 338310 [details] [diff] [review] Patch fixinig this bug in primitive prototype resolution obsolete because incomplete as explained in a previous comment
Attachment #338310 - Attachment is obsolete: true
Does everyone still agree that my original idea of modifying ScriptRuntime.toObjectOrNull signature to take a "Scriptable topLevelScope" argument, and then propagating the change through the code is the way to go?
I don't agree. This was what I've done in a first time but this is not enough: you need to know which scope has to be taken and this is not necessarily the current one. To be more precise, it is not possible to know which scope it is if it hasn't be saved together with the primitive. Just think of a primitive that is placed as property of some object (or of the top scope) and that is used later but accessed from an other scope. The attached tests handle this case too.
Comment on attachment 347736 [details] Set of unit tests illustrating different cases that need to be handle OOPS, these requirements go far beyond what browsers do. Sorry. Marking as obsolete. More later.
Attachment #347736 - Attachment is obsolete: true
Adding a ScriptRuntime.toObjectOrNull signature with a scope argument might be a good idea for itself for those cases where a scope is already available, which is the case in a few usages of that method, even if it doesn't fix this problem. As for this bug, as Marc said it looks like we need to store the prototype reference (I think it's the prototype and not actually the scope that matters) with primitive values. It's rather obvious to me that the best way to do this is to always store numbers, strings and booleans using Rhino's NativeNumber, NativeString, and NativeBoolean wrapper classes. Primitives are converted to objects a lot anyway (each time you invoke a method on a string, for example), so primitives and their object counterparts really look and behave more like the same thing anyway. (I remember there was a time when primitives weren't converted to objects automatically when calling a method on them - can anybody remember what language version that was and if it still matters?) As for the differences between primitives and object wrappers, the best way to handle this would probably be to use subclasses for one or the other, so the (JS) typeof or instanceof operators could look for that using a simple (Java) instanceof. Any other difference between primitives and object wrappers that we'd need to take care of? Since I guess storing prototype/parent scope info with each primitive would likely result in decreased performance and increased memory usage and most embeddings would not need it, I think it might be wise to implement this as Context feature which is disabled by default, but we can look at that when we have a working patch. It's also possible the spared conversion between primitives and object wrappers helps performance in real world scenarios. What do you think?
(In reply to comment #24) > Adding a ScriptRuntime.toObjectOrNull signature with a scope argument might be > a good idea for itself for those cases where a scope is already available, > which is the case in a few usages of that method, even if it doesn't fix this > problem. Right. > As for this bug, as Marc said it looks like we need to store the prototype > reference (I think it's the prototype and not actually the scope that matters) > with primitive values. It's rather obvious to me that the best way to do this > is to always store numbers, strings and booleans using Rhino's NativeNumber, > NativeString, and NativeBoolean wrapper classes. Primitives are converted to > objects a lot anyway (each time you invoke a method on a string, for example), > so primitives and their object counterparts really look and behave more like > the same thing anyway. They don't. Consider these examples: if(false) { ... } if(new Boolean(false)) { ... } if("") { ... } if(new String("")) { ... } if(0) { ... } if(new Number(0)) { ... } > Since I guess storing prototype/parent scope info with each primitive would > likely result in decreased performance and increased memory usage and most > embeddings would not need it, I think it might be wise to implement this as > Context feature which is disabled by default, but we can look at that when we > have a working patch. It's also possible the spared conversion between > primitives and object wrappers helps performance in real world scenarios. > > What do you think? I'm getting to a point where I'm feeling anxious about this whole thing. As an ol' rule of thumb, if the architectural complexity starts to look too hairy, that's usually a good indication we're heading in the wrong direction. And it starts to look hairy to me. Marc originally said the problem is that "the protoype of a string primitive is always resolved to the one in the context's top call scope rather than to the one in the executing code's top scope." From my understanding of that problem statement, if the "top scope of the executing code" were always passed to the toObjectOrNull, instead of relying on a thread local (current context's current top level scope), that'd solve the problem by breaking the assumption that any scope that's active within a Context ultimately has getCurrentContext().getTopLevelScope() as its ancestor. That's not the same as tracking the "prototype of the primitive". Note that this concept is actually not even well defined. A better defined one would be "prototype of the corresponding object type for a primitive as per active scope at the point of the primitive creation". Which promptly raises another question: when is a primitive created? When is boolean literal true, or number 42 created? Does evaluation of an expression, i.e. (i == 42) result in a new instance of true/false being created, or does it return an intrinsic true/false value? The problem is that not every occurrence of a primitive value is a separate instance. You can't even really talk about primitive values being instances. Finally, The semantics of sharing objects between Contexts that use different global standard object instances is totally out of the ECMA spec, and if allowed, is completely implementation dependent. From point of view of the spec, these are two different programs. Therefore, if we want to embrace this, we need to make sure that we get it right and don't shoot ourselves in the foot (or make it easy for other people to do so). Honestly, I'm still fairly anxious about breaking the invarant stating that "any scope that's active within a Context ultimately has getCurrentContext().getTopLevelScope() as its ancestor". On the mailing list, I suggested Marc to try a different solution using a shared scope for the frameset HTML, with all standard objects in it, and use separate scopes for individual frames, with each frame-scope setting the shared scope as its prototype. I believe that'd make his use case work, and we wouldn't need to change anything.
Here is the smallest patch that: - illustrates 2 cases where Rhino behaviour is wrong - fixes these 2 cases passing a scope parameter to a new ScriptRuntime.toObjectOrNull method I haven't changed the methods calling ScriptRuntime.toObjectOrNull without scope parameter when my unit test doesn't show the need for it. This makes this patch smaller and should increase the chance that it gets applied quickly.
I've followed a wrong way for a far too long time (in fact this bug is really too old). I apologize for the time you've lost trying to understand my incorrect requirements. I don't know at which point I've made a wrong test in my browser that gave me an incorrect understanding of the situation, but the fact is that the situation is far easier to handle than I feared (see comment#26). Attila was mostly right (not concerning frames and shared scope but this is an other point that I will answer on the mailing list): NO information about the "origin" scope needs to be saved, only the current top scope matters (which means that for the same variable x, x.foo may evaluate to different values depending on the scope where the call is performed).
Last note: reworking the code base to use PrimitiveXxxx to store primitives (what is not needed, see previous comment), I had to perform different refactorings of Rhino code base. If there is interest for it, I can extract 2 patches that: - use a "Stack" object to hide the Object[] and double[] in Interpreter.java - add methods isPrimitiveXxxx() in ScriptRuntime and use them instead of direct instanceof tests I think that these changes make the code more readable, easier to debug and the first one can make work on "death by double" easier. Let me know if you would welcome such changes.
I've patched in the patch from #26. It seems like a fine change to make. I tweaked one part of the change to reduce copied code. The resulting change passes the regression tests. I'm fine with accepting this change; any objections from others?
Fine with me. One place that should also call the new ScriptRuntime.toObjectOrNull with scope argument is in FunctionObject.java at line 238. That code just changed to ignore the local scope variable in the fix to bug 461138 a few days ago.
(In reply to comment #30) > Fine with me. One place that should also call the new > ScriptRuntime.toObjectOrNull with scope argument is in FunctionObject.java at > line 238. That code just changed to ignore the local scope variable in the fix > to bug 461138 a few days ago. Thanks, I made that change and reran the unit test for bug 461138 to ensure that the fix didn't regress that issue.
(In reply to comment #30) > Fine with me. One place that should also call the new > ScriptRuntime.toObjectOrNull with scope argument is in FunctionObject.java at > line 238. That code just changed to ignore the local scope variable in the fix > to bug 461138 a few days ago. Hannes, can you provide a unit test that shows the need for it? I thought too in a first time that changes needed to be made at many other places but as long as this need it not demonstrated by unit tests, I believe that it is better not to change it.
(In reply to comment #32) > (In reply to comment #30) > > Fine with me. One place that should also call the new > > ScriptRuntime.toObjectOrNull with scope argument is in FunctionObject.java at > > line 238. That code just changed to ignore the local scope variable in the fix > > to bug 461138 a few days ago. > > Hannes, > > can you provide a unit test that shows the need for it? I thought too in a > first time that changes needed to be made at many other places but as long as > this need it not demonstrated by unit tests, I believe that it is better not to > change it. Nope, but using the new toObjectOrNull() signature just reestablishes the state before the fix for bug 461138 - calling toObject() with the available scope object as argument. So I think your argument works in the other direction - unless we have real evidence, we should go back to the behaviour we had for the past several years.
The change is indeed needed, but invoking that it is the case because this is the behaviour that we had for the past several years is... hmm, not very constructive. This is a very special case where a FunctionObject takes a Scriptable as parameter, gets invoked with a primitive and wants to look at properties of the prototype. No idea if this ever gets used. No matter, this should be tested and fixed. Here is the test case showing the need for this: public static class MyObject extends ScriptableObject { @Override public String getClassName() { return "MyObject"; } public Object readPropFoo(final Scriptable s) { return ScriptableObject.getProperty(s, "foo"); } } /** * Test that FunctionObject use the right top scope to convert a primitive to an object */ @Test public void functionObjectPrimitiveToObject() throws Exception { final String scriptScope2 = "function f() { String.prototype.foo = 'from 2'; \n" + "var s2 = 's2';\n" + "var s2Foo = s2.foo;\n" + "var s2FooReadByFunction = myObject.readPropFoo(s2);\n" + "if (s2Foo != s2FooReadByFunction) throw 's2 got: ' + s2FooReadByFunction;\n" + "}"; // define object with custom method final MyObject myObject = new MyObject(); final String[] functionNames = { "readPropFoo" }; myObject.defineFunctionProperties(functionNames, MyObject.class, ScriptableObject.EMPTY); final String scriptScope1 = "String.prototype.foo = 'from 1'; scope2.f()"; final ContextAction action = new ContextAction() { public Object run(final Context cx) { final Scriptable scope1 = cx.initStandardObjects(new MySimpleScriptableObject("scope1")); final Scriptable scope2 = cx.initStandardObjects(new MySimpleScriptableObject("scope2")); scope2.put("myObject", scope2, myObject); cx.evaluateString(scope2, scriptScope2, "source2", 1, null); scope1.put("scope2", scope1, scope2); return cx.evaluateString(scope1, scriptScope1, "source1", 1, null); } }; Utils.runWithAllOptimizationLevels(action); }
Well, if your test backs my assumption i guess it's all the better. Thanks Marc!
Thanks, I added the extra unit test code and have submitted the change to the branch and CVS head.
Assignee: nobody → norrisboyd
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Cool! @Norris: - in comment #39 you write that "The resulting change passes the regression tests". How do you run the tests? For me there are still a large number of failing standard tests and this makes very difficult to know whether changes introduce regression or not. The build servers seems to have the same results as I have. - answers to comment #28?
(In reply to comment #37) > Cool! > > @Norris: > - in comment #39 you write that "The resulting change passes the regression > tests". How do you run the tests? For me there are still a large number of > failing standard tests and this makes very difficult to know whether changes > introduce regression or not. The build servers seems to have the same results > as I have. First, everything passes for me off of Rhino1_7R2_BRANCH. There are some failures on CVS HEAD due to problems introduced by the AST API. I'm running the tests using the approach in https://developer.mozilla.org/en/Running_the_Rhino_tests, and then running the JUnit tests from Eclipse. I just checked for new tests in mozilla/js/tests, and there were 16 new tests that fail. I'll update the skip list for these. I think you are right when you said in a separate thread that we should move to a list of tests to run rather than a list of tests to skip to avoid this problem. > > - answers to comment #28? - use a "Stack" object to hide the Object[] and double[] in Interpreter.java I'm worried that the performance of a stack object will be less than the direct reference to the array objects. - add methods isPrimitiveXxxx() in ScriptRuntime and use them instead of direct instanceof tests Again, worried about performance impacts, although less so than the first. I suppose a good JVM will inline the methods.
Norris, I've tried to run the tests as you do and I didn't get any error either. I think that there is a major problem here and I have opened a special issue for that: https://bugzilla.mozilla.org/show_bug.cgi?id=464898 Concerning the performance. I couldn't notice any significant difference while running all the tests locally with a version of Rhino that had not only these 2 changes, but that used new classes to save JavaScript primitives. It doesn't really surprise me: modern JVM are good enough to optimize execution of such code. What would be for you a sufficient proof that the performance is not impacted?
Thanks to Marc and all of the committers for looking into this!!
(In reply to comment #39) > Norris, > > I've tried to run the tests as you do and I didn't get any error either. I > think that there is a major problem here and I have opened a special issue for > that: > https://bugzilla.mozilla.org/show_bug.cgi?id=464898 > > Concerning the performance. I couldn't notice any significant difference while > running all the tests locally with a version of Rhino that had not only these 2 > changes, but that used new classes to save JavaScript primitives. It doesn't > really surprise me: modern JVM are good enough to optimize execution of such > code. > > What would be for you a sufficient proof that the performance is not impacted? I've been using the benchmarks from http://ejohn.org/projects/javascript-engine-speeds/ to test performance. If there's noticeable change in those, then I think we're fine. Note I'm normally not so persnickety about things, but the inner loop of the interpreter is a real hotspot, and even just adding code that pushes us over certain codesize limits can make Rhino interpreter 2X slower.
(In reply to comment #41) > ... > I've been using the benchmarks from > http://ejohn.org/projects/javascript-engine-speeds/ to test performance. If > there's noticeable change in those, then I think we're fine. Ok, I'll try it. Do you know where the whole sources of these tests can be downloaded at once? Many links are now broken.
Target Milestone: 1.6R6 → ---
Hmm. You're right that these benchmarks don't seem to be available anymore. I'll mail you a zip file of what I have saved.
John Resig is subscribed to the Rhino mailing list -- maybe just ask him where did the tests go?
I already mailed him this morning but no answer until now (in fact I think that he is subscribed to the bugzilla issues too).
I had just mailed him as well.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: