440834 - do not cache enumerators when object shape overflowed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

This came up in bug 419091 comment 17 through 21.

If a program manages to overflow rt->shapeGen, SpiderMonkey should disable the property cache (runtime-wide) and run without it until such time as shapes are GC'd to reduce the total number of shapes to less 2**24.

Currently the property cache is permanently disabled if this happens, and GC starts happening very frequently (from js_GenerateShape).

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: stacktrace of assertion failure

Comment 2

[Dana Keeler (she/her) [:keeler]]

Attachment: very slow testcase

This is the original, very slow test case (it takes ~1 hour on my machine).  I'm working on a faster one.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Attachment: patch to make overflow happen faster

with this patch, the very slow test case only takes a few seconds

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> Created an attachment (id=398048) [details]
> stacktrace of assertion failure

The stack indicates that the native enumerators should not be cached when shape overflows.

Comment 5

[Igor Bukanov]

Attachment: fix v1

Comment 6

[Igor Bukanov]

Attachment: fix v2

In the previous patch I missed another case of storing overflown shape into the cache.

Comment 7

[Igor Bukanov]

I mark this bug as restrictive for now. It could be used to change the enumeration property list for for-in loops via caching with overflown shape some preset lists potentially leading to XSS and other attacks.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 399114 [details] [diff] [review]
fix v2

>@@ -5028,17 +5031,23 @@ js_Enumerate(JSContext *cx, JSObject *ob
>                 if (!js_AddAsGCBytes(cx, allocated)) {
>                     /* js_AddAsGCBytes releases the GC lock on failures. */
>                     cx->free(ne);
>                     return JS_FALSE;
>                 }
>                 ne->next = cx->runtime->nativeEnumerators;
>                 cx->runtime->nativeEnumerators = ne;
>                 JS_ASSERT(((jsuword) ne & (jsuword) 1) == (jsuword) 0);
>-                *cachep = (jsuword) ne;
>+
>+                /*
>+                 * Do not cache enumerators for objects with an overflown

"overflowed"

fly/flew/[have|had] flown != overflow/overflowed/[have|had] overflowed

Ain't English grand? :-P

So "with a shape that had overflowed" might be better, but your sentence structure (with the word fixed) is fine comment-speak.

r=me with this fixed. Thanks,

/be

Comment 9

[Igor Bukanov]

Attachment: fix v3

fixing English in comments

Comment 10

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/7cd82c08ad9e

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7cd82c08ad9e

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/48e5373f10bc

Comment 13

[Samuel Sidler (old account; do not CC)]

Does this patch apply to 1.9.1? If so, can you please request approval? If not, please attach a patch that does apply and request approval. Code freeze is currently scheduled for November 10 at 11:59pm.

(Or does this bug require some part of the bug 503080 and its dependencies?)

Comment 14

[Daniel Veditz [:dveditz]]

Comment 7 is still worrying -- does this patch work for the 1.9.1 branch or does it require any part of the big set of changes/regressions in bug 503080?