1374862 - [e10s only] Assertion failure: !gHandled || !aCommands.IsEmpty() pressing ctl+shift+A

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

STR:
1. Load a page in a remote tab.
2. Press ctl+shift+a (to open the addons manager)

Expected results: The addons manager opens
Actual results:

Assertion failure: !gHandled || !aCommands.IsEmpty(), at /home/mrbkap/work/main/mozilla/widget/gtk/NativeKeyBindings.cpp:363

There appear to be two parts to this. The first is that in the parent process, we unconditionally call InitAllEditCommands on keypress events (note the XXX in the code) [1]. That ends up calling into [2] with `count == 0`, setting `gHandled` to true and not adding any commands to gCurrentCommands. That causes the assertion to fire.

I don't know enough about input contexts to fix the TabParent code, but fixing `move_cursor_cb` to only set gHandled if `count != 0` seems to fix this assertion without any ill effects.

This is mostly annoying for me when debugging things related to addons in debug builds.

[1] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/dom/ipc/TabParent.cpp#1428-1431
[2] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/widget/gtk/NativeKeyBindings.cpp#172

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1374862 - Avoid setting gHandled when we don't handle signals.

This also changes delete_from_cursor_cb though I doubt we can get there with
count being 0.

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

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8879753 [details]
Bug 1374862 - Avoid setting gHandled when we don't handle signals.

https://reviewboard.mozilla.org/r/151102/#review160110

It seems sensible to me to leave gHandled unset when there are no commands.

Please request review from the author of the fatal assert, after updating the commit message.
I assume the assert was added because they wanted to know about this case.
https://hg.mozilla.org/mozilla-central/rev/b75c111837a802ceb953dba50a3c5a193d53ca22#l14.229

::: commit-message-58c51:1
(Diff revision 1)
> +Bug 1374862 - Avoid an assertion by skipping work in edge cases. r=karlt

Please be more specific.  This comment is not helpful without knowledge of the edge case, work, or assert.

But the key difference is that gHandled is not set, which I assume is a change in behavior rather than just skipping unnecessary work.

Comment 3

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

Comment on attachment 8879753 [details]
Bug 1374862 - Avoid setting gHandled when we don't handle signals.

https://reviewboard.mozilla.org/r/151102/#review160142

::: widget/gtk/NativeKeyBindings.cpp:67
(Diff revision 1)
>  static void
>  delete_from_cursor_cb(GtkWidget *w, GtkDeleteType del_type,
>                        gint count, gpointer user_data)
>  {
>    g_signal_stop_emission_by_name(w, "delete_from_cursor");
> +  if (count == 0) {

FYI: |!count| is usually used in our code instead of |count == 0|. (IIRC, there was such rule in our coding style guide. But there is not in it now.)

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8879753 [details]
Bug 1374862 - Avoid setting gHandled when we don't handle signals.

https://reviewboard.mozilla.org/r/151102/#review160142

> FYI: |!count| is usually used in our code instead of |count == 0|. (IIRC, there was such rule in our coding style guide. But there is not in it now.)

I actually prefer comparing integers with constants of their own type over
conversion to bool.  The names of variables often don't clearly indicate the
type, and so the constants provide useful clues to help understand the code.

The style guide requests conversion to bool for pointers in preference to
comparison with nullptr.  I don't recall the style guide ever requesting
conversion of int to bool.  0 is not such a special integer and so integers
can be compared with 0 in the same way as they are compared with other
integers.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Masayuki, are you otherwise OK with this patch? I can resubmit with a better commit message (though I agree with comment 4 that integer comparison against 0 is better than the !-syntax) if so.

Comment 6

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

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> Masayuki, are you otherwise OK with this patch?

Yeah.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8879753 [details]
Bug 1374862 - Avoid setting gHandled when we don't handle signals.

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

Comment 8

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

Comment on attachment 8879753 [details]
Bug 1374862 - Avoid setting gHandled when we don't handle signals.

https://reviewboard.mozilla.org/r/151102/#review162398

Same crash can occur with some other cases though, it's okay since it's not so to be detactable it only on debug build.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf0251c25bbb
Avoid setting gHandled when we don't handle signals. r=masayuki

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem worth backporting to Beta. Feel free to set the status back to affected and nominate for approval if you feel otherwise, though.