1312997 - Crash in nsWeakFrame::Init

Status: RESOLVED FIXED

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

Description

[Ting-Yu Chou [:ting] (away)]

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.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> Aug 14)]

Just got it with Aurora too: https://crash-stats.mozilla.com/report/index/9072b29e-1d5c-4353-97cc-fffa02161104

Comment 2

[Mats Palmgren (inactive)]

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
[...]

Comment 3

[Mats Palmgren (inactive)]

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?

Comment 4

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

P3 according to the low volume.

CC Stone to see if he has something to chime in.

Comment 5

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

PresShell::HandleEventInternal keeps ESM alive in a stack variable.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: fix?

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

Comment 8

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

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.

Comment 9

[Mats Palmgren (inactive)]

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

Good point, I'll skip those two null-checks.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a2a329a708
Store 'aTargetFrame' in 'mCurrentTarget' before doing anything else, then use 'mCurrentTarget' throughout PostHandleEvent.  r=smaug

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8815037 [details] [diff] [review]
fix?

crash fix for aurora52

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99348c6bd30b

Comment 15

[Gerry Chang [:gchang]]

Mark 51 won't fix as the volume of crash in Beta51 is very low.

Comment 16

[Mats Palmgren (inactive)]

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.