The default bug view has changed. See this FAQ.

Crash [@ nsTextControlFrame::FinishedInitializer] with textarea, font-face in iframe

VERIFIED FIXED in Firefox 6

Status

()

Core
Layout
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

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

Tracking

({crash, regression, testcase})

Trunk
mozilla8
x86
Windows 7
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(firefox6+ fixed, firefox7+ fixed, status1.9.2 unaffected)

Details

(Whiteboard: [sg:dos] [probably back out bug 655084 for Aurora and Beta], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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

Updated

6 years ago
Depends on: 655084
(Reporter)

Comment 2

6 years ago
Thanks for finding out, Arno!
Blocks: 655084
No longer depends on: 655084
(Assignee)

Comment 3

6 years ago
taking
Assignee: nobody → rjesup
(Assignee)

Comment 4

6 years ago
Created attachment 544872 [details]
Traceback of the call to Revoke() from inside EnsureEditorInitialized()

Looking into solutions
(Assignee)

Comment 5

6 years ago
Added reviewers to CC list from original patch
Should we be holding a scriptblocker across the whole thing here?
(Assignee)

Comment 7

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #544880 - Flags: review?(ehsan)
(Assignee)

Comment 8

6 years ago
I'm assume a scriptblocker would work as well; I'm open to what's the best method.
Comment on attachment 544880 [details] [diff] [review]
Proposed patch

I think the correct solution here is to hold a script blocker.
Attachment #544880 - Flags: review?(ehsan) → review-
(Assignee)

Comment 10

6 years ago
I'll redo it with a scriptblocker
(Assignee)

Comment 11

6 years ago
Created attachment 545192 [details] [diff] [review]
Patch using a scriptblocker

Tested against the testcase with no problems
Attachment #544880 - Attachment is obsolete: true
Attachment #545192 - Flags: review?(ehsan)
This regression needs to be addressed on Aurora.
tracking-firefox7: --- → ?
Comment on attachment 545192 [details] [diff] [review]
Patch using a scriptblocker

Looks good to me.
Attachment #545192 - Flags: review?(ehsan) → review+
Can you please add the test case as a crashtest when landing?  Thanks!

Comment 15

6 years ago
Please request approval for the patch with a risk analysis. Thanks.
tracking-firefox7: ? → +

Comment 16

6 years ago
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.

Updated

6 years ago
Crash Signature: 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.&hellip; → [@ nsTextControlFrame::FinishedInitializer() ]
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?
Group: core-security
status-firefox7: --- → affected
tracking-firefox6: --- → ?

Updated

6 years ago
tracking-firefox6: ? → +
Keywords: qawanted
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.
Group: core-security
tracking-firefox6: + → ?
Whiteboard: [sg:dos]
(Assignee)

Comment 19

6 years ago
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....) ;-)
(Assignee)

Comment 20

6 years ago
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
Attachment #545726 - Flags: review?(ehsan)

Updated

6 years ago
Attachment #545726 - Flags: review?(ehsan) → review+
(Assignee)

Comment 21

6 years ago
Checked in as http://hg.mozilla.org/mozilla-central/rev/2b9a669880df

Once it's green I'll look to checking in for Aurora
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #545192 - Attachment is obsolete: true
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.
(Assignee)

Comment 23

6 years ago
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.
Yeah, let's back out 655084 on Aurora.
(Assignee)

Comment 25

6 years ago
Ok.  How does getting an a= for that work nowadays?

Comment 26

6 years ago
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.
(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.
The crashtest for this bug continues to crash in mozilla-central.  Does this patch really fix the issue?
(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.
(Reporter)

Comment 30

6 years ago
Asserting is more or less normal for Mozilla builds, but you could file a new bug for that, I guess.

Updated

6 years ago
Whiteboard: [sg:dos] → [sg:dos] [probably back out bug 655084 for Aurora and Beta]
(Assignee)

Comment 31

6 years ago
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.
Attachment #546565 - Flags: review?(ehsan)

Updated

6 years ago
tracking-firefox6: ? → +
(Assignee)

Comment 32

6 years ago
(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.

Updated

6 years ago
Attachment #546565 - Flags: review?(ehsan)
Attachment #546565 - Flags: review+
Attachment #546565 - Flags: approval-mozilla-aurora?
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

6 years ago
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.
Attachment #546565 - Flags: approval-mozilla-beta+
Attachment #546565 - Flags: approval-mozilla-aurora?
Attachment #546565 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 35

6 years ago
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

6 years ago
Please land today if at all possible.
(Assignee)

Comment 37

6 years ago
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
(Assignee)

Comment 38

6 years ago
Beta: Check-build done and no problems on testcase.
http://hg.mozilla.org/releases/mozilla-beta/rev/79f80d48e0d3
(Assignee)

Updated

6 years ago
status-firefox6: --- → affected

Comment 39

6 years ago
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.
Status: RESOLVED → VERIFIED

Updated

6 years ago
status-firefox6: affected → fixed
status-firefox7: affected → fixed
(Reporter)

Updated

6 years ago
Keywords: qawanted
Target Milestone: --- → mozilla8
status1.9.2: --- → unaffected
You need to log in before you can comment on or make changes to this bug.