(In reply to Kamil Jozwiak [:kjozwiak] from comment #2)
> (In reply to Olli Pettay [:smaug] from comment #1)
> > Does the issue happen on FF40 too?
>
> Yup, reproduced it with fx40 using the same steps that are listed above.
So very much looks like a regression from bug 1154701.
I assume one of those
for (i = 0; i < array.length; ++i) -> for (item: array)
changes caused this. those are rather different ways to iterate array, especially from security point of view.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> This is very likely a use after free.
And it is. We're accessing what an iterator points to after we mutate the array to remove the item.

Comment on attachment 8608951[details][diff][review]
Part 1: Copy the editor observers array before iterating over it
(Note: these approvals are being requested for both of the patches here.)
[Security approval request comment]
How easily could an exploit be constructed based on the patch? The first patch makes the issue pretty obvious, but it's impossible to fix this issue in another way. It is not easy to go from the patch to an exploit, however.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? See above.
Which older supported branches are affected by this flaw? Only Nightly and Aurora.
If not all supported branches, which bug introduced the flaw? Bug 1154701
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I'd prefer to fix this, the backout may be regression prone.
How likely is this patch to cause regressions; how much testing does it need? It technically changes the behavior for running the edit actions, but I don't expect it to be very regression prone. At any rate, we do have some baking time.
Approval Request Comment
[Feature/regressing bug #]: Bug 1154701.
[User impact if declined]: Comment 0.
[Describe test coverage new/current, TreeHerder]: Locally tested.
[Risks and why]: See above.
[String/UUID change made/needed]: None.

(Replying bug 1162360 comment 41)
I think I was indeed trying to solve the same issue there, but just by reading the descriptions I don't know if these patches work for my use case. I explicitly choose not to copy the array before iterating it because we do need to notify the observers added while the loop is being run on. Should we instead landing my patch in this bug?
Anyhow, I am going to read the patches here and try them too on Monday.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #17)
> > I explicitly choose not to copy the array before iterating it because we do need to notify
> > the observers added while the loop is being run on.
>
> I agree with this.
Which observers do we need to run? If you know of one where this will end up doing the wrong behavior, please file a new bug for it and needinfo me on it so that we can discuss that. Thanks!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18)
> Which observers do we need to run? If you know of one where this will end
> up doing the wrong behavior, please file a new bug for it and needinfo me on
> it so that we can discuss that. Thanks!
Let me re-phrase bug 1162360 comment 11:
On B2G, we would have 3 observers in the observers array
0: nsTextInputListener
1: FormAssistant (in forms.js)
2: IMEContentObserver
The part 1 patch of bug 1162360 is applied, FormAssistant.EditAction will eventually cause IMEContentObserver at 2 to be destroyed and a new one added being assigned in its place. A range-based for loop will attempt to call on the old one, hence the crash. we will ask RemoveEditorObserver do things differently: when the old one got replaced, we will replace it with a nullptr so it still keep its place. The newly added IMEContentObserver will be assigned in the 3rd place.
0: nsTextInputListener
1: FormAssistant (in forms.js)
2: nullptr (IMEContentObserver removed cause by action in FormAssistant)
3: IMEContentObserver (added cause by action in FormAssistant)
And a traditional for loop will ensure we'll always run to to the very end so the added one gets notified. nullptr will get cleaned up once the loop is finished.
The first patch here (copying the array) will not prevent the crash in the case mentioned, and it will miss the newly added IMEContentObserver.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #19)
> The first patch here (copying the array) will not prevent the crash in the
> case mentioned, and it will miss the newly added IMEContentObserver.
The problem with your patch #2 on that bug is that it will cause us to call EditAction() on observers that were not registered at the time that the editing operation happened, which is incorrect in the general case for other observers.
Couldn't you call the EditAction manually when you register the observer as part of another observer's EditAction()?
(FWIW, we can move this discussion to bug 1162360, I think.)