1300129 - Crash in InvalidArrayIndex_CRASH | mozilla::EventListenerManager::GetListenerInfo

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-79ac1c16-9a1b-4f2b-a4bf-6e2d12160902.
=============================================================

This crash is odd.

uint32_t count = mListeners.Length();
for (uint32_t i = 0; i < count; ++i) {
  const Listener& listener = mListeners.ElementAt(i);

The index is one more than the length. The mostly likely cause that I can think of is iterator invalidation, ie while we're iterating over the member variable of the ELM, we end up removing something from the array.

This is the only crash report I saw with a deep stack:
  bp-96879694-cd7f-4036-8d85-5df8e2160901

Comment 1

[Andrew McCreight [:mccr8]]

This signature is 44% of the invalid array index crashes in the last 7 days, with 37 crashes. It is the most common such crash.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I wonder... since this is a bit similar to that other mListeners related bug.
Could we have some bug in nsAutoTObserverArray?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

or are we using it in some wrong way.

Comment 4

[Andrew McCreight [:mccr8]]

Oh, I didn't even notice that it was an auto observer array. So we would expect this to be okay, even in the presence of some iterator invalidation.

Comment 5

[Andrew McCreight [:mccr8]]

The comment on nsAutoTObserverArray::ElementAt() is:
  // This method provides direct access to an element of the array. The given
  // index must be within the array bounds. If the underlying array may change
  // during iteration, use an iterator instead of this function.

So I think the ElementAt is bad.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ELM fixes

Make RemoveEventListenerInternal safer by moving NotifyEventListenerRemoved outside the loop
(and fix also i handling). Iterators are hard to use here because we want to remove item and forward iterator can't quite deal that right now. And I want to keep doing forward iteration, not backwards.

Switch EventListenerManager::GetListenerInfo to use iterator in case CompileEventHandlerInternal causes changes to mListeners.

Pure guess fixes.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8787704 [details] [diff] [review]
ELM fixes

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

Thanks for fixing this.

::: dom/events/EventListenerManager.cpp
@@ +670,2 @@
>          --count;
> +        --i;

Hmm. This will underflow if i = 0, but I guess that's okay because this is an unsigned integer. It'll go back to 0 at the end of the loop.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

yes, I was thinking about that with i, and it should just work.

Hopefully the patch fixes something. 

since this doesn't have security rating, do you think I need approval.

I'd use commit message "Ensure devtools get the right event listener data" or something like that. vague, but true.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #8)
> since this doesn't have security rating, do you think I need approval.

It needs a rating before it can be landed. The rating would determine whether or not it needs approval. How bad of a security issue do you think this is? On Nightly and Aurora, it is less of a problem because we have these checks, but for older branches it might be a problem. I don't know how easily content could control these, and reading a few off the end of an array doesn't seem like it would do too much.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Well, I don't still understand how the crashes might happen, so really hard to say.

Comment 11

[Andrew McCreight [:mccr8]]

Olli said that EventListenerService is the only thing that can trigger this method, and it can only be run from Chrome JS, so I'm just going to mark this sec-moderate, so it will not need sec-approval before landing.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: safer_elm_for_devtools.diff

Let do only this change in this bug.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Olli, should this be uplifted to Aurora50?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8787751 [details] [diff] [review]
safer_elm_for_devtools.diff

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: possible crashes when using devtools
[Describe test coverage new/current, TreeHerder]: NA
[Risks and why]: Should be very safe, but since the crash isn't reproduceable, it isn't 100% sure whether this helps
[String/UUID change made/needed]: NA

Comment 17

[Andrew McCreight [:mccr8]]

I don't see this signature on Nightly any more, while it used to be the most common crash with InvalidArrayIndex_CRASH. I still see it on Nightly. So I think your patch worked.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8787751 [details] [diff] [review]
safer_elm_for_devtools.diff

Crash fix, seems to have worked in Nightly51, Aurora50+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd43153e25c9