Closed Bug 1374862 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Widget: Gtk, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: regression)

Attachments

(1 file)

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 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.
Attachment #8879753 - Flags: review?(karlt)
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 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.
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.
Flags: needinfo?(masayuki)
(In reply to Blake Kaplan (:mrbkap) from comment #5)
> Masayuki, are you otherwise OK with this patch?

Yeah.
Flags: needinfo?(masayuki)
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.
Attachment #8879753 - Flags: review?(masayuki) → review+
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf0251c25bbb
Avoid setting gHandled when we don't handle signals. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/bf0251c25bbb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
You need to log in before you can comment on or make changes to this bug.