1163304 - Nighglty crashes at [@ nsEditor::EnsureComposition(mozilla::WidgetGUIEvent*) ]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Hideo Oshima]

Steps to reproduce:
1. Click the search box and turn on Input Method.
2. Input あ and commit.
3. Press Alt key and hit F.

Actual result:
Nightly crashes.

Crash report:
https://crash-stats.mozilla.com/report/index/860fe4d7-c545-4d1c-8cad-10e9b2150509

Bug 1117087 has beed fixed but I can still reproduce.

My environment:
OS: Debian unstable(uptodate)
Toolkit: libgtk2.0-0 2.24.25-3

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

This must be a bug in widget/gtk/nsGtkIMModule.cpp.

Oshima-san, what IME are you using? iBus?

Comment 2

[Hideo Oshima]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #1)
> This must be a bug in widget/gtk/nsGtkIMModule.cpp.
> 
> Oshima-san, what IME are you using? iBus?

No.
I use fcitx(1:4.2.8.5-2) + Mozc(1.15.1857.102-1+b1).

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Hmm, odd.

Do you use e10s?
Do you see suggest list of search engine after step #2?
Do you expect that Alt+F should open File menu?

Comment 4

[Alice0775 White]

I can reproduce the crash on ubuntu14.04 32bit Firefox30 and later.

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=802d87c77e76&tochange=a62bde1d6efe

Suspect: Bug 960866

Comment 5

[Hideo Oshima]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #3)
> Hmm, odd.
> 
> Do you use e10s?
Yes.

> Do you see suggest list of search engine after step #2?
Yes.

> Do you expect that Alt+F should open File menu?
Yes.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Um, I still cannot reproduce this bug. Does somebody post log file created with NSPR_LOG_MODULES=nsGtkIMModuleWidgets:5,IMEStateManager:5? (Don't forget to specify NSPR_LOG_FILE too and don't type anything your private data like password!)

Comment 7

[Alice0775 White]

Attachment: log.txt

attached log.txt with non-e10s

40.0a1 non-e10s: bp-1f6371b4-ecdf-4ea3-9eb1-8241b2150509

Mozilla/5.0 (X11; Linux i686; rv:40.0) Gecko/20100101 Firefox/40.0 ID:20150509030210


Other crashes:
30.0: bp-8a2f10a8-8ed7-4f55-87e0-628b22150509
31.5.0: bp-1ddd0987-b900-4808-a71d-32c242150509
32.0: bp-2eb80900-51b3-4e4e-8953-c20432150509
33.1.1: bp-2d6b5342-97e8-48b7-8667-7291e2150509
34.0: bp-4b4a7cf7-553d-45d3-8021-96d422150509
35.0.1: bp-d368f9d0-10b5-441d-932d-0fc1c2150509
36.0.1: bp-8739192f-5118-4323-a64c-d01d52150509
37.0.2: bp-9067fc1f-5d34-49d4-92ef-a6bad2150509
38.0 BUILD3: bp-bde1b8fd-4cfd-4a87-b9fb-4e1aa2150509
39.0a2: bp-f92e447a-86ad-484f-a979-a6cc42150509
40.0a1 e10s: bp-55bfefe9-b9d3-454e-a959-cecaa2150509

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you for the log file.

I have a question, do you reproduce this bug without step #1? Looks like that the first composition is closed normally.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I meant step #2, not #1.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Oh, does #3 meant Alt key press -> Alt key release -> f key press -> f key release? I understood as Alt+F.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Ah, I see the cause. This must be an ancient bug.

IMEStateManager::OnInstalledMenuKeyboardListener(true) should be called when menubar is opened, but it's called when 'f' key is pressed. I'm not sure why this happens... Similar bug can be reproduced on Windows too.

Comment 12

[Alice0775 White]

> do you reproduce this bug without step #1? Looks like that the first composition is closed normally.
> I meant step #2, not #1.
No.

However, Browser(Nightly40.0a1 non-e10s) clashes with following str
STR
1. Focus SearchBar
2. Type "a" (without quotation marks) to popup autocomplete dropdown
3. IME on (Ctrl+space)
4. Alt key press -> Alt key release -> f key press -> f key release


> does #3 meant Alt key press -> Alt key release -> f key press -> f key release? 
Yes

Comment 13

[Alice0775 White]

s/clashes/crashes/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Hmm, I'm confused.

1. In the default settings, menubar should be autohide in Linux?
2. Do you set autohide true?
3. Alt key press -> Alt key release -> f key press -> f key release should open "File" menu, really?

Especially, #3, I cannot open the menu with non-autohide menubar. (And I don't find where is the implementation of this...)

Comment 15

[Alice0775 White]

1.
Menubar is hidden by default in Linux.
2.
Default, so, autohide true
3.
Yes, File menu open.

Comment 16

[Alice0775 White]

I think that you should be setting up "Gnome classic desktop".

On "Gnome unity desktop", I cannot reproduce the crash.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Alice0775 White from comment #16)
> I think that you should be setting up "Gnome classic desktop".

Yeah, I use it...

I got odd behavior. When I release Alt key, nsMenuBarFrame::InstallKeyboardNavigator() is called but nsXULPopupManager::UpdateKeyboardListeners() doesn't install key event listeners...

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I see what happens.

When there is a non-menu popup (e.g., suggest window of search engine or URL bar) and opens menubar by Alt key release, the existing popup isn't closed automatically. Therefore, IME is still available but the composition events are not fired on any editor since no editor has focus. So, this makes the situation, no editor manages composition but IME has composition and IME keeps composition after the menu is closed.

For fixing this issue, we should close all existing popups when a menubar becomes active.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=918cce6384f1

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39bab41b3288

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Could you confirm if the patch fixes this bug?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-39bab41b3288/

Comment 22

[Alice0775 White]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 - 5/6) from comment #21)
> Could you confirm if the patch fixes this bug?
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.
> com-39bab41b3288/

The try build(linux-i686) seems okay, no crash.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you!

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8603810 [details] [diff] [review]
Patch

See comment 18 for the detail.

Comment 25

[Hideo Oshima]

(In reply to Alice0775 White from comment #22)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 5/2 -
> 5/6) from comment #21)
> > Could you confirm if the patch fixes this bug?
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.
> > com-39bab41b3288/
> 
> The try build(linux-i686) seems okay, no crash.

The try build(linux-x86_64) seems also good.

Comment 26

[Neil Deakin]

Comment on attachment 8603810 [details] [diff] [review]
Patch

> void
> nsXULPopupManager::SetActiveMenuBar(nsMenuBarFrame* aMenuBar, bool aActivate)
> {
>-  if (aActivate)
>+  if (aActivate) {
>+    // When menubar becomes active, existing all popups should be closed
>+    // since keyboard navigation should be handled only by the menubar and
>+    // IME should be disabled in such case.
>+    if (mPopups) {
>+      Rollup(0, false, nullptr, nullptr);
>+    }
>     mActiveMenuBar = aMenuBar;

Since Rollup() will cause popups to hide and events to fire, aMenuBar may have deleted. As well, any methods higher in the stack won't be able to access frames without potentially crashing. Rollup() needs to be called as early as possible. If this is a crash that happens when releasing alt, the rollup should be called when the keyup happens. But I'm not sure we should be closing other popups when a menubar opens at all.

Note also that IME could also just listen for the DOMMenuBarActive event, or we could call some IME method at the same time.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Neil Deakin from comment #26)
> Comment on attachment 8603810 [details] [diff] [review]
> Patch
> 
> > void
> > nsXULPopupManager::SetActiveMenuBar(nsMenuBarFrame* aMenuBar, bool aActivate)
> > {
> >-  if (aActivate)
> >+  if (aActivate) {
> >+    // When menubar becomes active, existing all popups should be closed
> >+    // since keyboard navigation should be handled only by the menubar and
> >+    // IME should be disabled in such case.
> >+    if (mPopups) {
> >+      Rollup(0, false, nullptr, nullptr);
> >+    }
> >     mActiveMenuBar = aMenuBar;
> 
> Since Rollup() will cause popups to hide and events to fire, aMenuBar may
> have deleted. As well, any methods higher in the stack won't be able to
> access frames without potentially crashing. Rollup() needs to be called as
> early as possible. If this is a crash that happens when releasing alt, the
> rollup should be called when the keyup happens.

Good point.

> But I'm not sure we should
> be closing other popups when a menubar opens at all.

I believe that all other popups should be closed because it's odd keys are handled by both a popup and menubar. Actually, I was confused by such behavior at testing. The only problem of closing all popups is, menubar *might* be on a popup. However, I don't think that such case is realistic scenario.

> Note also that IME could also just listen for the DOMMenuBarActive event, or
> we could call some IME method at the same time.

Hmm, that's true but we need to disable IME when menu is open. So, I think that at installing key navigator is the best timing to notify widget of IME state change.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Patch (with test)

As I mentioned above, I still believe that we should close all popups when a menubar gets pseudo focus because the menubar needs specific IME state (disabled) and should handle all key events rather than already opened popups.

This patch also includes automated test which I succeeded to create!

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Enn: ping

Comment 30

[Neil Deakin]

Comment on attachment 8606634 [details] [diff] [review]
Patch (with test)

>+        // If menubar active state is changed or the menubar is destroyed
>+        // during closing the popups, we should do nothing anymore.
>+        toggleMenuActiveState = !Destroyed() && !mMenuBarFrame->IsActive();

If mMenuBarFrame was destroyed then 'this' would be destroyed as well. The event caller adds a reference to 'this' right? Otherwise, we need to keep one.

You might want to just ask the other Neil ([e-mail]) if he sees any issue with closing popups when a menubar opens.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Neil Deakin from comment #30)
> Comment on attachment 8606634 [details] [diff] [review]
> Patch (with test)
> 
> >+        // If menubar active state is changed or the menubar is destroyed
> >+        // during closing the popups, we should do nothing anymore.
> >+        toggleMenuActiveState = !Destroyed() && !mMenuBarFrame->IsActive();
> 
> If mMenuBarFrame was destroyed then 'this' would be destroyed as well. The
> event caller adds a reference to 'this' right?

I think so. The event dispatcher should hold the event listener during calling the event handler.

> You might want to just ask the other Neil ([e-mail]) if he sees any
> issue with closing popups when a menubar opens.

Okay, thanks!

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8606634 [details] [diff] [review]
Patch (with test)

Neil, could you review this?

When menubar becomes active, we should close all popups because existing popups shouldn't handle key events, the user must want to operate in the menubar. Then, IME state is disabled as expected for allow to navigate menubar with keyboard.

The point which I want to ask you is, if it may cause some problems when this patch closes popups during menubar being activated.

Comment 33

[[e-mail]]

Comment on attachment 8606634 [details] [diff] [review]
Patch (with test)

Ah yes, if you press and release Alt while a popup (doorhanger/autocomplete for instance) is open, then things get confused, so I would agree to this change.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcca18bc301

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dcca18bc301