1363150 - Assertion failure: !denseElementsAreFrozen(), at /home/andre/git/mozilla-central/js/src/vm/NativeObject-inl.h:223

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
---
var array;
var len = 10;
oomTest(function() {
    var a = [];
    for (var i = 0; i < len; ++i) a[i] = 0;
    for (var i = 0; i < len; ++i) a[String.fromCharCode(i + 40)] = 0;
    try { Object.freeze(a); } catch(e) {}
    if (Object.getOwnPropertyDescriptor(a, "length").writable) {
        a.splice(0, 0, 1);
        // Also reproducible with: a.unshift(1);
    }
    len = len + (len >> 1);
});
---

Asserts with:
---
Assertion failure: !denseElementsAreFrozen(), at /home/andre/git/mozilla-central/js/src/vm/NativeObject-inl.h:223
---

This one is kind of similar to bug 1300904: We set FROZEN flag on the object, but then OOM, which gets us into an inconsistent state. 




Stack trace:
---
#0  0x0000000000555ac1 in js::NativeObject::ensureDenseInitializedLengthNoPackedCheck(JSContext*, unsigned int, unsigned int) (this=(js::NativeObject * const) 0x7ffff7e01278 [object Array], cx=0x7ffff6975000, index=49, extra=1) at /home/andre/git/mozilla-central/js/src/vm/NativeObject-inl.h:223
#1  0x0000000000555eb7 in js::NativeObject::ensureDenseElements(JSContext*, unsigned int, unsigned int) (this=(js::NativeObject * const) 0x7ffff7e01278 [object Array], cx=0x7ffff6975000, index=49, extra=1) at /home/andre/git/mozilla-central/js/src/vm/NativeObject-inl.h:310
#2  0x000000000053b9ea in array_splice_impl(JSContext*, unsigned int, JS::Value*, bool) (cx=0x7ffff6975000, argc=3, vp=0x7fffffffbbb0, returnValueIsUsed=false)
    at /home/andre/git/mozilla-central/js/src/jsarray.cpp:2693
#3  0x000000000053c021 in array_splice_noRetVal(JSContext*, unsigned int, JS::Value*) (cx=0x7ffff6975000, argc=3, vp=0x7fffffffbbb0) at /home/andre/git/mozilla-central/js/src/jsarray.cpp:2770
#4  0x00000000005a2978 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (cx=0x7ffff6975000, native=0x53bff6 <array_splice_noRetVal(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:291
#5  0x000000000057f53c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (cx=0x7ffff6975000, args=..., construct=js::NO_CONSTRUCT)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:470
#6  0x000000000057f8b5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) (cx=0x7ffff6975000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:515
#7  0x000000000057f8f3 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7ffff6975000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:521
#8  0x00000000006b2e92 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, uint32_t, JS::Value*, JS::MutableHandleValue) (cx=0x7ffff6975000, frame=0x7fffffffbc38, stub_=0x7ffff4246888, argc=3, vp=0x7fffffffbbb0, res=JSVAL_VOID) at /home/andre/git/mozilla-central/js/src/jit/BaselineIC.cpp:2457
....
---

Comment 1

[Daniel Veditz [:dveditz]]

Is this a potential security problem? the similar bug 1300904 was not hidden.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

With this patch we freeze the elements at the end of SetIntegrityLevel instead of in PreventExtensions. This avoids the OOM bug and passes tests.

OOM here is still a bit weird though, because we can end up with a non-extensible object with non-frozen elements, but the same applies to the other (non-element) properties if we OOM while redefining them. So I'm not sure if that's a problem...

Comment 3

[André Bargull [:anba]]

Comment on attachment 8872593 [details] [diff] [review]
Patch

Review of attachment 8872593 [details] [diff] [review]:
-----------------------------------------------------------------

I like it. :-)

And I think it makes it also easier to follow the logic around frozen elements, because markNotFrozen() is no longer needed and the type information is directly updated in ObjectElements::FreezeElements().

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> OOM here is still a bit weird though, because we can end up with a
> non-extensible object with non-frozen elements, but the same applies to the
> other (non-element) properties if we OOM while redefining them. So I'm not
> sure if that's a problem...

Agreed, I don't think this is problematic.

Comment 5

[Daniel Veditz [:dveditz]]

guessing sec-moderate?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8872593 [details] [diff] [review]
Patch

OK, changing to r?

sec-moderate probably makes sense and I don't think we need to backport this if it makes 55. But to be extra cautious it seems best to keep this hidden for now and to land the testcase later.

Comment 7

[André Bargull [:anba]]

Comment on attachment 8872593 [details] [diff] [review]
Patch

Review of attachment 8872593 [details] [diff] [review]:
-----------------------------------------------------------------

Still good! :-)

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b14e3c9ba39cc0b09eb75025a1ec44eb0ebcd576

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b14e3c9ba39c