319614 - Sealing breaks lazy loading

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect, P5)

Description

[Michael Lipp]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc4 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.7.12) Gecko/20050922 Fedora/1.0.7-1.1.fc4 Firefox/1.0.7

As described in the tutorial, I have created my ScriptableObject standardsObject. Then I have created a new ScriptableObject, set the standardsObject as prototype and the parent as null. Now I seal.

When my script tries to access java (java.lang.System.out.println...) I get an exception because Rhino tries to (lazily) create the "java" property (although conceptually it is predefined, i.e. already there) in a sealed context.

I might work around this by creating the "java" property before sealing. Does anybody know how I can do this?


Reproducible: Always

Comment 1

[Michael Lipp]

It's rhino 1.6r2.

Comment 2

[A. Hill]

I came across this issue when playing with serialization and sealed scopes in Rhino 1.6 release 2 2005 09 19.

Attempting to serialize objects when you have a sealded scope also fails because RegExp wasnt loaded and it therefore tries to load it at serialization time. (btw: Serialization in 1.6R2 will also fail because of a different problem with XML objects as documented in bug 323054 - interestingly you get a different exception in relation that bug when the top scope is sealed.)

Based on what I read in the post:
http://groups.google.com/group/netscape.public.mozilla.jseng/browse_thread/thread/2a21bd205a960a6e/d4e48e157697f9d2?q=scopes&rnum=3#d4e48e157697f9d2
I discovered that if you 'mention' the object before sealing the top scope it will force it to be loaded for example evaluating something like
var forceLoad=java;
before sealing the scope is a workaround.

Comment 3

[A. Hill]

Attachment: Zip that contains code to demonstrate issue

This zip contains code that demonstrates the issue & workaround with 3 scenarios. In each scenario an attempt is made to use 'java' object, and then code in java tries to serialise a function from a scope below the sealed one.

In scenario 1 the workaround I mentioned is employed so no errors are shown.

The second shows the error when there is no workaround for accesing java and RegExp.

(The third shows what happens when we dont create dummy XML objects to get around bug 323054. Could that bug be related to this in some way?)

The output I got from running the example code (on java 1.4.2_06 on Windows) is also included in a txt file.

Comment 4

[Igor Bukanov]

Reassigning to [e-mail] pending resolution of bug 288433

Comment 5

[Attila Szegedi]

I'm suffering from this as well, but there's no easy way to get around it. We could define a separate mutation operation that replaces an object with a semantically identical another one, but again, it'd open tricky backdoors and defeat the purpose of sealing as the software couldn't really figure out that the replacement object is semantically identical to the replaced one.

For now, I'm working around it by serializing the topmost (shared, therefore instantiated only once) scope into a ByteArrayOutputStream (a "/dev/null" type OutputStream would suffice too :-) ), thus triggering loading of all lazy objects. 

As I said, I'm affected by this as well, but still I'm inclined to mark this a "Won't fix" - sealing is a feature that if you live with, has such side effects, and you should be prepared for them.

Comment 6

[Michael Lipp]

Serializing the scope to cause loading is a nice idea, but it is a hack. If this is the only way to achieve the desired effect without opening security holes, sealObject() should do it automatically.

I don't think that a user should have to live with the problem. Lazy loading is a purely internal optimization of the interpreter, it should not change observable behaviour.

Comment 7

[A. Hill]

+1

"Wont fix - because its difficult and high risk but low priority and theres a workaround" is one thing and justifiable, but "wont fix - cos 'stuff happens' so live with it" is just poor form mate.

I mean while we are at it why not just mark ALL the bugs "Won't fix" - after all Rhino is a scripting implementation that if you live with, has lots of bugs, and you should be prepared for them.

And if you are going to leave it unfixed, then the distributions documentation should indicate the issue clearly as well as the workaround and the reasons for not fixing it. People shouldnt have to spend hours wondering what they did wrong before they finally stumble across this bug report here in Bugzilla.

btw: Is there a way to disable lazy loading altogether?

Comment 8

[Attila Szegedi]

(In reply to comment #7)
> +1
> 
> "Wont fix - because its difficult and high risk but low priority and theres a
> workaround" is one thing and justifiable, but "wont fix - cos 'stuff happens'
> so live with it" is just poor form mate.

It's not "stuff happens", but rather "conflicting requirements" leading to a code implementation that produces likewise "conflicting behavior". Note that I'm not defending the status quo, merely explaining it.

> I mean while we are at it why not just mark ALL the bugs "Won't fix" - after
> all Rhino is a scripting implementation that if you live with, has lots of
> bugs, and you should be prepared for them.

Okay, you made your point. Anyway, when I said that I'm inclined towards not fixing it, I didn't say I'd refuse any fixes. I don't have time to fix this at the moment, but I'll gladly review any patch that you submit and commit it if it is good. I'm not trigger-happy to mark fixable issues that people really need as "won't fix", so expect it to remain open for a good while.

Michael's proposal for triggering construction of any lazily-loaded objects from sealObject() would be a working idea. Making the lazy loading optional is an even better idea.

> 
> And if you are going to leave it unfixed, then the distributions documentation
> should indicate the issue clearly as well as the workaround and the reasons for
> not fixing it. People shouldnt have to spend hours wondering what they did
> wrong before they finally stumble across this bug report here in Bugzilla.
> 
> btw: Is there a way to disable lazy loading altogether?
> 

Not that I know of.

Comment 9

[Joshua Levy]

Just ran into this too, in the case of E4X.

I noticed that if I sealed a scope, then did
  context.evaluateString(scope, "<foo/>", "<cmd>", 1, null)
I'd get the
  "Cannot modify a property of a sealed object: XML."
exception.

But more problematically, if you do than make the same call
again,
   context.evaluateString(scope, "<foo/>", "<cmd>", 1, null)
Rhino hangs in an infinite recursion involving
LazilyLoadedCtor.getProperty().

(This is with today's CVS build.)

As it sounds this won't be fixed soon, might I suggest
a note about this issue on the "sealed scopes" section of
the docs?
  http://www.mozilla.org/rhino/scopes.html
It took me a while to figure out what was wrong; this
might save others some time...

Comment 10

[Norris Boyd]

Changing priority to P5 based on recent bug triage.

Comment 11

[Hannes Wallnoefer]

Attachment: initialize ctors before sealing

Looping through property slots and initializing all LazilyLoadedCtors to fix this problem.

Comment 13

[Hannes Wallnoefer]

Attila: I was trying to understand why you hesitated to solve this bug one way or other. IMO losing the benefit of lazy constructor loading is an acceptable compromise when object sealing is used so I committed my patch. Please let me know if you disagree. 

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.138; previous revision: 1.137
done

Comment 14

[Attila Szegedi]

(In reply to comment #13)
> Attila: I was trying to understand why you hesitated to solve this bug one way
> or other.

I just didn't have time to do it. I said that I'd welcome a solution, specifically "Michael's proposal for triggering construction of any lazily-loaded objects
from sealObject() would be a working idea." in comment #8.

I'm actually fairly relieved that this is fixed finally - a big thank you :-) (for all the other hard work in Bugzilla over the past few days, too.)