Closed Bug 1344544 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect, critical)

54 Branch
Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: calixte, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

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
Flags: needinfo?(xidorn+moz)
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.
Attachment #8843794 - Flags: review?(erahm)
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...
Flags: needinfo?(xidorn+moz)
Assignee: nobody → xidorn+moz
Whiteboard: [clouseau]
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`.
Attachment #8843794 - Flags: review?(erahm) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07345c5aa073
Use nsTObserverArray for listeners of nsWindowMediator. r=erahm
https://hg.mozilla.org/mozilla-central/rev/07345c5aa073
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(xidorn+moz)
Version: 52 Branch → 54 Branch
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.
Flags: needinfo?(xidorn+moz)
Attachment #8843794 - Flags: approval-mozilla-aurora?
Comment on attachment 8843794 [details]
Bug 1344544 - Use nsTObserverArray for listeners of nsWindowMediator.

Fix a crash. Aurora54+.
Attachment #8843794 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.