322499 - runtime->anynameObject + runtime->functionNamespaceObject circular dependency

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Alexander Mohr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: SpiderMonkey v. 1.6

If you do the following steps in order twice for a runtime:
1) Create new Context
2) Create object in new context with null parent and proto
3) InitStandardClasses with new object as root

it results in the root of object of the second context dependant on the AnyName function from the first context due to JS_InitClass setting the constructor and prototype properties for the second context's AnyName class construction not actually creating a new object.

This is a problem in that it makes it impossible to release all the objects from the second class immediately upon context destruction.

for more details see: http://groups.google.com/group/netscape.public.mozilla.jseng/browse_frm/thread/d34a97ee2a758dac

Reproducible: Always

Steps to Reproduce:
If you do the following steps in order twice for a runtime:
1) Create new Context
2) Create object in new context with null parent and proto
3) InitStandardClasses with new object as root

Now set the root object for the second context to NULL.
Now call JS_GC on the second context.
Actual Results:  
with GC_MARK_DEBUG enabled you'll not that the second context's Root object is dependant on a object chain starting from the AnyName function from the first context.

Expected Results:  
with GC_MARK_DEBUG enabled you should see that the Root object for the second context has been GC'd.

I've tried in my sandbox moving these two objects to the context and that fixed the problem...however Brendan tells me this will break E4X ptr based comparisons.

I'd put a note in JSRuntime for anyone who does this in the future.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Fixing the entrainment hazards is easy enough, and motivated the customization of anyname_toString.

The jsparse.c fix is to dereference * when not preceded by @ or used with ::.  Oops.  If you are used to thinking of atom-index immediate operands, remember that E4X stacks all operands, so JSOP_ANYNAME must push the anyname singleton onto the stack for the JSOP_XMLNAME to dereference.  Just don't forget to emit the latter bytecode!

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 207680 [details] [diff] [review]
fix

r=mrbkap

Comment 3

[Brendan Eich [:brendan]]

Fixed on trunk, hoping for fast shaver love to nominate for branches so I can mark this FIXED but leave its patch on radar.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 207680 [details] [diff] [review]
fix

>+     * Weak references to lazily-created, well-known XML singletons.
>+     *
>+     * NB: Singleton objects must be carefully disconnected from the rest of

"NB: These must be" would be better, IMO.  Not all
singletons must be thusly disconnected, and it's
not the singleton-ness that demands the explicit
disconnection.

sr=shaver with that.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (From update of attachment 207680 [details] [diff] [review] [edit])
> >+     * Weak references to lazily-created, well-known XML singletons.
> >+     *
> >+     * NB: Singleton objects must be carefully disconnected from the rest of
> 
> "NB: These must be" would be better, IMO.  Not all
> singletons must be thusly disconnected, and it's
> not the singleton-ness that demands the explicit
> disconnection.

No, all singleton objects mapped by the runtime and shared in arbitrary order of use by multiple contexts each with its own global and standard class objects are in need of this disconnecting treatment.  It's not just XML singletons.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 207680 [details] [diff] [review]
fix

Ah, quite.  I recant.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 207680 [details] [diff] [review]
fix

dveditz is in the loop on this one.

/be

Comment 8

[Daniel Veditz [:dveditz]]

FIXED per comment 3, need QA love.

Comment 9

[Marcia Knous [:marcia]]

QA needs a testcase in order to verify this one.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> QA needs a testcase in order to verify this one.

Bob's got the skinny.

/be

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #9)
> QA needs a testcase in order to verify this one.

It may be sufficient that AnyName is no longer defined on trunk builds.
javascript:AnyName gives undefined in today's trunk and function AnyName() { [native code] } on today's 1.5 builds on windows xp.

Comment 12

[Marcia Knous [:marcia]]

verifying fixed on the trunk per bc's comment #11, when I input that in the JS console I get "AnyName is not defined." Using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060109 Firefox/1.6a1

Comment 13

[Brendan Eich [:brendan]]

Verify the jsparse.c fix via javascript:* -- should get an error with the fix.

/be

Comment 14

[Bob Clary [:bc] (inactive)]

(In reply to comment #13)
> Verify the jsparse.c fix via javascript:* -- should get an error with the fix.

Today's trunk WinXP verified:
Error: reference to undefined XML name *
Source File: javascript:*
Line: 1

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 207680 [details] [diff] [review]
fix

a=dveditz for drivers

Comment 16

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-322499.js,v  <--  regress-322499.js

Comment 18

[Bob Clary [:bc] (inactive)]

v 20060113 windows/linux/mac 1.8.0.1, 1.8, 1.9a1