Closed Bug 516291 Opened 15 years ago Closed 5 years ago

Serialization of NativeRegExp prototype functions broken

Categories

(Rhino Graveyard :: Core, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INACTIVE

People

(Reporter: djgredler, Unassigned)

Details

Attachments

(1 file, 3 obsolete files)

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
Attached patch Proposed Patch (obsolete) — Splinter Review
Attached file Unit Test (obsolete) —
Attached file Unit Test (version 2) (obsolete) —
Attaching a tweaked unit test.
Attachment #400397 - Attachment is obsolete: true
Ping! What about applying this patch?
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
Attached patch minimal patchSplinter Review
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
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?
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

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.

Attachment

General

Creator:
Created:
Updated:
Size: