Closed Bug 1218072 Opened 4 years ago Closed 4 years ago
crash in ns
Text Editor State::Finished Restoring Selection
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.
4 years ago
Mark, would you be interested in taking a look into this?
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.
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
:-) 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.
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.
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.
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)
(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.
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.
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.
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+
You need to log in before you can comment on or make changes to this bug.