Closed Bug 1312997 Opened 3 years ago Closed 3 years ago

Crash in nsWeakFrame::Init

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ting, Assigned: mats)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-91ecd8ae-d60f-4836-abaf-53e462161024.
=============================================================
#21 of Nightly 20161023030206 on Linux, 1 crash. There were 1417 crashes with this signature in the last 6 months.
Even though nsWeakFrame::Init is the top stack frame here, I'm pretty sure
the bug isn't in nsWeakFrame, or anything else in Layout.  The rest of
the stack looks more like Event Handling to me:

0 	nsWeakFrame::Init
1 	mozilla::EventStateManager::PostHandleEvent
2 	PresShell::HandleEventInternal
3 	PresShell::HandlePositionedEvent
4 	PresShell::HandleEvent
5 	nsViewManager::DispatchEvent
6 	nsView::HandleEvent
7 	nsWindow::DispatchEvent
8 	nsBaseWidget::ProcessUntransformedAPZEvent
9 	nsBaseWidget::DispatchInputEvent
10 	nsWindow::DispatchDragEvent
11 	nsDragService::RunScheduledTask
12 	nsDragService::TaskDispatchCallback
Ø 13 	libglib-2.0.so.0.5000.1@0x4a689
Ø 14 	libglib-2.0.so.0.5000.1@0x31291f
15 	nsDragService::RunScheduledTask
[...]
Component: Layout → Event Handling
Stack frame #1 points to layout/generic/nsIFrame.h:3552 which is the Init
call in nsWeakFrame::operator=.  I suspect the line in PostHandleEvent
might be this one:
http://searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/dom/events/EventStateManager.cpp#2912
Perhaps HandleCrossProcessEvent did something that caused the ESM or PresShell
to get destroyed?
P3 according to the low volume.

CC Stone to see if he has something to chime in.
Priority: -- → P3
PresShell::HandleEventInternal keeps ESM alive in a stack variable.
OK.  Maybe the problem is that 'aTargetFrame' is destroyed
by the HandleCrossProcessEvent call somehow?

There are also some crashes in EventStateManager::ComputeScrollTarget
which looks like the same underlying bug:
bp-3e68ed57-add1-411f-965c-d7c692161127
Crash Signature: [@ nsWeakFrame::Init] → [@ nsWeakFrame::Init] [@ mozilla::EventStateManager::ComputeScrollTarget ]
Attached patch fix?Splinter Review
This moves the "mCurrentTarget = aTargetFrame" assignment before
the call to HandleCrossProcessEvent, so if the frame was destroyed
by that call mCurrentTarget will be null.  The rest of the method
already deals with a null target frame so we only need to make sure
we use mCurrentTarget instead.

WDYT?

Seems to pass tests, fwiw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ab6c18a59a02a4c789a4e95c98d036ca55e2abe&exclusion_profile=false
Attachment #8815037 - Flags: review?(bugs)
Comment on attachment 8815037 [details] [diff] [review]
fix?

I don't see how HandleCrossProcessEvent could cause the frame to be destroyed. But we can try this.

Don't know the reason for the null check in 
-          DoScrollZoom(aTargetFrame, intDelta);
+          if (mCurrentTarget) {
+            DoScrollZoom(mCurrentTarget, intDelta);
+          }
Perhaps remove it?

Also, why the null check in EventStateManager::ComputeScrollTarget?

Usually EventStateManager::PostHandleEvent returns early when mCurrentTarget is null.

So, either explain those null checks or remove them.
Attachment #8815037 - Flags: review?(bugs) → review+
> Usually EventStateManager::PostHandleEvent returns early when mCurrentTarget is null.

Good point, I'll skip those two null-checks.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a2a329a708
Store 'aTargetFrame' in 'mCurrentTarget' before doing anything else, then use 'mCurrentTarget' throughout PostHandleEvent.  r=smaug
https://hg.mozilla.org/mozilla-central/rev/f4a2a329a708
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(mats)
Assignee: nobody → mats
Flags: needinfo?(mats)
Comment on attachment 8815037 [details] [diff] [review]
fix?

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: crash
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: trivial change
[String changes made/needed]: none

There are no new reported crashes for either signature in the past 14 days
in v53, fwiw.  It's a low-risk patch so it seems worth uplifting to Aurora.
Attachment #8815037 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mats)
Comment on attachment 8815037 [details] [diff] [review]
fix?

crash fix for aurora52
Attachment #8815037 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mark 51 won't fix as the volume of crash in Beta51 is very low.
There are a total of 8 crashes in v52 and 3 in v53b1 with this signature,
none of which have PostHandleEvent on the stack, so I believe this fix was effective.
Flags: needinfo?(mats)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.