Assertion failure: !denseElementsAreFrozen(), at js/src/vm/NativeObject.h:1033

RESOLVED FIXED in Firefox 51

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: emilio)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla52
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 attachments)

The following testcase crashes on mozilla-central revision 215f96861176 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

var a = Object.freeze([4, 5, 1]);
a.watch("x", function(id, oldval, newval) {});



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000502d28 in js::NativeObject::setDenseElement (this=<optimized out>, index=<optimized out>, val=...) at js/src/vm/NativeObject.h:1033
#0  0x0000000000502d28 in js::NativeObject::setDenseElement (this=<optimized out>, index=<optimized out>, val=...) at js/src/vm/NativeObject.h:1033
#1  0x0000000000b44e1d in js::NativeObject::removeDenseElementForSparseIndex (index=0, obj=..., cx=0x7ffff695f000) at js/src/vm/NativeObject-inl.h:102
#2  js::NativeObject::sparsifyDenseElement (cx=cx@entry=0x7ffff695f000, obj=obj@entry=..., index=index@entry=0) at js/src/vm/NativeObject.cpp:519
#3  0x0000000000b45067 in js::NativeObject::sparsifyDenseElements (cx=cx@entry=0x7ffff695f000, obj=..., obj@entry=...) at js/src/vm/NativeObject.cpp:546
#4  0x0000000000982e9c in js::WatchGuts (cx=0x7ffff695f000, origObj=..., id=..., callable=...) at js/src/jsobj.cpp:2769
#5  0x0000000000c72fa4 in obj_watch (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Object.cpp:532
#6  0x0000000000b158c1 in js::CallJSNative (cx=cx@entry=0x7ffff695f000, native=0xc72e00 <obj_watch(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:239
[...]
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7901
rax	0x0	0
rbx	0x7fffffffcb70	140737488341872
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffca70	140737488341616
rsp	0x7fffffffca50	140737488341584
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x0	0
r13	0x7ffff695f000	140737330409472
r14	0x7fffffffca90	140737488341648
r15	0x7fffffffcb70	140737488341872
rip	0x502d28 <js::NativeObject::setDenseElement(unsigned int, JS::Value const&)+248>
=> 0x502d28 <js::NativeObject::setDenseElement(unsigned int, JS::Value const&)+248>:	movl   $0x0,0x0
   0x502d33 <js::NativeObject::setDenseElement(unsigned int, JS::Value const&)+259>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4839dd5d7d96
user:        Emilio Cobos Álvarez
date:        Wed Oct 19 19:23:33 2016 +0200
summary:     Bug 1310744: Add missing assertions in NativeObject.h r=nbp

This iteration took 256.740 seconds to run.
Nicolas/Emilio, is bug 1310744 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(ecoal95)
Yes, sort of. This uncovers a bug that was present but ignored before. I'll send a patch ASAP.

Thanks Gary!
This patch also fixes bug 1312487, here's the test-case for it.
Assignee: nobody → ecoal95
Flags: needinfo?(ecoal95)
Attachment #8804040 - Flags: review?(nicolas.b.pierron)
Cancelling the review request since I pushed the wrong patch (this one didn't include the fix for the other bug), plus also I discovered another bug with Object.watch I'm fixing.
Hi Nicolas, what do you think is the correct solution here?

So: js::WatchGuts sparsifies elements, which sets dense elements as holes. My solution to this was just not doing it for dense elements. But then triggerWatchHandler tries to access the sparse elements, givin undefined for the old value of the property. My initial thinking is that triggering the handler was an error anyway, because the object's properties won't ever change, but apparently the handler is expected to be executed *before* actually setting the property.

One option is making watch being a noop for frozen objects, which isn't great, but this is an API only SpiderMonkey implements anyway, and it doesn't make a lot of sense to watch frozen objects.

The other one is, I guess, specializing the "old property value" lookup for frozen objects. We only have a HandleId there, would it be enough to look at the dense elements if `id.isInt32()`, and assume everything else is undefined or reachable through the shape?
When I saw this bug yesterday, I was thinking we could just make removeDenseElementForSparseIndex work for frozen elements (by passing a flag to setDenseElement or calling a new method)? We just have to be careful we create sparse elements with the right attributes for a frozen property.

IMO there's no good reason why a frozen object cannot be sparsified, here it's |watch| doing it but there might be other places.
Comment on attachment 8804040 [details]
Bug 1312485: array: Don't optimize for dense storage if the elements are frozen.

https://reviewboard.mozilla.org/r/88184/#review87320
Attachment #8804040 - Flags: review?(nicolas.b.pierron) → review+
I agree with Jan, dense/sparse is an internal representation of the object.  So technically changing this representation is fine as long as the changes are not observable.

So in most cases, I also agree that there is no reason which should cause a frozen object to changed between sparse/dense representation to another, as it is not supposed to be mutated.  I think I am fine with the current proposal, except that  Unchecked  is probably too alarmist.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Too bad Object.watch throws after sparsifying, though I guess this is not a
> case
> worth special-casing for.

It throws *while sparsifying*, because we remove the dense element and after that we immediately throw when we add the sparse data property, so we don't sparsify the object.

sparsifyDenseElement shouldn't go through addDataProperty, because we don't want the extensibility check in this case. It might be best to call addPropertyInternal, like we do here: http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/js/src/vm/Shape.cpp#536-541

You still have to pass the correct attributes if the property is frozen. For this it would be nice to factor out these 3 lines into ObjectElements::elementAttributes() and call it from both places:

http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/js/src/vm/Shape-inl.h#178-180
Comment on attachment 8804193 [details]
Bug 1312485: Properly sparsify frozen objects.

https://reviewboard.mozilla.org/r/88296/#review87568

See previous comment. Thanks a lot for fixing this!
Attachment #8804193 - Flags: review?(jdemooij)
Sounds like we'll want this on 51 as well.
Comment on attachment 8804193 [details]
Bug 1312485: Properly sparsify frozen objects.

https://reviewboard.mozilla.org/r/88296/#review88022

Very nice, thanks for fixing!

::: js/src/vm/NativeObject.h:309
(Diff revision 2)
>          MOZ_ASSERT(isFrozen());
>          MOZ_ASSERT(!isCopyOnWrite());
>          flags &= ~FROZEN;
>      }
>  
> +    uint8_t elementAttributes() {

Nit: can mark this const:

  uint8_t elementAttributes() const {

::: js/src/vm/NativeObject.h:877
(Diff revision 2)
>      inline bool updateSlotsForSpan(ExclusiveContext* cx, size_t oldSpan, size_t newSpan);
>  
> -  public:
> +  private:
> +    void prepareElementRangeForOverwrite(size_t start, size_t end) {
> +        MOZ_ASSERT(end <= getDenseInitializedLength());
> +        MOZ_ASSERT(!denseElementsAreCopyOnWrite());

Is the indentation here correct? mozreview shows the second MOZ_ASSERT is indented differently, but I'm not sure if that's a UI quirk.

::: js/src/vm/NativeObject.h:1036
(Diff revision 2)
>      }
>  
> +    // Use this function with care.
> +    // This is done to allow sparsifying frozen objects,
> +    // but should only be called in a few places, and should
> +    // be audited carefully!

Nit: wrap to 80 columns, something like:

    // Use this function with care. This is done to allow sparsifying frozen
    // objects, but should only be called in a few places, and should be audited
    // carefully!

::: js/src/vm/NativeObject.cpp:531
(Diff revision 2)
> +    if (obj->inDictionaryMode())
> +        entry = &obj->lastProperty()->table().search<MaybeAdding::Adding>(id);
> +
> +    // NOTE: We don't use addDataProperty because we don't want the extensibility
> +    // check if we're, for example, sparsifying frozen objects..
> +    RootedNativeObject nobj(cx, obj);

Remove this line, |nobj| is unused.

::: js/src/vm/NativeObject.cpp:1023
(Diff revision 2)
>      return addProperty(cx, self, id, nullptr, nullptr, slot, attrs, 0);
>  }
>  
>  Shape*
>  NativeObject::addDataProperty(ExclusiveContext* cx, HandlePropertyName name,
> -                          uint32_t slot, unsigned attrs)
> +                              uint32_t slot, unsigned attrs)

Remove the >>>> marker here.
Attachment #8804193 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #17)
> > -                          uint32_t slot, unsigned attrs)
> > +                              uint32_t slot, unsigned attrs)
> 
> Remove the >>>> marker here.

Oops I misunderstood mozreview, you just fixed the indentation. Nevermind :)
Comment on attachment 8804193 [details]
Bug 1312485: Properly sparsify frozen objects.

https://reviewboard.mozilla.org/r/88296/#review88022

> Nit: can mark this const:
> 
>   uint8_t elementAttributes() const {

Good catch :)

> Remove this line, |nobj| is unused.

Hmm, I meant to pass nobj as a parameter to `addPropertyInternal`. `addDataProperty` creates a root, so I didn't want to change that.

I'll keep that variable (using it) for now because I don't know enough about rooting to know whether is totally safe to remove it, but let me know if it's safe to do so and I'll edit/push a followup.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #19)
> I'll keep that variable (using it) for now because I don't know enough about
> rooting to know whether is totally safe to remove it, but let me know if
> it's safe to do so and I'll edit/push a followup.

Replied on IRC:

<jandem>	 emilio: it's safe to remove
<jandem>	 emilio: |obj| is a HandleObject, that means it's properly rooted
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b46310f1fb77
array: Don't optimize for dense storage if the elements are frozen. r=nbp
https://hg.mozilla.org/integration/autoland/rev/e97f6b0391f9
Properly sparsify frozen objects. r=jandem
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 10a2b6ebcd44).
https://hg.mozilla.org/mozilla-central/rev/b46310f1fb77
https://hg.mozilla.org/mozilla-central/rev/e97f6b0391f9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8804040 [details]
Bug 1312485: array: Don't optimize for dense storage if the elements are frozen.

This request is for both patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: Bug 1283334
[User impact if declined]: Incorrect JS behavior under some circumstances (potentially crashing if they hit the code path from bug 1312485).
[Describe test coverage new/current, TreeHerder]: Automated tests added.
[Risks and why]: Low, more regressions from bug 1283334 discovered by the assertions added in bug 1310744.
[String/UUID change made/needed]: None
Attachment #8804040 - Flags: approval-mozilla-aurora?
Comment on attachment 8804040 [details]
Bug 1312485: array: Don't optimize for dense storage if the elements are frozen.

Fix an assertion failure related to JS. Take it in 51 aurora.
Attachment #8804040 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.