Last Comment Bug 713427 - crash IsElementVisible
: crash IsElementVisible
Status: VERIFIED FIXED
[qa!]
: crash, regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 All
: P1 critical (vote)
: mozilla12
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 532972 635618
  Show dependency treegraph
 
Reported: 2011-12-25 03:23 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2012-03-21 03:37 PDT (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified


Attachments
testcase (159 bytes, text/html)
2011-12-25 03:23 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase 2 (643 bytes, application/xhtml+xml)
2011-12-31 17:39 PST, Jesse Ruderman
no flags Details
stacks for testcase 2 (6.83 KB, text/plain)
2011-12-31 17:40 PST, Jesse Ruderman
no flags Details
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing. (4.28 KB, patch)
2012-01-17 23:42 PST, Boris Zbarsky [:bz]
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)
2012-01-18 13:16 PST, Boris Zbarsky [:bz]
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-12-25 03:23:22 PST
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
Comment 1 Boris Zbarsky [:bz] 2011-12-26 13:15:53 PST
It's a null deref, but I can't tell of what....
Comment 2 Bob Clary [:bc:] 2011-12-27 07:50:29 PST
Automation hit this pretty hard. Windows, Mac and Linux / Aurora/11, Nightly/12
Comment 3 Boris Zbarsky [:bz] 2011-12-27 10:01:34 PST
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.
Comment 4 Timothy Nikkel (:tnikkel) 2011-12-29 01:10:14 PST
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.
Comment 5 Timothy Nikkel (:tnikkel) 2011-12-29 22:37:51 PST
Or maybe we should just deal with it? Clearing lazy bits in an entire subtree could be time consuming.
Comment 6 Jesse Ruderman 2011-12-31 17:39:48 PST
Created attachment 585174 [details]
testcase 2
Comment 7 Jesse Ruderman 2011-12-31 17:40:07 PST
Created attachment 585175 [details]
stacks for testcase 2
Comment 8 Boris Zbarsky [:bz] 2012-01-17 23:39:35 PST
> 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.
Comment 9 Boris Zbarsky [:bz] 2012-01-17 23:42:19 PST
Created attachment 589416 [details] [diff] [review]
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
Comment 10 Timothy Nikkel (:tnikkel) 2012-01-18 01:18:43 PST
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
Comment 11 Boris Zbarsky [:bz] 2012-01-18 08:22:36 PST
> 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 12 Timothy Nikkel (:tnikkel) 2012-01-18 12:45:23 PST
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.
Comment 13 Boris Zbarsky [:bz] 2012-01-18 13:16:22 PST
Created attachment 589617 [details] [diff] [review]
Don't assume things about lazy frame construction bits that just aren't true when doing IsVisible() testing.
Comment 15 Boris Zbarsky [:bz] 2012-01-18 13:33:17 PST
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.
Comment 16 Marco Bonardo [::mak] 2012-01-19 02:49:59 PST
https://hg.mozilla.org/mozilla-central/rev/efb577526489
Comment 17 Alex Keybl [:akeybl] 2012-01-20 13:49:46 PST
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.
Comment 18 Alex Keybl [:akeybl] 2012-02-13 18:07:30 PST
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.
Comment 20 Paul Silaghi, QA [:pauly] 2012-02-24 05:48:32 PST
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.
Comment 21 Paul Silaghi, QA [:pauly] 2012-03-21 03:37:28 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.