The default bug view has changed. See this FAQ.

crash IsElementVisible

VERIFIED FIXED in Firefox 11

Status

()

Core
Editor
P1
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

unspecified
mozilla12
x86
All
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11+ verified, firefox12+ verified)

Details

(Whiteboard: [qa!], crash signature)

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 584250 [details]
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

Updated

5 years ago
Component: General → Editor
Product: Firefox → Core
QA Contact: general → editor
It's a null deref, but I can't tell of what....

Comment 2

5 years ago
Automation hit this pretty hard. Windows, Mac and Linux / Aurora/11, Nightly/12
Blocks: 532972
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.

Comment 6

5 years ago
Created attachment 585174 [details]
testcase 2

Comment 7

5 years ago
Created attachment 585175 [details]
stacks for testcase 2

Updated

5 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
> 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]
Created attachment 589416 [details] [diff] [review]
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
Attachment #589416 - Flags: review?(tnikkel)
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+
Created attachment 589617 [details] [diff] [review]
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
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
Last Resolved: 5 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+
http://hg.mozilla.org/releases/mozilla-beta/rev/3ad684ba34d0
status-firefox11: affected → fixed
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.