1230473 - crash in nsEditor::EnsureComposition | In a compositionstart listener, blur() then focus() causes a segfault

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[david]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/45.0.2454.101 Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

1. On Ubuntu 15.10, go to http://jsfiddle.net/6tsyptoh/2/ .
2. Trigger a compositionstart event. For example, press backtick twice in the UK (Extended WinKeys) keyboard layout. (Various IMEs I've tried seem to cause the same results)


Actual results:

The browser segfaults after the listener returns.

You can get the same effect by replacing the "blur(); focus()" with an alert(), in which case the segfault does not happen until the alert dialog is closed.


Expected results:

No segfault (though the IME may be interrupted)

Comment 1

[Gingerbread Man]

Can you reproduce the crash with a build from mozilla.org? If so, please submit a crash report, then post the respective report ID from about:crashes in a comment here.
https://www.mozilla.org/firefox/all/
https://developer.mozilla.org/docs/How_to_get_a_stacktrace_for_a_bug_report

Comment 2

[david]

Yes it segfaults a mozilla.org build too; report ID bp-27dfb1e4-70b0-4fd7-bd4c-073542151204 (sorry for not including this before).

Comment 3

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

null pointer crash. Regression from bug 960866?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, interesting case...

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsEditor::EnsureComposition':
 - nightly(version 50):0 crashes from 2016-06-06.
 - aurora (version 49):18 crashes from 2016-06-07.
 - beta   (version 48):1 crash from 2016-06-06.
 - release(version 47):12 crashes from 2016-05-31.
 - esr    (version 45):3 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        6       0       1       0       1       1       7
 - beta          1       0       0       0       0       0       0
 - release       0       4       2       2       1       0       3
 - esr           0       0       1       1       0       0       1

Affected platforms: Windows, Mac OS X, Linux

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=610701601a39

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing

Even when editor receives eCompositionStart event, the active composition may have gone since web contents can listen to composition events before editor (so, web contents can commit the composition before "compositionstart" reaching focused editor).

Therefore, editor shouldn't crash as unexpected scenario.  Instead, it should do nothing in such case.

Note that when editor receives 2nd or later "compositionupdate" or "text" event, it should not modify composition.  However, currently, it does that.  This patch does NOT fix it since it's really rare case.  It should be fixed in another bug because this should be uplifted (crashing with some IMEs on Android is serious).

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

Comment 9

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

Comment on attachment 8808559 [details]
Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing

https://reviewboard.mozilla.org/r/91372/#review91222

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6184510d1b73
If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6184510d1b73

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8808559 [details]
Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing

Approval Request Comment
[Feature/regressing bug #]: bug 960866
[User impact if declined]: According to comments in crash reports, this bug can be reproduced with some IMEs (we're not sure what they are) when user logs in to Firefox Account at setting Firefox Sync on Android. I think that Firefox Sync is very important feature of Firefox for Android. So, this should be uplifted as far as possible.
[Describe test coverage new/current, TreeHerder]: Landed on mozilla-central a couple of days ago and has automated test.
[Risks and why]: Low, the patch removes MOZ_CRASH() and in such case, stops handling "outdated" composition events. The new behavior is tested by the automated test.
[String/UUID change made/needed]: Nothing.

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8808559 [details]
Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing

This patch fixes a crash and includes tests. Take it in 51 aurora.

Comment 14

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for aurora

Sorry! This is rebased patch for Aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21f4fa7acb97

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8808559 [details]
Bug 1230473 If there is no TextComposition instance even when EditorBase receives eCompositionStart event, the editor should do nothing

This was uplifted to 51 before it went to beta, so nothing to uplift there anymore.