636428 - e4x: no cyclic error when an XML object's __proto__ is set to itself, potentially leading to high memory use

Status: RESOLVED DUPLICATE of bug 634150

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: valgrind log

a = <x/>
function f(o) {
    for (var j = 0; j < 10; j++) {
        o.__proto__ = a;
        o.__proto__ = a;
    }
}
f(a);

causes chunks of Valgrind errors involving use of uninitialised value of size 4, when run in a debug shell on TM changeset 725b8cce5c72 with -m and -j.

This will hang the browser when the tab displaying this script is refreshed multiple times.

s-s because I have no idea how bad this is.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The testcase should be passed into the shell as a CLI argument.

The hang in the browser can be reproduced only after multiple refreshes of the tab. The first load will not hang the browser, but about:memory will display quite a large number (~1.5Gb?) for gc heap. When one tries to refresh the tab containing the testcase in <script></script> tag a few times, the browser will hang. Tested on Fx 4 Beta 12 on 64-bit Windows 7 SP1 with 8Gb RAM.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: opt shell valgrind log

I have no idea why these logs are obtained off shells without --enable-valgrind.

The shells compiled with --enable-valgrind don't seem to show these errors, but I'm not sure why.

Comment 3

[Julian Seward [:jseward]]

V is complaining about spidermonkey's conservative stack scanner
reading uninitialised words in the stack -- harmless, but V doesn't
know that.  --enable-valgrind enables annotations in SM that tell
V to ignore those words and so gets rid of all the warnings.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> V is complaining about spidermonkey's conservative stack scanner
> reading uninitialised words in the stack -- harmless, but V doesn't
> know that.  --enable-valgrind enables annotations in SM that tell
> V to ignore those words and so gets rid of all the warnings.

So I'm guessing the Valgrind warnings are not a bug after all.

Then, is the hang (or large memory consumption) intended? (if so, this bug can be resolved quickly)

Comment 5

[Julian Seward [:jseward]]

The V warnings are harmless, unless they persist after you rebuild
with --enable-valgrind.

As for the hang / large mem consumption, I don't know.

Comment 6

[Nicholas Nethercote [inactive]]

Here's a simpler reproducer:

 a = <x/>
 for (var j = 0; j < 10; j++) {
     a.__proto__ = a;
     a.__proto__ = a;
 }   

This OOMs for me in a 32-bit shell.  Doesn't matter what combination of interpreter/JITs is used.

Setting an object's __proto__ property to itself is pretty screwy.  Indeed, if you change |a| to a non-XML object you get "TypeError: cyclic __proto__ value".

As for why this error doesn't occur for an XML object, I don't know.  Maybe it's allowed?  Or maybe it's just a bug.

Comment 7

[Brendan Eich [:brendan]]

E4X doesn't support __proto__, any access to a property goes through a custom [[Get]] and looks for XML children of the given name, returning them in a list. This continues, ad nauseum.

/be

Comment 8

[Nicholas Nethercote [inactive]]

Is there any kind of plan to deprecate e4x?

(Nb: I'm not trolling, this is a genuine question... I know very little about e4x, but I get the sense there's quite a history behind it.)

Comment 9

[Brendan Eich [:brendan]]

The plan is to self-host the runtime using proxies: bug 485791. Syntax extension machinery to allow the same to be done for the compiler is more distant.

I'm going to fix bug 485791 pretty soon, since Adobe and Mozilla are on the hook in Ecma TC39 to evolve E4X into sane future sub-specs that satisfy the use-cases better. Right now ECMA-357 references ECMA-262 Edition 3 and cannot track ES5.

/be