917515 - crash in mozilla::Selection::Collapse(nsINode*, int)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-65d28770-7c32-4591-a09e-3fc412130916.
=============================================================

https://crash-stats.mozilla.com/report/list?product=FennecAndroid&range_value=7&range_unit=days&signature=mozilla%3A%3ASelection%3A%3ACollapse%28nsINode*%2C+int%29&version=FennecAndroid%3A26.0a1

This crashes much more often in Firefox 25. Requesting tracking for the increase in crashes.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

It's inside IME code, and steps would be nice.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for FF25 then while QA looks for steps, what's the volume like on 26/27?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

I think this is a regression from bug 860123 (which I can't access). More specifically, presContext could be NULL at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#4385

Can you take a look, Blake?

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

lsblakk I was waiting on beta data before raking this crash. This is the number 2 top crash on Beta.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

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

I spent a bunch of time looking into this today. The situation we're in is that we have a mozilla::dom::Selection that has a non-null mFrameSelection who's mShell member is null. The only way this can happen is after the presshell for the given document has been torn down (and we've disconnected the various selection objects). We used to soldier on only not doing stuff that required a presContext and now we crash.

The patch I'm trying right now errs on the side of caution and throws in this case. Jim, would you mind testing to see if it breaks Android IME? I assume we're not running tests for it on tinderbox given that the original patch was green there.

Mats, do you understand this code well enough to say if this patch is likely to break things? I doubt that selections on torn-down documents are actually useful, but I'm not sure.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=edab5fb2a435
> 
> I spent a bunch of time looking into this today. The situation we're in is
> that we have a mozilla::dom::Selection that has a non-null mFrameSelection
> who's mShell member is null. The only way this can happen is after the
> presshell for the given document has been torn down (and we've disconnected
> the various selection objects). We used to soldier on only not doing stuff
> that required a presContext and now we crash.

I think we can also get a null PresShell after nsTextEditorState::UnbindFromFrame [1] calls nsTextInputSelectionImpl::SetScrollableFrame(null) [2]. This can happen when the editor is switching to using a new frame. In this case, this old frame selection is discarded anyways (we create a new frame selection during nsTextEditorState::BindToFrame), so I think it makes sense to throw here and not proceed.

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#1555
[2] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#285

> The patch I'm trying right now errs on the side of caution and throws in
> this case. Jim, would you mind testing to see if it breaks Android IME? I
> assume we're not running tests for it on tinderbox given that the original
> patch was green there.

Unfortunately we don't have definite steps to reproduce, but I don't think it will have much impact (other than make it not crash :) )

Comment 7

[Mats Palmgren (inactive)]

I don't think the Try patch will break anything...

It feels like wallpaper though -- we shouldn't be here with a null pres shell/context.
Looking at the stack in comment 0 - PresShell::HandleEvent doesn't dispatch events
if it's teared down:
http://hg.mozilla.org/mozilla-central/annotate/57d160eda301/layout/base/nsPresShell.cpp#l6018

So it looks like something from stack frame #6 and forward destroyed the shell
(which seems unlikely) -or- we somehow got a selection associated with a
different shell.  Looking at ESM::PreHandleEvent:
http://hg.mozilla.org/mozilla-central/annotate/57d160eda301/content/events/src/nsEventStateManager.cpp#l1061
we give 'mPresContext' to nsContentEventHandler but it doesn't use that in
OnSelectionEvent, but instead asks IME for the selection (GetFocusSelectionAndRoot):
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#1055

My hunch is that the real bug is that IME gives us a stale selection.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Hey Blake, do you need any more information from me? Like I said in comment 6, I don't expect throwing an error to cause IME to break on Android.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, I must have lost the comment I wrote there...

Could you look into the last sentence of comment 7? It seems like something to fix regardless of this bug (I'll attach my patch with the wallpaper later today).

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Wallpaper

As Mats said, this is probably a wallpaper, but it's also the safest patch to fix the crash on Aurora and Beta. We should probably take it and look at the underlying cause in a followup.

Comment 12

[Aaron Train [:aaronmt]]

i) http://imgur.com
ii) Tap the top left green button
iii) Enter text, and hit submit

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5f51249cb8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d5f51249cb8

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> Sorry, I must have lost the comment I wrote there...
> 
> Could you look into the last sentence of comment 7? It seems like something
> to fix regardless of this bug (I'll attach my patch with the wallpaper later
> today).

I looked at it more yesterday using the STR from comment 12. After pressing the enter key, the input field becomes hidden, and the editor is unbound from its frame as the frame is destroyed. However, the focus appeared to stay inside the input field. In this situation, IME still sends selection events to content because the input field is still focused. However, because the selection was discarded when unbinding from frame, the selection no longer has a PresContext and the call fails. I'm not sure where the true bug is in this chain of events, or if there's a bug at all.

Comment 16

[Aaron Train [:aaronmt]]

(In reply to Aaron Train [:aaronmt] from comment #12)
> i) http://imgur.com
> ii) Tap the top left green button
> iii) Enter text, and hit submit

Unable to reproduce on today's Nightly.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 810566 [details] [diff] [review]
Wallpaper

[Approval Request Comment]
User impact if declined: Crashes in certain cases.
Testing completed (on m-c, etc.): On m-c, green try run.
Risk to taking this patch (and alternatives if risky): Low. We haven't had reports of regressions from the originating bug and this patch will simply prevent us from crashing in new cases.

Comment 18

[Kyle Wooten]

Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3ca2e186baf
https://hg.mozilla.org/releases/mozilla-beta/rev/45d2971e3002