Created attachment 544365 [details]
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:
<optgroup contenteditable="true" style="display: inline;"></optgroup>
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:
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
Therefore I think suspicious changeset is this one:
Thanks for finding out, Arno!
Created attachment 544872 [details]
Traceback of the call to Revoke() from inside EnsureEditorInitialized()
Looking into solutions
Added reviewers to CC list from original patch
Should we be holding a scriptblocker across the whole thing here?
Created attachment 544880 [details] [diff] [review]
Patch tested against the testcase with no crash.
Note that FinishedInitializer() won't get run and so it won't do
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
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]
I think the correct solution here is to hold a script blocker.
I'll redo it with a scriptblocker
Created attachment 545192 [details] [diff] [review]
Patch using a scriptblocker
Tested against the testcase with no problems
This regression needs to be addressed on Aurora.
Comment on attachment 545192 [details] [diff] [review]
Patch using a scriptblocker
Looks good to me.
Can you please add the test case as a crashtest when landing? Thanks!
Please request approval for the patch with a risk analysis. Thanks.
Copying the crash from the old "Crash signature" field:
Replacing with a crash sig.
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?
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.
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....) ;-)
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
Checked in as http://hg.mozilla.org/mozilla-central/rev/2b9a669880df
Once it's green I'll look to checking in for Aurora
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.
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.
Ok. How does getting an a= for that work nowadays?
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.
Asserting is more or less normal for Mozilla builds, but you could file a new bug for that, I guess.
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.
(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.
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 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.
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)
Please land today if at all possible.
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
Beta: Check-build done and no problems on testcase.
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.