1344544 - Crash in InvalidArrayIndex_CRASH | mozilla::ArrayIterator<T>::operator*

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-3f9c9168-afba-44e1-a264-44dfc2170304.
=============================================================

There are 11 crashes on nightly 54 with buildid 20170304110455, they probably come from the same installation (the install time is the same). In analyzing the backtrace the regression may have been introduced by patch [1] to fix bug 1342303.

[1] https://hg.mozilla.org/mozilla-central/rev?node=2d861e4e01ef372c9ae1bdabf379c8658a79a4f2

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

So nsWindowMediator's listeners can be JavaScript so we cannot trust that the array is unchanged during the iteration. It indicates that it should probably be using observer array rather than nsCOMArray.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

Review commit: https://reviewboard.mozilla.org/r/117370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117370/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8843794 [details]
Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117370/diff/1-2/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Try push looks mostly good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02f92dba2ec6b734a7e5e0b04d54cbeb2dc30283

But tc-VP(b-t) looks pretty bad... Not sure whether this is cause by this change...

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8843794 [details]
Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

https://reviewboard.mozilla.org/r/117370/#review119296

r=me Glad we caught some potentially buggy code!

::: xpfe/appshell/nsWindowMediator.h:15
(Diff revision 2)
>  #include "nsIWindowMediator.h"
>  #include "nsIObserver.h"
>  #include "nsTArray.h"
>  #include "nsXPIDLString.h"
>  #include "nsWeakReference.h"
> -#include "nsCOMArray.h"
> +#include "nsTObserverArray.h"

Huh, we should really change the name of `nsTObserverArray`.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07345c5aa073
Use nsTObserverArray for listeners of nsWindowMediator. r=erahm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/07345c5aa073

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8843794 [details]
Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1342303
[User impact if declined]: May crash in some case.
[Is this code covered by automated tests?]: no. Probably it's worth adding one...
[Has the fix been verified in Nightly?]: just landed.
[Needs manual test from QE? If yes, steps to reproduce]: probably no. I have no idea how to reproduce without code work.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: not risky.
[Why is the change risky/not risky?]: Changes the internal representation of an array to ensure it behaves correctly when it is mutated during iteration.
[String changes made/needed]: none.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8843794 [details]
Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

Fix a crash. Aurora54+.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/176b346ae762