Closed Bug 713427 Opened 13 years ago Closed 13 years ago

crash IsElementVisible

Categories

(Core :: DOM: Editor, defect, P1)

Product:
Core
Component:
DOM: Editor
Platform:
x86
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla12
Tracking Flags:
Tracking Status
firefox11 + verified
firefox12 + verified

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [qa!])

Crash Data

Attachments

(5 files)

testcase
159 bytes, text/html
Details
testcase 2
643 bytes, application/xhtml+xml
Details
stacks for testcase 2
6.83 KB, text/plain
Details
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
4.28 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
4.54 KB, patch
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file testcase —
This bug was filed from the Socorro interface and is report bp-1be87cc3-23f4-47c2-8203-062542111224 . ============================================================= 0 xul.dll IsElementVisible 1 xul.dll nsEditor::IsEditable editor/libeditor/base/nsEditor.cpp:3659 2 xul.dll nsEditor::IsEditable editor/libeditor/base/nsEditor.cpp:3647 3 xul.dll nsEmptyEditableFunctor::operator editor/libeditor/html/nsHTMLEditRules.cpp:144 4 xul.dll nsDOMIterator::AppendList editor/libeditor/base/nsEditorUtils.cpp:136 5 xul.dll nsHTMLEditRules::AdjustSpecialBreaks editor/libeditor/html/nsHTMLEditRules.cpp:7583 6 xul.dll nsHTMLEditRules::AfterEditInner editor/libeditor/html/nsHTMLEditRules.cpp:455 7 xul.dll nsHTMLEditRules::AfterEdit editor/libeditor/html/nsHTMLEditRules.cpp:387 8 xul.dll nsHTMLEditor::EndOperation editor/libeditor/html/nsHTMLEditor.cpp:3936 9 xul.dll nsEditor::DeleteNode 10 xul.dll nsHTMLEditor::DeleteNode editor/libeditor/html/nsHTMLEditor.cpp:3635 11 xul.dll nsHTMLEditRules::DocumentModifiedWorker editor/libeditor/html/nsHTMLEditRules.cpp:9290 12 xul.dll nsRunnableMethodImpl<void obj-firefox/dist/include/nsThreadUtils.h:345 13 xul.dll nsContentUtils::RemoveScriptBlocker content/base/src/nsContentUtils.cpp:4413 14 xul.dll nsRefPtr<nsIDOMEventListener>::~nsRefPtr<nsIDOMEventListener> obj-firefox/dist/include/nsAutoPtr.h:907 15 xul.dll nsDocument::EndUpdate content/base/src/nsDocument.cpp:3989 16 xul.dll nsHTMLDocument::EndUpdate content/html/document/src/nsHTMLDocument.cpp:2544 17 xul.dll nsHtml5TreeOpExecutor::EndDocUpdate parser/html/nsHtml5TreeOpExecutor.h:290 18 xul.dll nsHtml5TreeOpExecutor::DidBuildModel parser/html/nsHtml5TreeOpExecutor.cpp:128 19 xul.dll nsHtml5TreeOperation::Perform parser/html/nsHtml5TreeOperation.cpp:662 20 xul.dll NS_IsMainThread_P obj-firefox/xpcom/build/nsThreadUtils.cpp:138 21 xul.dll AbortIfOffMainThreadIfCheckFast xpcom/base/nsCycleCollector.cpp:1287 22 xul.dll nsRange::QueryInterface content/base/src/nsRange.cpp:317
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
It's a null deref, but I can't tell of what....
Automation hit this pretty hard. Windows, Mac and Linux / Aurora/11, Nightly/12
status-firefox11: --- → affected
status-firefox12: --- → affected
OS: Windows NT → All
Oh, my apologies. I missed the testcase in comment 0! I can totally reproduce the bug with that; the issue is that on this line in IsElementVisible: if (parent->GetPrimaryFrame()->IsLeaf()) { parent->GetPrimaryFrame() is null. Note that this fails the assertion right before that line! And the reason it fails that assert is that I don't see how this situation can arise. In this case, cur is the <blockquote>, parent is the <span>. The parent of the <span> is the <body>, and that has a frame. The <span> doesn't have the NODE_NEEDS_FRAME flag. The <blockquote> does. Timothy, any idea how that can happen? If it's supposed to happen, sometimes, we need to change the code flow here; otherwise we should fix whatever is causing this situation to happen.
Blocks: 635618
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
Keywords: regression
The span gets a frame. We insert the blockquote and mark it for frame construction later. Then we get a ContentInserted notification for the code element with display: table-row. WipeContainingBlock recognizes that it doesn't have the right parent type so it RecreateFramesForContent's the parent (the span). We ContentRemoved the span, killing its frame, and then PostRestyleEvent with reconstruct frame hint to do the recreate frame part async. And then the editor starts doing things before we finish that up and we crash like you've outlined. I guess we need to do some lazy bit clearing when removing nodes... I'll think about this some more.
Or maybe we should just deal with it? Clearing lazy bits in an entire subtree could be time consuming.
Attached file testcase 2 —
Attached file stacks for testcase 2 —
> Or maybe we should just deal with it? Yes, I think we should. Not only that, but I think the existing code is actually wrong for the case when aElement has no frame but does have the NODE_NEEDS_FRAME bit and its parent has a frame. Patch coming up.
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 589416 [details] [diff] [review] Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. >+ // Walk up the tree looking for the nearest ancestor with a frame. >+ // The state of the child right below it will determined whether determine > cur = cur->GetFlattenedTreeParent(); > if (!cur) { > // None of our ancestors have lazy bits set, so we shouldn't have a frame > return false; > } I guess this relies on the root element never having the NEEDS_FRAME bit set? Would be nice if we didn't have to rely on that though.. Otherwise looks good
> I guess this relies on the root element never having the NEEDS_FRAME bit set? Yes, it does. I can remove that reliance by breaking if haveLazyBitOnChild, right? Want me to?
Comment on attachment 589416 [details] [diff] [review] Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. (In reply to Boris Zbarsky (:bz) from comment #11) > > I guess this relies on the root element never having the NEEDS_FRAME bit set? > > Yes, it does. I can remove that reliance by breaking if haveLazyBitOnChild, > right? Want me to? I think so, it means we don't have to change this code if that invariant ever changes. r+ so you don't need the upload the new patch.
Attachment #589416 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb577526489
status-firefox12: affected → fixed
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla12
Comment on attachment 589617 [details] [diff] [review] Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. [Approval Request Comment] Regression caused by (bug #): bug 635618 User impact if declined: Crashes on various HTML editing operations Testing completed (on m-c, etc.): Passes try, landed on inbound now Risk to taking this patch (and alternatives if risky): Risk should be low. The other option is to back out part or all of 713427. Backing out part is more risky. Backing out all may also be somewhat risky, and regresses the performance fix.
Attachment #589617 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/efb577526489
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 589617 [details] [diff] [review] Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. [Triage Comment] Deemed to be low risk and affects automation. Approved for Aurora.
Attachment #589617 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 589617 [details] [diff] [review] Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. [Triage Comment] Missed Aurora 11, but we still feel comfortable with taking that patch for Beta 11 based upon the possible user effect. Approved for Beta.
Attachment #589617 - Flags: approval-mozilla-beta+
Whiteboard: [qa+]
No crashes on: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0 Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0 Verified fixed on Firefox 11b4.
status-firefox11: fixed → verified
Whiteboard: [qa+] → [qa+][qa!:11]
No crashes during test cases loading: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0 Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0 Verified fixed on Firefox 12b1.
Status: RESOLVED → VERIFIED
status-firefox12: fixed → verified
Whiteboard: [qa+][qa!:11] → [qa!]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: