Serialization of NativeRegExp prototype functions broken

NEW
Unassigned

Status

Rhino
Core
9 years ago
8 years ago

People

(Reporter: Daniel Gredler, Unassigned)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

9 years ago
Created attachment 400395 [details] [diff] [review]
Proposed Patch
(Reporter)

Comment 2

9 years ago
Created attachment 400397 [details]
Unit Test
(Reporter)

Comment 3

9 years ago
Created attachment 400400 [details]
Unit Test (version 2)

Attaching a tweaked unit test.
Attachment #400397 - Attachment is obsolete: true

Comment 4

9 years ago
Ping! What about applying this patch?

Comment 5

9 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

9 years ago
Created attachment 402058 [details] [diff] [review]
minimal patch

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

9 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.

Comment 8

9 years ago
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
You need to log in before you can comment on or make changes to this bug.