Closed Bug 1133629 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Editor, defect, critical)

35 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed

People

(Reporter: mats, Assigned: masayuki)

Details

(Keywords: crash, platform-parity, topcrash)

Crash Data

Attachments

(1 file)

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)
...
[Tracking Requested - why for this release]: topcrash
(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...
Attached patch PatchSplinter Review
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8565344 - Flags: review?(bugs)
FYI: crashed here:
nsIWidget* topLevelWidget = mWidget->GetTopLevelWidget();

The cause is accessing null pointer.
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.
Attachment #8565344 - Flags: review?(bugs) → review+
(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)
https://hg.mozilla.org/mozilla-central/rev/efaae84c2ed0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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 on attachment 8565344 [details] [diff] [review]
Patch

This looks correct and safe for RC to me, fwiw.
Attachment #8565344 - Flags: review+
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.)
Flags: needinfo?(lmandel)
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.
Flags: needinfo?(lmandel)
Flags: needinfo?(kairo)
Flags: needinfo?(ctalbert)
Masayuki-san, thanks for the quick fix.  Can you request branch approvals please?
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(ctalbert)
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.
Flags: needinfo?(masayuki)
Attachment #8565344 - Flags: approval-mozilla-beta?
Attachment #8565344 - Flags: approval-mozilla-aurora?
(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.
Flags: needinfo?(kairo)
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+
Attachment #8565344 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It was indeed too late for 36
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.
Attachment #8565344 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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)?
Flags: needinfo?(sledru)
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.
Flags: needinfo?(sledru)
Attachment #8565344 - Flags: approval-mozilla-release+
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
You need to log in before you can comment on or make changes to this bug.