Closed Bug 1218072 Opened 4 years ago Closed 4 years ago

crash in nsTextEditorState::FinishedRestoringSelection

Categories

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

43 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- affected
firefox42 --- affected
firefox43 --- affected
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed
fennec + ---

People

(Reporter: kats, Assigned: capella)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-2e7666bc-71d0-4ddd-b7e7-829d12151024.
=============================================================

Running recent Aurora on a Nexus 4. Crash appears to be reproducible (3/3 times so far). sTR: load homestars.com and put focus in the "Search for" text field. Crash happens right away.

Additional reports:
https://crash-stats.mozilla.com/report/index/bp-d10de60f-3bf0-4252-9904-b345c2151024
https://crash-stats.mozilla.com/report/index/bp-2c51555b-ffc9-4322-a3d8-996bd2151024
Affects all channels including current release and current nightly.
tracking-fennec: --- → ?
Mark, would you be interested in taking a look into this?
Flags: needinfo?(markcapella)
This is offset from null crash, as far as I see.
nsAutoScriptBlocker scriptBlocker; protects mTextEditorState to become null in the block it is defined, but mTextEditorState->FinishedRestoringSelection() is outside it.
And then the offset if null + (nsTextEditorState::mRestoringSelection)
So, a null check before mTextEditorState->FinishedRestoringSelection() should be fine.
Blocks: 655084
Thanks smaug!

It looks like in between these two blocks:
   http://mxr.mozilla.org/mozilla-central/source/dom/html/nsTextEditorState.cpp?rev=41dea9df27ed&mark=112-113#91

nsTextControlFrame::DestroyFrom() triggers ->
   HTMLInputElement::UnbindFromFrame() triggers ->
      nsTextEditorState::UnbindFromFrame() triggers ->
         RestoreSelectionState::Revoke() which -> NULLS both the mFrame and mTextEditorState references.

( Also, nsTextEditorState::PrepareEditor() then quickly creates a new RestoreSelectionState(), which Run()'s through normally, triggering->FinishedRestoringSelection(), and is destroyed. )

So, null checking mTextEditorState at that point will work, but so would null checking mFrame. Would that be more "correct"?

Finally, simply NULL checking there may not be enough. Subsequent text entry in that field is /mucking/ up as seen here:
   https://www.dropbox.com/s/cye9y4k9tcc9k79/bug1218072.mp4?dl=0

Looking further, and cc:ing jchen
Flags: needinfo?(markcapella)
:-) Forgot to actually cc: jchen in previous comment ....
Assignee: nobody → nchen
tracking-fennec: ? → +
Mark, did you intend to work on this? Feel free to take this bug.
Flags: needinfo?(markcapella)
I can look. bug 1215959 is winding down, and I picked up a few more things there :)

Last I knew, we can avoid this crash but wind up broken anyhow with the null-check, so need to "re-purpose" this bug.
Flags: needinfo?(markcapella)
I cannot reproduce this any longer, using the same test device as previously. Further, using nightly builds from 10/24 (original bug report), 10/27, 10/28, 10/29, and 11/05 fail to demonstrate the issue.

Can someone else can still see this as a thing?
Well, the code is still buggy. A null check wouldn't be too hard to add. Want to write a patch, I'd promise a fast review ;)
And null check would be, as far as I see, enough for this particular bug.
Null checking mTextEditorState would be more correct than null checking mFrame.
Attached patch bug1218072.diffSplinter Review
Good answers! Good to fix, though I hate to think something site-specific was responsible :/
Assignee: nchen → markcapella
Status: NEW → ASSIGNED
Attachment #8693220 - Flags: review?(bugs)
Attachment #8693220 - Flags: review?(bugs) → review+
(In reply to Mark Capella [:capella] from comment #9)
> I cannot reproduce this any longer, using the same test device as
> previously. Further, using nightly builds from 10/24 (original bug report),
> 10/27, 10/28, 10/29, and 11/05 fail to demonstrate the issue.
> 
> Can someone else can still see this as a thing?

I can't repro this on latest aurora any more either, so probably something site-specific changed.
https://hg.mozilla.org/mozilla-central/rev/959df62d450d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Can we uplift this patch to Beta 44? A friend of mine had to stop using Firefox for Android because this crash happens every time he accesses his university's web mail interface.
Flags: needinfo?(markcapella)
Comment on attachment 8693220 [details] [diff] [review]
bug1218072.diff

Approval Request Comment: See comment #15 use justification
[Feature/regressing bug #]: not determined
[User impact if declined]: Various issues/crashes due to basically flawed code.
[Describe test coverage new/current, TreeHerder]: Success in m-c for ~3 weeks.
[Risks and why]: Extremely low-to-none.
[String/UUID change made/needed]: None.
Flags: needinfo?(markcapella)
Attachment #8693220 - Flags: approval-mozilla-beta?
Comment on attachment 8693220 [details] [diff] [review]
bug1218072.diff

Crash fix that has stabilized in Nightly for 3 weeks, Beta44+
Attachment #8693220 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.