792957 - "ASSERTION: Attempting to allocate excessively large array" with MozLockOrientation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file nsTArray-inl.h, line 112

Maybe this assertion is just bogus.

Comment 1

[Jesse Ruderman]

Attachment: stack+

Comment 2

[Justin Lebar (not reading bugmail)]

> Maybe this assertion is just bogus.
>
> #4  0x00000001011f72b6 in nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity 
> (this=0x7fff5fbf8f40, capacity=183420626, elemSize=16) at nsTArray-inl.h:112

That looks excessively large to me!

Comment 3

[Boris Zbarsky [:bzbarsky]]

This code needs to use a fallible array, at the very least, no?

Or better yet, we should convert nsScreen to WebIDL, which does this sort of thing right... ;)

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #2)
> > Maybe this assertion is just bogus.
> >
> > #4  0x00000001011f72b6 in nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity 
> > (this=0x7fff5fbf8f40, capacity=183420626, elemSize=16) at nsTArray-inl.h:112
> 
> That looks excessively large to me!

This is because the js code is passing an array of that size.

I think we should probably not size the array too early. We might actually want a set and in that case, the set will never be higher than 4 entries, whatever the input is.

Comment 5

[Boris Zbarsky [:bzbarsky]]

On the other hand, I'm not sure it make sense to assert here.  We should just fail the capacity set and move on.  Moving on in the infallible array case should mean aborting, by the way!  That seems like a bug in the array code.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Patch

Use a std::set<nsString> instead of a nsTArray. That way, we will never allocate more elements than needed.
I haven't use nsTHashtable because that API is annoying (you have to iterate trough another function) and I see no reason why we shouldn't be allowed to use std::set in our codebase.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Does std::set ever throw exceptions?

Comment 9

[Justin Lebar (not reading bugmail)]

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

Ehsan, do you know if it's safe to use std::set in our code?  If not, can you put this f? to someone who you think might know?

Comment 10

[(no longer active)]

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

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

std::set is fine.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

How does this fix anything? We're just hitting OOM in std::set and hope nothing blows up?

Comment 12

[Justin Lebar (not reading bugmail)]

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

I'm with Ms2ger on this one; why not build the bitmap without ever building the set/list?

Comment 13

[Mounir Lamouri (:mounir)]

Bug 793244 is going to fix this.