Closed
Bug 516291
Opened 15 years ago
Closed 5 years ago
Serialization of NativeRegExp prototype functions broken
Categories
(Rhino Graveyard :: Core, defect)
Tracking
(Not tracked)
RESOLVED
INACTIVE
People
(Reporter: djgredler, Unassigned)
Details
Attachments
(1 file, 3 obsolete files)
3.07 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 Build Identifier: trunk NativeRegExp.initPrototypeId(int id) uses REGEXP_TAG as the tag, but REGEXP_TAG is an Object (not serializable); REGEXP_TAG needs to be something serializable, like a String (all the other tag constants I've seen are Strings). The attached unit test demonstrates this issue. I will also attach a patch (separate from the unit test) which does two things: 1. Changes NativeRegExp.REGEXP_TAG to a String so that it is serializable and the unit test passes. 2. Changes the type of IdFunctionObject.tag from Object to Serializable so that this never happens again with future tags (this change required some cascading changes in some method parameter types and constant types). Reproducible: Always
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Comment 2•15 years ago
|
||
Reporter | ||
Comment 3•15 years ago
|
||
Attaching a tweaked unit test.
Attachment #400397 -
Attachment is obsolete: true
Comment 5•15 years ago
|
||
Thanks for the patch and test, Daniel. I think we should consider a more conservative patch that doesn't change the signature of the IdFunctionObject constructor. This is a public class, and changing it might require code that uses it to be modified or at least recompiled, and I wouldn't consider the possibility to write non-serializable functions a good enough reason for that. If we change IdFunctionObject, it should be a deliberate step and probably happen in a major release IMO.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 6•15 years ago
|
||
A minimal patch as explained in comment #5. I also extended the test case to test the regexp before and after (de)serialization.
Attachment #400395 -
Attachment is obsolete: true
Attachment #400400 -
Attachment is obsolete: true
Comment 7•15 years ago
|
||
OK, I think I get it. The thing is that the functions in RegExp.prototype should never be serialized with a regexp object, and when they do, they won't work when deserialized. For example, calling the deserialized RegExp.prototype.toString in your test case results in this: BAD FUNCTION ID=2 MASTER=/(?:)/ java.lang.IllegalArgumentException: BAD FUNCTION ID=2 MASTER=/(?:)/ at org.mozilla.javascript.IdFunctionObject.unknown(IdFunctionObject.java:186) at org.mozilla.javascript.IdScriptableObject.execIdCall(IdScriptableObject.java:597) at org.mozilla.javascript.regexp.NativeRegExp.execIdCall(NativeRegExp.java:2535) at org.mozilla.javascript.IdFunctionObject.call(IdFunctionObject.java:129) at org.mozilla.javascript.tests.NativeRegExpTest.testSerialization(NativeRegExpTest.java:46) What you should do is use the ScriptableInput/OutputStream classes in the org.mozilla.javascript.serialize package. These will make sure you're only serializing the objects you want to serialize and not your whole JS environment including all global objets. The only thing that leaves me a bit puzzled is that serialization of RegExp objects worked for me before, so I'm wondering what triggered that problem in the first place.
It seems to me that ScriptableInput/OutputStream may be useful in some special cases but that it isn't a general solution when Scriptable objects are fields of the class(es) being serialized. Is it correct?
Comment 9•14 years ago
|
||
This is a mass change. Every comment has "assigned-to-new" in it. I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Status: ASSIGNED → NEW
Comment 10•5 years ago
|
||
Closing. Bug management is now done here:
https://github.com/mozilla/rhino
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INACTIVE
You need to log in
before you can comment on or make changes to this bug.
Description
•