Last Comment Bug 669767 - Crash [@ nsTextControlFrame::FinishedInitializer] with textarea, font-face in iframe
: Crash [@ nsTextControlFrame::FinishedInitializer] with textarea, font-face in...
Status: VERIFIED FIXED
[sg:dos] [probably back out bug 65508...
: crash, regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows 7
: -- critical (vote)
: mozilla8
Assigned To: Randell Jesup [:jesup]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: 655084
  Show dependency treegraph
 
Reported: 2011-07-06 15:53 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-09-18 08:06 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
unaffected


Attachments
testcase (638 bytes, text/html)
2011-07-06 15:53 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Traceback of the call to Revoke() from inside EnsureEditorInitialized() (5.40 KB, text/plain)
2011-07-08 11:44 PDT, Randell Jesup [:jesup]
no flags Details
Proposed patch (924 bytes, patch)
2011-07-08 12:17 PDT, Randell Jesup [:jesup]
ehsan: review-
Details | Diff | Splinter Review
Patch using a scriptblocker (1.20 KB, patch)
2011-07-11 09:46 PDT, Randell Jesup [:jesup]
ehsan: review+
Details | Diff | Splinter Review
Same scriptblocker patch with crashtest added (2.36 KB, patch)
2011-07-13 12:50 PDT, Randell Jesup [:jesup]
ehsan: review+
Details | Diff | Splinter Review
Backout patch for both changesets (17.57 KB, patch)
2011-07-18 10:34 PDT, Randell Jesup [:jesup]
ehsan: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-06 15:53:10 PDT
Created attachment 544365 [details]
testcase

See testcase, which crashes current trunk build on load.
It doesn't crash in Firefox 4, so looks like a regression.

The iframe content consists of this:
<html><head></head><body>
<textarea></textarea>


<style>
@font-face {
      font-family: "cutabovetherest";
      src: url("http://www.webpagepublicity.com/free-fonts/a/A%20Cut%20Above%20The%20Rest.ttf");
}    

</style>

<optgroup contenteditable="true" style="display: inline;"></optgroup>

</body></html>

https://crash-stats.mozilla.com/report/index/bp-68d84473-ccb3-448a-adaf-a74e92110706
0 	xul.dll 	nsTextControlFrame::FinishedInitializer 	layout/forms/nsTextControlFrame.h:401
1 	xul.dll 	nsTextControlFrame::EditorInitializer::Run 	layout/forms/nsTextControlFrame.h:304
2 	xul.dll 	nsContentUtils::RemoveScriptBlocker 	content/base/src/nsContentUtils.cpp:4401
3 	xul.dll 	PresShell::InitialReflow 	layout/base/nsPresShell.cpp:2738
4 	xul.dll 	PresShell::InitialReflow 	layout/base/nsPresShell.cpp:2746
5 	xul.dll 	DocumentViewerImpl::InitPresentationStuff 	
6 	xul.dll 	DocumentViewerImpl::Show 	
7 	xul.dll 	nsFrameLoader::Show 	content/base/src/nsFrameLoader.cpp:809
8 	xul.dll 	nsDocShell::SetVisibility 	docshell/base/nsDocShell.cpp:4929
9 	xul.dll 	NS_TableDrivenQI 	obj-firefox/xpcom/build/nsISupportsImpl.cpp:49
10 	xul.dll 	nsGenericHTMLFrameElement::QueryInterface 	content/html/content/src/nsGenericHTMLElement.cpp:3029
11 	xul.dll 	NS_IsMainThread_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:138
12 	xul.dll 	NS_IsMainThread_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:138
13 	xul.dll 	nsSubDocumentFrame::ShowViewer 	layout/generic/nsSubDocumentFrame.cpp:231
14 	xul.dll 	AsyncFrameInit::Run 	layout/generic/nsSubDocumentFrame.cpp:148
15 	xul.dll 	nsContentUtils::RemoveScriptBlocker 	content/base/src/nsContentUtils.cpp:4401
16 	xul.dll 	nsPresContext::GetRootPresContext 	layout/base/nsPresContext.cpp:1167
17 	xul.dll 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4765
18 	xul.dll 	nsDocument::FlushPendingNotifications 	content/base/src/nsDocument.cpp:6313
19 	xul.dll 	nsDocument::FlushPendingNotifications 	content/base/src/nsDocument.cpp:6311
20 	xul.dll 	nsHTMLDocument::EditingStateChanged 	content/html/document/src/nsHTMLDocument.cpp:2811
21 	xul.dll 	NS_IsMainThread_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:138
22 	xul.dll 	AbortIfOffMainThreadIfCheckFast 	xpcom/base/nsCycleCollector.cpp:1221
23 	xul.dll 	nsCycleCollectingAutoRefCnt::decr 	obj-firefox/dist/include/nsISupportsImpl.h:211
24 	mozcrt19.dll 	arena_dalloc_small 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4054
25 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:
Comment 1 arno renevier 2011-07-08 07:47:55 PDT
This happens because text frame is destroyed *during* EditorInitializer Run method execution.
Then, mFrame is set at start of method, but is null at the moment of mFrame->FinishedInitializer(); then the crash.

regression range is
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21c304c5f351&tochange=52f72d71dc59
Therefore I think suspicious changeset is this one:
http://hg.mozilla.org/mozilla-central/rev/52f72d71dc59
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-08 08:00:19 PDT
Thanks for finding out, Arno!
Comment 3 Randell Jesup [:jesup] 2011-07-08 08:16:12 PDT
taking
Comment 4 Randell Jesup [:jesup] 2011-07-08 11:44:58 PDT
Created attachment 544872 [details]
Traceback of the call to Revoke() from inside EnsureEditorInitialized()

Looking into solutions
Comment 5 Randell Jesup [:jesup] 2011-07-08 11:45:55 PDT
Added reviewers to CC list from original patch
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-07-08 12:12:26 PDT
Should we be holding a scriptblocker across the whole thing here?
Comment 7 Randell Jesup [:jesup] 2011-07-08 12:17:33 PDT
Created attachment 544880 [details] [diff] [review]
Proposed patch

Patch tested against the testcase with no crash.

Note that FinishedInitializer() won't get run and so it won't do
    Properties().Delete(TextControlInitializer());
which I believe shouldn't be a problem since (of course) the frame has been/is being deleted.

I don't think this problem applies to the ::Run() call in nsTextEditorState.cpp
Comment 8 Randell Jesup [:jesup] 2011-07-08 12:24:00 PDT
I'm assume a scriptblocker would work as well; I'm open to what's the best method.
Comment 9 :Ehsan Akhgari 2011-07-11 08:29:20 PDT
Comment on attachment 544880 [details] [diff] [review]
Proposed patch

I think the correct solution here is to hold a script blocker.
Comment 10 Randell Jesup [:jesup] 2011-07-11 08:46:22 PDT
I'll redo it with a scriptblocker
Comment 11 Randell Jesup [:jesup] 2011-07-11 09:46:03 PDT
Created attachment 545192 [details] [diff] [review]
Patch using a scriptblocker

Tested against the testcase with no problems
Comment 12 :Ehsan Akhgari 2011-07-11 10:44:07 PDT
This regression needs to be addressed on Aurora.
Comment 13 :Ehsan Akhgari 2011-07-11 10:45:00 PDT
Comment on attachment 545192 [details] [diff] [review]
Patch using a scriptblocker

Looks good to me.
Comment 14 :Ehsan Akhgari 2011-07-11 12:11:58 PDT
Can you please add the test case as a crashtest when landing?  Thanks!
Comment 15 Asa Dotzler [:asa] 2011-07-12 14:51:05 PDT
Please request approval for the patch with a risk analysis. Thanks.
Comment 16 [:Cww] 2011-07-12 14:51:27 PDT
Copying the crash from the old "Crash signature" field:

https://crash-stats.mozilla.com/report/index/bp-68d84473-ccb3-448a-adaf-a74e92110706

Replacing with a crash sig.
Comment 17 Daniel Veditz [:dveditz] 2011-07-12 14:58:04 PDT
If this is a regression from bug 655084 then Firefox 6 is probably affected as well.

Although this testcase results in a crash on null, missing scriptblockers are generally considered an exploitable problem. Are we sure this one can only cause a "safe" null dereference?
Comment 18 Daniel Veditz [:dveditz] 2011-07-13 12:09:12 PDT
Olli says I'm overly paranoid, Revoke should mark mFrame null so this is a safe crash. Firefox 6 may be affected and we may want to approve the patch but we don't need to track it.
Comment 19 Randell Jesup [:jesup] 2011-07-13 12:28:22 PDT
Agreed - I had written an analysis saying the same thing but hadn't hit "Save"  on it, and when I refreshed before saving I see I was beaten to it.  Attaching a patch including the requested crashtest; no code change so I would assume the r= carries over (if policy hasn't changed in the last 9 or so years....) ;-)
Comment 20 Randell Jesup [:jesup] 2011-07-13 12:50:05 PDT
Created attachment 545726 [details] [diff] [review]
Same scriptblocker patch with crashtest added

Same patch with crashtest.  r=? to ehsan but I'll assume it's r+ given his comment
Comment 21 Randell Jesup [:jesup] 2011-07-13 14:02:50 PDT
Checked in as http://hg.mozilla.org/mozilla-central/rev/2b9a669880df

Once it's green I'll look to checking in for Aurora
Comment 22 :Ehsan Akhgari 2011-07-14 13:36:22 PDT
Can we address the Aurora regression by backing out bug 655084 instead of taking the fix to this bug?  I would be a lot more comfortable with that.
Comment 23 Randell Jesup [:jesup] 2011-07-14 14:21:26 PDT
That's an option, yes.  This O(n^2) issue doesn't hurt us *that* badly except in extreme cases.   These may be slightly more common in "enterprise" apps, but it's not horrible on a modern CPU at any normal number of form controls.  Measurable, yes, noticable, maybe on bigger ones.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-14 18:27:48 PDT
Yeah, let's back out 655084 on Aurora.
Comment 25 Randell Jesup [:jesup] 2011-07-14 18:49:51 PDT
Ok.  How does getting an a= for that work nowadays?
Comment 26 Asa Dotzler [:asa] 2011-07-14 19:49:05 PDT
Randell, there are approval flags on the patch next to the review flags. Use the one(s) appropriate for the channels you want the changes in. Please include a risk analysis with your requests.
Comment 27 :Ehsan Akhgari 2011-07-15 10:54:33 PDT
(In reply to comment #25)
> Ok.  How does getting an a= for that work nowadays?

You should create the backout patch, attach it here and ask for approval.
Comment 28 Bill Gianopoulos [:WG9s] 2011-07-16 15:43:31 PDT
The crashtest for this bug continues to crash in mozilla-central.  Does this patch really fix the issue?
Comment 29 Bill Gianopoulos [:WG9s] 2011-07-16 15:45:47 PDT
(In reply to comment #28)
> The crashtest for this bug continues to crash in mozilla-central.  Does this
> patch really fix the issue?

Actually I guess it is just asserting, and not crashing, but still would seem to indicate that perhaps this is not completely fixed.
Comment 30 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-16 23:28:15 PDT
Asserting is more or less normal for Mozilla builds, but you could file a new bug for that, I guess.
Comment 31 Randell Jesup [:jesup] 2011-07-18 10:34:53 PDT
Created attachment 546565 [details] [diff] [review]
Backout patch for both changesets

Backout patch for Aurora.  Note that the backout is complicated by a patch landing on a few of the same routines for key event handling, though the solution is simple.

Risk: O(n^2) behavior for text forms will return; "normal" pages won't be hurt much even with a fair number of controls, and it restores behavior many years old.  Other risk: if the backout merge was done incorrectly.  I compared the original patch to the backout patch to make sure that outside of the merge spots that patch was the inverse of the original patch.

Currently doing verification builds but thought I'd get this up for review.
Comment 32 Randell Jesup [:jesup] 2011-07-18 18:48:52 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > The crashtest for this bug continues to crash in mozilla-central.  Does this
> > patch really fix the issue?
> 
> Actually I guess it is just asserting, and not crashing, but still would
> seem to indicate that perhaps this is not completely fixed.

Is this the assertion you refer to:

-1219713344[b724a110]: ###!!! ASSERTION: Why is the root in mDirtyRoots already?: '!mDirtyRoots.Contains(rootFrame)', file ../../../layout/base/nsPresShell.cpp, line 2755
###!!! ASSERTION: Why is the root in mDirtyRoots already?: '!mDirtyRoots.Contains(rootFrame)', file ../../../layout/base/nsPresShell.cpp, line 2755

That I can't say, but I'd assume that something is expecting the script to run before ::Run() returns, or some such.  This is in ::InitialReflow(). It may or may not be relevant, but this is the comment right before the assert (it may apply to the code following the assert):

  // Note: Because the frame just got created, it has the NS_FRAME_IS_DIRTY
  // bit set.  Unset it so that FrameNeedsReflow() will work right.
Comment 33 :Ehsan Akhgari 2011-07-19 08:32:14 PDT
I think we should just file a new bug for the assertions (if one does not exist already).  They're not relevant to the issue at hand in this bug.
Comment 34 Asa Dotzler [:asa] 2011-07-19 14:55:34 PDT
Comment on attachment 546565 [details] [diff] [review]
Backout patch for both changesets

I think we need this for both aurora and beta. if it's not needed on beta, then please say so.
Comment 35 Randell Jesup [:jesup] 2011-07-19 15:03:14 PDT
Definitely on both; I was waiting until the Aurora patch was ok'd since I didn't have a beta tree set up  yet (will have it tonight, and see if the patch applies cleanly to beta)
Comment 36 christian 2011-07-20 13:28:06 PDT
Please land today if at all possible.
Comment 37 Randell Jesup [:jesup] 2011-07-20 14:39:41 PDT
Aurora:  
http://hg.mozilla.org/releases/mozilla-aurora/rev/420c2b3c4aee and http://hg.mozilla.org/releases/mozilla-aurora/rev/d78bbed90c65

Beta coming as soon as I pull/build/verify
Comment 38 Randell Jesup [:jesup] 2011-07-20 16:23:19 PDT
Beta: Check-build done and no problems on testcase.
http://hg.mozilla.org/releases/mozilla-beta/rev/79f80d48e0d3
Comment 39 George Carstoiu 2011-07-22 09:35:25 PDT
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0 - beta 3

Verified issue on  Ubuntu 11.04, WinXP, Mac OS X 10.7 and Win7 using the attached testcase - Fx does not crash on any of the three channels.

Setting status to Verified Fixed.

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