1133629 - crash in mozilla::IMEContentObserver::OnMouseButtonEvent(nsPresContext*, mozilla::WidgetMouseEvent*)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-fb8195e3-04a5-4890-8075-28ac52150212.
=============================================================


This signature started in v35, and still occurs in 36/37/38.
It's currently at #16 on the "Top Crashers for Firefox 35.0" list.
All crashes are on Windows.
There are many user comments - several mention logging in at Juno, eg:
"I was logging on to Juno.com and it crashed."


mozilla::IMEContentObserver::OnMouseButtonEvent(nsPresContext*, mozilla::WidgetMouseEvent*)
mozilla::IMEStateManager::OnMouseButtonEventInEditor(nsPresContext*, nsIContent*, nsIDOMMouseEvent*)
nsEditorEventListener::NotifyIMEOfMouseButtonEvent(nsIDOMMouseEvent*)
nsEditorEventListener::HandleEvent(nsIDOMEvent*)
mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*)
mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)
mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*)
PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*)
PresShell::HandlePositionedEvent(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*)
PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)
PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*)
nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*)
nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool)
nsWindow::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&)
nsWindow::DispatchWindowEvent(mozilla::WidgetGUIEvent*)
nsWindow::DispatchMouseEvent(unsigned int, unsigned int, long, bool, short, unsigned short)
nsWindow::ProcessMessage(unsigned int, unsigned int&, long&, long*)
nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
CallWindowProcCrashProtected
nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
InternalCallWinProc
UserCallWinProcCheckWow
DispatchMessageWorker
DispatchMessageW
nsAppShell::ProcessNextNativeEvent(bool)
...

Comment 1

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]: topcrash

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Mats Palmgren (:mats) from comment #0)
> It's currently at #16 on the "Top Crashers for Firefox 35.0" list.

Wow, I'm surprised at the fact. I was thinking this feature is used by not so many users.

> All crashes are on Windows.
> There are many user comments - several mention logging in at Juno, eg:
> "I was logging on to Juno.com and it crashed."

Hmm, wired comment. It's called only when mouse button is pressed or released on composition string...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch

I have no idea to reproduce this bug, though.

The feature is enabled even if there is no composition. I think that the cause is IMEContentObserver is destroyed by flushing pending layout at initializing ContentEventHandler. So, using kungFuDeathGrip and checking mWidget after using ContentEventHandler must fix this bug.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

FYI: crashed here:
nsIWidget* topLevelWidget = mWidget->GetTopLevelWidget();

The cause is accessing null pointer.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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


>+  nsRefPtr<IMEContentObserver> kungFuDeathGrip(this);
The right way for this would be to make caller to have a strong reference to the IMEContentObserver object.

But fine.


We may need a bug to change IMEStateManager so that it always takes a strong reference to 
sActiveIMEContentObserver before using it.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8565344 [details] [diff] [review]
> Patch
> 
> 
> >+  nsRefPtr<IMEContentObserver> kungFuDeathGrip(this);
> The right way for this would be to make caller to have a strong reference to
> the IMEContentObserver object.
> 
> But fine.
> 
> 
> We may need a bug to change IMEStateManager so that it always takes a strong
> reference to 
> sActiveIMEContentObserver before using it.

Ah, good point. But we should go with the patch for now. (it's safer)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efaae84c2ed0

Comment 8

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking topcrash for 36+. Note that with 36 building Beta 10 today, in order to take this fix in the 36 release the fix will need to be verified and safe as it will have to land in the RC. It may be preferable to wait until 37 for this fix.

Comment 10

[Mats Palmgren (inactive)]

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

This looks correct and safe for RC to me, fwiw.

Comment 11

[Mats Palmgren (inactive)]

I'm a bit worried that a high volume crash like this can slip past our radar
for an entire cycle without being reported.  Can we put some process in place
so that crashes like this gets reported earlier?  (I only caught this one by
accident while looking for something else.)

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

ni kairo and ctalbert to respond to comment 11.

Mats - We certainly have work to do to prevent issues from falling through the cracks. I'll add this one to the list but would like to hear from Kairo and Clint as our process right now has QE filing bugs based on Socorro data.

Comment 13

[Mats Palmgren (inactive)]

Masayuki-san, thanks for the quick fix.  Can you request branch approvals please?

Comment 14

[cmtalbert]

Yeah, this sucks. I think we missed it with all the other things we were tracking in 36.  We will work to close this gap going forward.

Thanks for catching it mats.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Approval Request Comment
[Feature/regressing bug #]: bug 826657
[User impact if declined]: 16th topcrash
[Describe test coverage new/current, TreeHerder]: already landed on m-c.
[Risks and why]: Nothing, this grabs the instance with kungFuDeathGrip before calling a method which might cause destroying the instance and adding null check of its member after the method is called.
[String/UUID change made/needed]: No.

Comment 16

[Robert Kaiser]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> I'll add this one to the list but would like to hear from Kairo
> and Clint as our process right now has QE filing bugs based on Socorro data.

The process is that I file those that I see and that I have time for, starting from the largest priorities down, and focusing mostly on beta as long as its crash rate is higher than it should be, as well as large offenders on Aurora/DevEdition.
When overall rates on release are good, as they are on 35 atm, I don't look at topcrashes there too much, esp. not at stuff below #10, esp. if there are enough higher priorities (see above), and this cycle we had enough of those.
And yes, I definitely can use help in this area.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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

I'm approving for 37 and leaving it to Sylvestre to decide if and when we can take this in 36 because of the very late timing. Aurora+

Comment 18

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1d8467a5119

Comment 19

[Sylvestre Ledru [:Sylvestre]]

It was indeed too late for 36

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Marking Beta- as we decided that it was too late to take this fix in 36. Note that Beta is currently 37, which has this fix as it was uploaded while 37 was on Aurora.

Comment 21

[Robert Kaiser]

This is a top-20 crash in 36 release so far even though IME is only used for a small portion of our audience.
Sylvestre, should we take this for 36.0.1 (also setting status back to affected for that assessment)?

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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

[Triage Comment]
OK, we can take this as we are going to do a 36.0.1, Masayuki considers it is safe and it seems it didn't cause regression in 37.

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-release/rev/f753656b2806

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Verified Socorro data for the past 4 weeks, and this looks good so far:
39.0a1 - 0 crashes
38.0a2 - 0 crashes
38.0a1 (builds after Feb 17) - 0 crashes
37.0a2 (builds after Feb 20) - 0 crashes
37.0b - 0 crashes
36.0.1 - need more time to gather data

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=mozilla%3A%3AIMEContentObserver%3A%3AOnMouseButtonEvent%28nsPresContext%2A%2C+mozilla%3A%3AWidgetMouseEvent%2A%29#tab-sigsummary