Closed Bug 1163304 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: XUL, defect)

Product:
Core
Component:
XUL
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix

People

(Reporter: hidenosuke, Assigned: masayuki)

References

Details

(4 keywords)

Attachments

(2 files, 2 obsolete files)

log.txt
32.58 KB, text/plain
Details
Patch (with test)
6.79 KB, patch
enndeakin
: review+
neil
: review+
Details | Diff | Splinter Review 
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
Severity: normal → critical
Component: General → Editor
Flags: needinfo?(masayuki)
Keywords: crash
Product: Firefox → Core
Version: unspecified → Trunk
This must be a bug in widget/gtk/nsGtkIMModule.cpp.

Oshima-san, what IME are you using? iBus?
Flags: needinfo?(masayuki) → needinfo?(hidenosuke)
(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).
Flags: needinfo?(hidenosuke)
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?
(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.
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!)
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.
I meant step #2, not #1.
Oh, does #3 meant Alt key press -> Alt key release -> f key press -> f key release? I understood as Alt+F.
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.
> 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
s/clashes/crashes/
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...)
1.
Menubar is hidden by default in Linux.
2.
Default, so, autohide true
3.
Yes, File menu open.
I think that you should be setting up "Gnome classic desktop".

On "Gnome unity desktop", I cannot reproduce the crash.
(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...
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: Editor → XP Toolkit/Widgets: Menus
OS: Unspecified → Linux
Hardware: Unspecified → All
Could you confirm if the patch fixes this bug?
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/masayuki@d-toybox.com-39bab41b3288/
Flags: needinfo?(hidenosuke)
Flags: needinfo?(alice0775)
(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.
Flags: needinfo?(alice0775)
Thank you!
Flags: needinfo?(hidenosuke)
(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 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.
Attachment #8603810 - Flags: review?(enndeakin) → review-
(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.
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!
Attachment #8603810 - Attachment is obsolete: true
Attachment #8606634 - Flags: review?(enndeakin)
Enn: ping
Flags: needinfo?(enndeakin)
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 (neil@httl.net) if he sees any issue with closing popups when a menubar opens.
Flags: needinfo?(enndeakin)
Attachment #8606634 - Flags: review?(enndeakin) → review+
(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 (neil@httl.net) if he sees any
> issue with closing popups when a menubar opens.

Okay, thanks!
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.
Attachment #8606634 - Flags: review?(neil)
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.
Attachment #8606634 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/0dcca18bc301
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: