952170 - FX OS crash in mozilla::dom::TabChild::UpdateTapState(mozilla::WidgetTouchEvent const&, nsEventStatus)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-af8f8ccd-8e1c-4fc8-8322-64b1f2131219.
=============================================================

Seen on Buri while running the latest nightly.

Gaia   e6276c96fcf087a12800419be8a9e25fcbbc924d
SourceStamp 5c7fa2bfea8b
BuildID 20131219040202
Version 29.0a1

STR:
1. Last thing done on the phone was operating in gallery (editing a photo). Put phone in my pocket and when I retrieved it received this crash.

No logcat available since I was not tethered at the time.

Comment 3

[Jason Smith [:jsmith]]

This has to be a regression from something that landed yesterday.

Comment 4

[Jason Smith [:jsmith]]

Vivien - Did bug 950489 cause this crash? It appears to have touched the TabChild.cpp file within the regression range that matches the crash stack.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I assume it should be related. Will try to dig into it. I can reproduce with Naoki's step from one of the dup.

STR: 
Occurred when trying to update a contact with a picture really quick after creating a new contact with an image from the gallery.
(ie. gallery opened / closed and tried to open it again )

Comment 6

[Jason Smith [:jsmith]]

With bug 949347 now fixed, this crash is being hit instead during our smoketests, so this is now a smoketest blocker.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Remove unnecessary cast

I don't think this will fix the problem but unnecessary casts like this bug me.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add explicit copy constructor

I suspect this might be the problem. WidgetTouchEvent contains an nsTArray but has no explicit copy constructor, so when we do things like WidgetTouchEvent localEvent(aEvent) I think the nsTArray doesn't get copied properly. Specifically I don't think the refcount of the touches in the list gets incremented, and so they could get deleted out from under us, resulting in a dangling pointer and this error. I haven't confirmed this theory but putting the patch up anyway - it should be safe to land even if this isn't the problem.

Try push for builds at https://tbpl.mozilla.org/?tree=Try&rev=e3245179b723

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I feel a bit uncomfortable reviewing those. Is Chris Lord a good reviewer for those?

Comment 10

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

/me reviews

Comment 11

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

Comment on attachment 8356087 [details] [diff] [review]
Add explicit copy constructor

Could you rather fix AssignTouchEventData and
disabled copy-ctor.
Or if that ends up being tricky for some reason, fix AssignTouchEventData
and use that in copy-ctor.

Comment 12

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

Comment on attachment 8356087 [details] [diff] [review]
Add explicit copy constructor

Or actually, this setup is rather error prone already, and the patch
doesn't make it any worse.
I'll file a followup to clean up this.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Also I screwed up that try push. Better one here:

https://tbpl.mozilla.org/?tree=Try&rev=9679bb7f7957

Comment 14

[Kartikaya Gupta (email:[e-mail])]

The try run got a green build for each platform so I aborted the rest and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/343ac499c993
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2f2356f3b6

Comment 15

[Preeti Raghunath(:Preeti)]

Kats,

Is anything pending to land for the solution to be completed here?

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Not really. But I don't know if the patches I landed will actually fix the crash or not. If the STR are reliable can somebody from QA test a build with these patches to see if it fixes the problem?

Comment 17

[Brogan Zumwalt [Inactive]]

Issue appears to occur reliably if user scrolls when loading any app that has ability to horizontally or vertically scroll (e.g. Calender, Gallery, Homescreen [from lockscreen])

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/343ac499c993
https://hg.mozilla.org/mozilla-central/rev/bf2f2356f3b6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/758629e75ddf
https://hg.mozilla.org/releases/mozilla-aurora/rev/d8852933a8b4

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Whoops, I probably should have marked this as no_uplift until we verified the fix. Ah well the uplift shouldn't hurt. But we'll still need to verify the fix.

Comment 22

[Jason Smith [:jsmith]]

We just confirmed that the patches here didn't fix the crash, so I'm reopening.

Comment 23

[Jason Smith [:jsmith]]

Example - https://crash-stats.mozilla.com/report/index/0f3eb82f-5e5c-43a3-8ebd-cb0ae2140107

Comment 24

[Preeti Raghunath(:Preeti)]

Kats,

To make sure this blocker is on your radar I am ni you.

As you know this is a QC blocker and they are expecting a fix by 1/10/14.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Jason, did you try backing out bug 950489 to see if it fixes the crash?

Comment 26

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Jason, did you try backing out bug 950489 to see if it fixes the crash?

Not yet.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Interesting to note that this crash has actually been around for a long time. See for instance https://crash-stats.mozilla.com/report/index/4535287d-a194-4524-a005-649e32131215 which occurred on B2G-18 build from 20130621133927. The frequency just spiked way up on Dec 19, probably from the landing of bug 950489. Backing that out will probably reduce the frequency back to old levels but won't fix it completely.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Also, this crash should only happen when APZC is disabled, and as of last night APZC is enabled by default so this crash shouldn't get very often. Nonetheless I will spend some time today to see if I can figure it out.

Comment 32

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> Also, this crash should only happen when APZC is disabled, and as of last
> night APZC is enabled by default so this crash shouldn't get very often.
> Nonetheless I will spend some time today to see if I can figure it out.

Per talking w/Marcia - she mentioned on a build with APZC enabled, the crash rate is definitely lower. However, it's still possible to hit in Gaia. She can add more details on the reproduction steps.

Comment 33

[Marcia Knous [:marcia]]

Here are the steps from my crash today:

1. Incoming call log, select a new number and "Create a new contact"
2. Contacts app: Start typing a name in the first name field, and then backspace a few times using the delete key on the keyboard

Comment 34

[Jason Smith [:jsmith]]

No longer a smoketest blocker with turning APZC on.

Comment 35

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: bug952170.patch

I put in a patch just to null check and see what was going on.  without APZ turned on, I got an error : 
01-09 17:09:32.759: E/GeckoConsole(1653): [JavaScript Error: "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIContentFrameMessageManager.content]" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 737}]

And I did not crash.  Having said that I don't think I fixed it, something else must have fixed it.  We should probably check in tomorrow's build if we crash or not without APZ turned on.  If we still do, we can try creating a new build and seeing if someone else crashes with this patch?

Gaia      a5288df8de82cd84be14391c96f99a140535a78e
Gecko     78ebe7f47e81d6ffbaff3ff12f32597aca457205
BuildID   20140109155817
Version   29.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

Comment 36

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Apparently, this still crashed even with APZ turned on according to the smoke testers.

Comment 37

[Preeti Raghunath(:Preeti)]

Kats,

Please prioritize this blocker. it blocks QC

Comment 38

[Kartikaya Gupta (email:[e-mail])]

I have not yet been able to reproduce this. I'll keep trying.

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8358119 [details] [diff] [review]
bug952170.patch

Review of attachment 8358119 [details] [diff] [review]:
-----------------------------------------------------------------

I assume that since you didn't mention it, you never saw the "touch is null" output get hit, which means that the null check here never triggered at all. So I don't think this patch will have much effect.

Also according to the crashreports the crashes were a SIGBUS rather than a SIGSEGV so I don't think it's a null pointer dereference that's causing the problem.

Comment 40

[Marcia Knous [:marcia]]

(In reply to Marcia Knous [:marcia - use needinfo] from comment #33)
> Here are the steps from my crash today:
> 
> 1. Incoming call log, select a new number and "Create a new contact"
> 2. Contacts app: Start typing a name in the first name field, and then
> backspace a few times using the delete key on the keyboard

Another way I was able to reproduce it three times in a row using today's build involved installing the XE Currency app and then trying to add a currency by Country - typing a few characters in the text field caused the crash to happen three times: https://crash-stats.mozilla.com/report/index/58d37b4e-53c2-40a2-9b89-9e7d82140113

Comment 41

[Zac C (:zac)]

Caveat: Slightly older build and on Peak

https://crash-stats.mozilla.com/report/index/fd0896c7-ec57-4f67-b35e-7de842140113

STR:
1. Tap settings icon to load app.
2. Immediately tap the screen again before the app loads fully.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

I think I can reproduce it now, using the STR in comment 41 along with a call to nsDOMWindowUtils::GarbageCollectNow() just before the UpdateTapState call at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=49c6393d37f3#1875

Investigating...

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Well that was a bit anticlimactic. DispatchWidgetEvent was removing all the touch points from the event, so when we did event.touches[0] we got back a pointer that had a 0x1 value. Dereferencing that caused the SIGBUS.

The GarbageCollect() call I mentioned above seems unrelated except that maybe it slows down things a bit and widens the window in which I could trigger the bug.

Comment 44

[Brogan Zumwalt [Inactive]]

I am hitting this crash with a 100% repro rate by replying to an email in the Email app and typing in the body field. The keyboard disappears after a few characters are typed. When this process of selecting the body and attempting to type is repeated 1 to 4 more times this crash is experienced. 

Environmental Variables
Device: Buri v 1.4 Mozilla RIL
Build ID: 20140113040202
Gecko: http://hg.mozilla.org/mozilla-central/rev/12d3ba62a599
Gaia: fd3b9a97cb3c41cfa56be387b46a51db136b4422
Platform Version: 29.0a1
Firmware Version: V1.2-device.cfg

Comment 45

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

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

I think we should just null check, since if we remove stuff from the touch list,
we didn't dispatch events for the touch.

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Ok, that's even easier then.

Comment 47

[Martijn Wargers (dead)]

I get this crash after a while, while running:
http://people.mozilla.org/~mwargers/tests/contenteditable/contenteditable_focus_blur_parentframe.htm

Comment 48

[Kartikaya Gupta (email:[e-mail])]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #47)
> I get this crash after a while, while running:
> http://people.mozilla.org/~mwargers/tests/contenteditable/
> contenteditable_focus_blur_parentframe.htm

Presumably this is without the patch?

Comment 49

[Kartikaya Gupta (email:[e-mail])]

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

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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