446483 - Crash [@ nsIDocument::GetRootContent] with contenteditable and selection collapse on body

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build usually within 10 seconds.

This regressed between 2008-07-08 and 2008-07-12 (Ria, could you find out a more narrow regression range?)

http://crash-stats.mozilla.com/report/index/4f43ff21-5767-11dd-90e4-001a4bd43e5c?p=1
0  	xul.dll  	nsIDocument::GetRootContent  	 obj-firefox/dist/include/content/nsIDocument.h:448
1 	xul.dll 	SelectorMatches 	layout/style/nsCSSRuleProcessor.cpp:1355
2 	xul.dll 	ContentEnumFunc 	layout/style/nsCSSRuleProcessor.cpp:1817
3 	xul.dll 	RuleHash::EnumerateAllRules 	layout/style/nsCSSRuleProcessor.cpp:630
4 	xul.dll 	factory_MatchEntry

Comment 1

[Ria Klaassen (not reading all bugmail)]

It was not 100% reproducible for me. The only thing for sure is that it crashed on ID:2008071115. I couldn't make it crash on ID:2008071114 until now.
That would mean that on this page http://hg.mozilla.org/index.cgi/mozilla-central/shortlog/e30d33fe2ed8 the problem started between f174a0cc8c12 and (up) to ebfc2ad982f4 but I see nothing relevant in that range.

Comment 2

[Martijn Wargers (dead)]

Ok, thanks for trying.
I've narrowed down the regression range. I don't get a crash in a 2008-07-11 build, I do get a crash in a 2008-07-12 build:
http://hg.mozilla.org/index.cgi/mozilla-central/shortlog/2ac26a817782
http://hg.mozilla.org/index.cgi/mozilla-central/shortlog/e30d33fe2ed8
Regression from bug 431770, somehow?

Comment 3

[Mats Palmgren (inactive)]

Attachment: Crash stack

nsCOMPtr<nsINode>::get_DerivedSafe
nsIDocument::GetRootContent
SelectorMatches
ContentEnumFunc
RuleHash::EnumerateAllRules
nsCSSRuleProcessor::RulesMatching
EnumRulesMatching
nsStyleSet::FileRules
nsStyleSet::ResolveStyleFor
nsInspectorCSSUtils::GetStyleContextForContent
nsInspectorCSSUtils::GetStyleContextForContent
nsComputedDOMStyle::GetPropertyCSSValue
nsComputedDOMStyle::GetPropertyValue
IsBreakElement
mozInlineSpellWordUtil::BuildSoftText
mozInlineSpellWordUtil::EnsureWords
mozInlineSpellWordUtil::SetPosition
mozInlineSpellChecker::DoSpellCheck
mozInlineSpellChecker::ResumeCheck
mozInlineSpellResume::Run
nsThread::ProcessNextEvent
NS_ProcessNextEvent_P
nsBaseAppShell::Run
nsAppStartup::Run
XRE_main
main

Comment 4

[Mats Palmgren (inactive)]

Attachment: Stack for "WARNING: Context has no global"

There are a bunch of these warnings leading up to the crash:

WARNING: Context has no global.: dom/src/base/nsJSEnvironment.cpp, line 2231

Looking at the HTMLDocument at that point reveals that
mContentEditableCount == 0.

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Don't schedule a spell check after removing the last node
in the document.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 330770 [details] [diff] [review]
Patch rev. 1

Peter should review this...

Do we know why it regressed?

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 330770 [details] [diff] [review]
Patch rev. 1

This looks wrong, we want to rest the spellchecking state of the last node on which we turn off contentEditable. Does checking aElement->IsInDoc() help?

Comment 8

[Mats Palmgren (inactive)]

Firefox x86_64 debug builds on Linux:

hg clone changeset 35e2246a339f (bug 431770) => crash
  backing out 431770 => still crash

hg clone changeset b6f1af141782 => crash with/without bug 431770

hg clone changeset 6904a3773137 => no crash

hg clone f174a0cc8c12 => no crash
  apply bug 431770 => still no crash

hg pull -r ebfc2ad982f4 => no crash

hg pull -r dd2a7349ee20 => no crash

hg pull -r 6cf6bfb166b8 => no crash

hg pull -r 62e942b4fe34 => crash
  backout bug 422868 => still crash
  backout bug 422865 (changeset 751005894cf8) => no crash

==> bug 422865 is the culprit!

===============================

Current trunk does NOT crash, it appears to have been fixed in the range
2008-08-19-02 -- 2008-08-20-02.
http://hg.mozilla.org/mozilla-central/shortlog/5e68a48895e8
which is odd since there's only JS engine stuff in that range AFAICT.

There are several warnings and assertions in current trunk though,
which I think are related to the bug, so the underlying bug is still
there IMO.

Comment 9

[Mats Palmgren (inactive)]

(In reply to comment #7)
> Does checking aElement->IsInDoc() help?

No.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

The document has been removed from the docshell (mRemovedFromDocShell==true)
but it still has "mEditingState == eContentEditable" so the
EditingStateChanged() call at the beginning of ChangeContentEditableCount()
does not change the state due to the early return:
http://hg.mozilla.org/mozilla-central/annotate/72ebd32e6033/content/html/document/src/nsHTMLDocument.cpp#l3086
http://hg.mozilla.org/mozilla-central/annotate/72ebd32e6033/content/html/document/src/nsHTMLDocument.cpp#l3304

This patch fixes the crash in the old debug build and the associated
warnings/assertions disappears in that build and current trunk.

Comment 11

[Martijn Wargers (dead)]

This seems to be worksforme in current trunk build.

Comment 12

[Martijn Wargers (dead)]

I don't crash anymore with the testcase, but I still crash with this stacktrace in some complicated case where I haven't been able to make a testcase from.

Is the patch still relevant? Can it be reviewed and checked in?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think so.

Comment 14

[Martijn Wargers (dead)]

Ok, the patch still seems to fix the complicated case where I'm seeing this crash in my debug build.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Patch rev. 2.1

I was going to review with comments, but might as well post the patch with the change that I want (since it's so small). I'd rather set mEditingState to eOff asap, in RemovedFromDocshell, instead of waiting for a call to EditingStateChanged to update it.

Comment 16

[Mats Palmgren (inactive)]

Yeah, that's better, thanks.  I pushed it with a mochitest:
http://hg.mozilla.org/mozilla-central/rev/fef91437b4f0

-> FIXED