1504745 - Crash in nsMenuPopupFrame::ShouldFollowAnchor when machine is under load

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Edgaras]

Firefox crashes when copying text from address bar if:
- computer is under load 
- and ui.osk.detect_physical_keyboard=false

I can easily trigger crash on old computer with 4GB RAM, HDD, Intel Core2 Duo @ 2x 3.00GHz.
But I cannot reproduce on new computer with 16GB RAM, SSD, Intel i7 @16x 3.60GHz.

Everything else same: Windows 10 x64, Firefox x64 beta64. Computers have physical keyboards, screens are not touchscreens.

To trigger load, I start Windows Automatic Maintenance. Installing Windows Updates or another HDD heavy task can also be enough heavy.

With ui.osk.detect_physical_keyboard=true, I tried to trigger crash, but I couldn't.

If I switch value to false (no Firefox restart necessary), I can trigger crash within few minutes. Before crashing, Firefox becomes "Not responding" for 5-10 seconds.

It is probably due to some timeout or race condition if Firefox fails to open touch keyboard fast enough on slower computer.

Crash reports:
bp-28675c9f-ac12-4cd7-8894-19f840181105 	2018-11-05 18:19 	
bp-7dbfa23d-2223-4418-9070-4d9e70181105 	2018-11-05 18:14 	
bp-98eea061-4834-46e9-bea1-381f70181105 	2018-11-05 18:02 	
bp-c8945407-dc82-45c2-9833-dd1a00181105 	2018-11-05 17:47
bp-e1fb252c-ff57-42ef-8c3c-d30500181024 	2018-10-24 20:19
bp-72ec254a-6757-4bd8-a6fc-b26ba0180915 	2018-09-15 16:41

---

I was asked to open this bug report from possibly related bug 1444860.

Comment 1

[Marco Zehe (:MarcoZ)]

Moving this to a more appropriate component and product. Please triage and forward if necessary. Thanks!

Comment 2

[Edgaras]

I've tried mozregression-gui between release 45 (good) and 64 (bad)

20170519 good
20180204 bad
20170927 good
20171201 bad
20171030 good
20171115 bad
20171107 retry + skip (high cpu load)
20171111 bad
20171105 bad
20171103 good
20171104 bad

INFO : Narrowed nightly regression window from [2017-11-03, 2017-11-05] (2 days) to [2017-11-03, 2017-11-04] (1 days) (~0 steps left)

As I understood, pushlog for this window is
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=66f496680fae6e7d8f02bc17ff58b9234ee07c70&tochange=e7fee7042d971d73c0e5caafba3b8d15da1bc8ca

After that, mozregression-gui failed with (like in bug 1263358)
message: IndexError: list index out of range
traceback:   File ".\mozregui\bisection.py", line 82, in check_merge

Comment 3

[Neil Deakin]

I don't see anything within nsMenuPopupFrame that could cause this crash. Bug 1444860 suggests copying text from the address field can cause this, but I can't reproduce.  

However, in the crash reports, the stack traces imply that it always occurs while opening the on-screen keyboard on Windows when focusing a textbox. ShellExecute is being run to open the keyboard, but (I think) while waiting for it to open, a mouse event which opens a popup menu is processed.

Let's move this bug there to start. 

As an aside, I can't get the onscreen keyboard to open. ShellExecute runs and appears to return success but no keyboard appears. 

masayuki, do you know anything about this that could help here?

Comment 4

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

Hmm, in nsXULPopupManager::ShowPopupCallback(), the instance is newly created.
https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/layout/xul/nsXULPopupManager.cpp#895-896,936-937,940
Then, it's set to nsXULPopupManager::mPopups. Then, SetCaptureState() is called. Finally, nsMenuPopupFrame::ShouldFollowAnchor() is called via UpdateFollowAnchor().

nsMenuChainItem is destroyed with a call of nsXULPopupManager::PopupDestroyed(), nsXULPopupManager::HidePopupsInDocShell() or nsXULPopupManager::HidePopupCallback().

So, if a call of SetCaptureState() destroying a call of one of them, this crash may occur. Is it possible to release the refcount of mWidget from 1 to 0 and destroying the widget causes destroying the nsMenupPopupFrame()?

Comment 5

[Neil Deakin]

There is a weak frame check on the frame just before UpdateFollowAnchor, so it should be ok for the frame to be destroyed.

However, you're right that it is possible that the nsMenuChainItem could be destroyed as well. I'll see if I can find a fix for that.

Comment 6

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

Cannot make nsMenuChainItem treated with UniquePtr or RefPtr object?

Comment 7

[Neil Deakin]

Attachment: Update the follow state before SetCaptureState as the nsMenuChainItem now stored in mPopups could be destroyed

I think in this case UpdateFollowAnchor can just be called before SetCaptureState and before it is assigned to mPopups.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/881fd89a400e
update the follow state before SetCaptureState as the nsMenuChainItem now stored in mPopups could be destroyed, r=tn

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/881fd89a400e