Roc suggested this in bug 221820 comment 76. The idea here is to move the editor and selection controller objects for the following HTML elements to the content node. <input type=text> <input type=password> <textarea> These objects are currently owned by the respective frame elements. Patch coming up.

I have reached a problem I don't know how to solve. nsIEditor::Init requires a root content node to be passed to it, and inside nsTextControlFrame, we pass mAnonymousDiv to it. But if we take out mEditor from nsTextControlFrame, and move it to the new class (which I've called nsTextEditorState), then when the frame is "binding" to the text editor state object, we need to get the anonymous content under the frame in order to be able to initialize the editor, and if for some reason the frame gets reconstructed, then the content root that the editor has would be different to the content root that the new frame has, and it doesn't seem like nsEditor supports swapping the root content. I'm not quite sure how to solve this...

This is my current work in progress patch. This is not useful, and it doesn't even compile. I'm just attaching it to show how I'm implementing things.

Either we have to add the ability to redirect the editor to the new content, or we create a new editor for the new content and copy over whatever state is relevant from the old editor.

I'm thinking about making nsTextEditorState own the anonymous div as well, and cache the editor when the frame is destroyed, and when a new frame is created, give the old editor to the new frame. This approach leads to a bit more complex patch, but I think it's easier than both solutions proposed in comment 3. The main thing that this involves (AFAIK for now) is removing the notion of the current value being owned by frame or content. The value will always be owned by the nsTextEditorState object (which makes more sense anyway), and it will use the editor to change it if needed even if there's currently no frame associated with the content node. We'll see how this approach works when I dig deeper in the patch.

(In reply to comment #4) > The value will always be owned by the nsTextEditorState object (which > makes more sense anyway), and it will use the editor to change it if needed > even if there's currently no frame associated with the content node. That sounds really good, I've always hated the way ownership of the value shifts between content and frame!

(In reply to comment #5) > (In reply to comment #4) > > The value will always be owned by the nsTextEditorState object (which > > makes more sense anyway), and it will use the editor to change it if needed > > even if there's currently no frame associated with the content node. > > That sounds really good, I've always hated the way ownership of the value > shifts between content and frame! Yes, many parts of the interaction of nsTextControlFrame and nsHTML{Input,TextArea}Element seem strange, and I attempt to fix them all in this patch. Hoping to get this finished by the end of this week.

This patch is also incomplete, and doesn't compile yet. Attaching just as a progress update on my work here. I think ~80% of the work is done though.

This is a completely untested (as of yet) version of the patch. It finally compiles, though!

This version fixes many bugs in the previous version. I am still in the process of testing this manually and fixing the obvious bugs. After that I'm going to post it to the try server for the first time and move on from there.

As promised in bug 221820 comment 129, this patch backs out part 7 of that bug, which would no longer be necessary when this bug gets fixed.

I've updated the patch to incorporate the recent churn on trunk. I've also fixed a bunch of logic in the patch to make it more efficient. This is still a preliminary patch, which at least doesn't crash during a normal browsing session, but it still has a lot of problems. I'm in the process of debugging it.

I fixed a few bugs and crashes here, still a lot more to investigate. This work will probably be interrupted for a couple of days while I work on startup instrumentation at full capacity.

Lots of new fixes. The basic behavior of the text box controls for normal browsing is correct now, and I haven't seen a crash while browsing. Still working on getting all the tests to pass.

I've been trying to debug a crash with my patch. My investigation shows that it is related to cross-domain security checks, so I thought I'd ask here for help. The crash happens in this test: http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_bug287446.html?force=1 The crash happens as part of this code: http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_bug287446.html?force=1#53 What happens here is that nsTextControlFrame::CreateAnonymousContent is called, which calls nsTextEditorState::BindToFrame, which fails because of this security check: #0 0x1318e8bd in nsContentUtils::CanCallerAccess at nsContentUtils.cpp:1161 #1 0x132408f2 in nsRange::SetEnd at nsRange.cpp:688 #2 0x135b12f5 in nsTextEditRules::Init at nsTextEditRules.cpp:189 #3 0x135a9358 in nsPlaintextEditor::InitRules at nsPlaintextEditor.cpp:331 #4 0x135a4bc7 in nsPlaintextEditor::EndEditorInit at nsPlaintextEditor.cpp:225 #5 0x135ac099 in nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger at nsTextEditUtils.cpp:134 #6 0x135aa138 in nsPlaintextEditor::Init at nsPlaintextEditor.cpp:152 #7 0x132fe899 in nsTextEditorState::PrepareEditor at nsTextEditorState.cpp:1001 #8 0x132ff989 in nsTextEditorState::BindToFrame at nsTextEditorState.cpp:934 #9 0x13328c3c in nsHTMLInputElement::BindToTextControl at nsHTMLInputElement.cpp:1093 #10 0x12fff2f4 in nsTextControlFrame::CreateAnonymousContent at nsTextControlFrame.cpp:437 #11 0x12e68c70 in nsCSSFrameConstructor::GetAnonymousContent at nsCSSFrameConstructor.cpp:3933 #12 0x12e737d1 in nsCSSFrameConstructor::ProcessChildren at nsCSSFrameConstructor.cpp:9565 #13 0x12e77270 in nsCSSFrameConstructor::ConstructFrameFromItemInternal at nsCSSFrameConstructor.cpp:3818 #14 0x12e7776e in nsCSSFrameConstructor::ConstructFramesFromItem at nsCSSFrameConstructor.cpp:5460 #15 0x12e88ba2 in nsCSSFrameConstructor::ConstructFramesFromItemList at nsCSSFrameConstructor.cpp:9505 #16 0x12e73b26 in nsCSSFrameConstructor::ProcessChildren at nsCSSFrameConstructor.cpp:9615 #17 0x12e74839 in nsCSSFrameConstructor::ConstructBlock at nsCSSFrameConstructor.cpp:10665 #18 0x12e74c35 in nsCSSFrameConstructor::ConstructNonScrollableBlock at nsCSSFrameConstructor.cpp:4517 #19 0x12e76db4 in nsCSSFrameConstructor::ConstructFrameFromItemInternal at nsCSSFrameConstructor.cpp:3728 #20 0x12e7776e in nsCSSFrameConstructor::ConstructFramesFromItem at nsCSSFrameConstructor.cpp:5460 #21 0x12e88ba2 in nsCSSFrameConstructor::ConstructFramesFromItemList at nsCSSFrameConstructor.cpp:9505 #22 0x12e73b26 in nsCSSFrameConstructor::ProcessChildren at nsCSSFrameConstructor.cpp:9615 #23 0x12e74839 in nsCSSFrameConstructor::ConstructBlock at nsCSSFrameConstructor.cpp:10665 #24 0x12e7b701 in nsCSSFrameConstructor::ConstructDocElementFrame at nsCSSFrameConstructor.cpp:2497 #25 0x12e7bd8b in nsCSSFrameConstructor::ContentRangeInserted at nsCSSFrameConstructor.cpp:6916 #26 0x12e7d0de in nsCSSFrameConstructor::ContentInserted at nsCSSFrameConstructor.cpp:6813 #27 0x12efbc52 in PresShell::InitialReflow at nsPresShell.cpp:2499 #28 0x12ebabea in DocumentViewerImpl::InitPresentationStuff at nsDocumentViewer.cpp:751 #29 0x12ebbe1e in DocumentViewerImpl::Show at nsDocumentViewer.cpp:1981 #30 0x14cecd16 in nsDocShell::SetVisibility at nsDocShell.cpp:4623 #31 0x131fabe1 in nsFrameLoader::Show at nsFrameLoader.cpp:561 #32 0x12f5a45d in nsSubDocumentFrame::ShowViewer at nsFrameFrame.cpp:342 #33 0x12f5c4fb in AsyncFrameInit::Run at nsFrameFrame.cpp:260 #34 0x13190ea8 in nsContentUtils::RemoveScriptBlocker at nsContentUtils.cpp:4484 #35 0x12e84e2b in nsAutoScriptBlocker::~nsAutoScriptBlocker at nsContentUtils.h:1791 #36 0x12f01903 in PresShell::FlushPendingNotifications at nsPresShell.cpp:4613 #37 0x131c480a in nsDocument::FlushPendingNotifications at nsDocument.cpp:6261 #38 0x13204a2a in nsGenericElement::GetPrimaryFrame at nsGenericElement.cpp:3804 #39 0x1321314d in nsGenericElement::GetStyledFrame at nsGenericElement.cpp:1459 #40 0x132f0c4a in nsGenericHTMLElement::GetOffsetRect at nsGenericHTMLElement.cpp:444 #41 0x132eb192 in nsGenericHTMLElement::GetOffsetWidth at nsGenericHTMLElement.cpp:575 #42 0x132f3227 in nsGenericHTMLElementTearoff::GetOffsetWidth at nsGenericHTMLElement.cpp:188 #43 0x139cfa1a in nsIDOMNSHTMLElement_GetOffsetWidth at dom_quickstubs.cpp:17578 #44 0x0013c7ff in JSScopeProperty::get at jsscope.h:996 #45 0x00133953 in js_NativeGet at jsobj.cpp:4617 #46 0x00133ed8 in js_GetPropertyHelper at jsobj.cpp:4789 #47 0x00103de8 in js_Interpret at jsops.cpp:1479 #48 0x0011ca00 in js_Invoke at jsinterp.cpp:831 #49 0x1395cf99 in nsXPCWrappedJSClass::CallMethod at xpcwrappedjsclass.cpp:1696 #50 0x13953d97 in nsXPCWrappedJS::CallMethod at xpcwrappedjs.cpp:570 #51 0x004944c0 in PrepareAndDispatch at xptcstubs_unixish_x86.cpp:93 #52 0x0048f003 in nsXPTCStubBase::Stub3 at xptcstubsdef.inc:1 #53 0x1329f285 in nsEventListenerManager::HandleEventSubType at nsEventListenerManager.cpp:1085 #54 0x1329f669 in nsEventListenerManager::HandleEventInternal at nsEventListenerManager.cpp:1181 #55 0x132d0cb6 in nsEventListenerManager::HandleEvent at nsEventListenerManager.h:144 #56 0x132d0e79 in nsEventTargetChainItem::HandleEvent at nsEventDispatcher.cpp:212 #57 0x132cefb8 in nsEventTargetChainItem::HandleEventTargetChain at nsEventDispatcher.cpp:341 #58 0x132cfec6 in nsEventDispatcher::Dispatch at nsEventDispatcher.cpp:628 #59 0x134da53d in PostMessageEvent::Run at nsGlobalWindow.cpp:5469 #60 0x0046f076 in nsThread::ProcessNextEvent at nsThread.cpp:527 #61 0x003f0601 in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:200 #62 0x12cde541 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:126 #63 0x12c8955b in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:394 #64 0x9416115b in __CFRunLoopDoSources0 #65 0x9415ec1f in __CFRunLoopRun #66 0x9415e0f4 in CFRunLoopRunSpecific #67 0x9415df21 in CFRunLoopRunInMode #68 0x95c0f0fc in RunCurrentEventLoopInMode #69 0x95c0eeb1 in ReceiveNextEventCommon #70 0x95c0ed36 in BlockUntilNextEventMatchingListInMode #71 0x94738135 in _DPSNextEvent #72 0x94737976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] #73 0x946f9bef in -[NSApplication run] #74 0x12c882ad in nsAppShell::Run at nsAppShell.mm:747 #75 0x155fce32 in nsAppStartup::Run at nsAppStartup.cpp:184 #76 0x0001258d in XRE_main at nsAppRunner.cpp:3564 #77 0x00002629 in main at nsBrowserApp.cpp:158 This failed check causes CreateAnonymousContent to not add any nodes to aElements, and later on we crash here because we don't expect the case where there are no child frames: <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#2885>. Here is the crash stack: #0 0x12e5208f in nsIFrame::AddStateBits at nsIFrame.h:1180 #1 0x12ffcbd5 in nsTextControlFrame::SetInitialChildList at nsTextControlFrame.cpp:1418 #2 0x12e774d6 in nsCSSFrameConstructor::ConstructFrameFromItemInternal at nsCSSFrameConstructor.cpp:3868 #3 0x12e7776e in nsCSSFrameConstructor::ConstructFramesFromItem at nsCSSFrameConstructor.cpp:5460 #4 0x12e88ba2 in nsCSSFrameConstructor::ConstructFramesFromItemList at nsCSSFrameConstructor.cpp:9505 #5 0x12e73b26 in nsCSSFrameConstructor::ProcessChildren at nsCSSFrameConstructor.cpp:9615 #6 0x12e74839 in nsCSSFrameConstructor::ConstructBlock at nsCSSFrameConstructor.cpp:10665 #7 0x12e74c35 in nsCSSFrameConstructor::ConstructNonScrollableBlock at nsCSSFrameConstructor.cpp:4517 #8 0x12e76db4 in nsCSSFrameConstructor::ConstructFrameFromItemInternal at nsCSSFrameConstructor.cpp:3728 #9 0x12e7776e in nsCSSFrameConstructor::ConstructFramesFromItem at nsCSSFrameConstructor.cpp:5460 #10 0x12e88ba2 in nsCSSFrameConstructor::ConstructFramesFromItemList at nsCSSFrameConstructor.cpp:9505 #11 0x12e73b26 in nsCSSFrameConstructor::ProcessChildren at nsCSSFrameConstructor.cpp:9615 #12 0x12e74839 in nsCSSFrameConstructor::ConstructBlock at nsCSSFrameConstructor.cpp:10665 #13 0x12e7b701 in nsCSSFrameConstructor::ConstructDocElementFrame at nsCSSFrameConstructor.cpp:2497 #14 0x12e7bd8b in nsCSSFrameConstructor::ContentRangeInserted at nsCSSFrameConstructor.cpp:6916 #15 0x12e7d0de in nsCSSFrameConstructor::ContentInserted at nsCSSFrameConstructor.cpp:6813 #16 0x12efbc52 in PresShell::InitialReflow at nsPresShell.cpp:2499 #17 0x12ebabea in DocumentViewerImpl::InitPresentationStuff at nsDocumentViewer.cpp:751 #18 0x12ebbe1e in DocumentViewerImpl::Show at nsDocumentViewer.cpp:1981 #19 0x14cecd16 in nsDocShell::SetVisibility at nsDocShell.cpp:4623 #20 0x131fabe1 in nsFrameLoader::Show at nsFrameLoader.cpp:561 #21 0x12f5a45d in nsSubDocumentFrame::ShowViewer at nsFrameFrame.cpp:342 #22 0x12f5c4fb in AsyncFrameInit::Run at nsFrameFrame.cpp:260 #23 0x13190ea8 in nsContentUtils::RemoveScriptBlocker at nsContentUtils.cpp:4484 #24 0x12e84e2b in nsAutoScriptBlocker::~nsAutoScriptBlocker at nsContentUtils.h:1791 #25 0x12f01903 in PresShell::FlushPendingNotifications at nsPresShell.cpp:4613 #26 0x131c480a in nsDocument::FlushPendingNotifications at nsDocument.cpp:6261 #27 0x13204a2a in nsGenericElement::GetPrimaryFrame at nsGenericElement.cpp:3804 #28 0x1321314d in nsGenericElement::GetStyledFrame at nsGenericElement.cpp:1459 #29 0x132f0c4a in nsGenericHTMLElement::GetOffsetRect at nsGenericHTMLElement.cpp:444 #30 0x132eb192 in nsGenericHTMLElement::GetOffsetWidth at nsGenericHTMLElement.cpp:575 #31 0x132f3227 in nsGenericHTMLElementTearoff::GetOffsetWidth at nsGenericHTMLElement.cpp:188 #32 0x139cfa1a in nsIDOMNSHTMLElement_GetOffsetWidth at dom_quickstubs.cpp:17578 #33 0x0013c7ff in JSScopeProperty::get at jsscope.h:996 #34 0x00133953 in js_NativeGet at jsobj.cpp:4617 #35 0x00133ed8 in js_GetPropertyHelper at jsobj.cpp:4789 #36 0x00103de8 in js_Interpret at jsops.cpp:1479 #37 0x0011ca00 in js_Invoke at jsinterp.cpp:831 #38 0x1395cf99 in nsXPCWrappedJSClass::CallMethod at xpcwrappedjsclass.cpp:1696 #39 0x13953d97 in nsXPCWrappedJS::CallMethod at xpcwrappedjs.cpp:570 #40 0x004944c0 in PrepareAndDispatch at xptcstubs_unixish_x86.cpp:93 #41 0x0048f003 in nsXPTCStubBase::Stub3 at xptcstubsdef.inc:1 #42 0x1329f285 in nsEventListenerManager::HandleEventSubType at nsEventListenerManager.cpp:1085 #43 0x1329f669 in nsEventListenerManager::HandleEventInternal at nsEventListenerManager.cpp:1181 #44 0x132d0cb6 in nsEventListenerManager::HandleEvent at nsEventListenerManager.h:144 #45 0x132d0e79 in nsEventTargetChainItem::HandleEvent at nsEventDispatcher.cpp:212 #46 0x132cefb8 in nsEventTargetChainItem::HandleEventTargetChain at nsEventDispatcher.cpp:341 #47 0x132cfec6 in nsEventDispatcher::Dispatch at nsEventDispatcher.cpp:628 #48 0x134da53d in PostMessageEvent::Run at nsGlobalWindow.cpp:5469 #49 0x0046f076 in nsThread::ProcessNextEvent at nsThread.cpp:527 #50 0x003f0601 in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:200 #51 0x12cde541 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:126 #52 0x12c8955b in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:394 #53 0x9416115b in __CFRunLoopDoSources0 #54 0x9415ec1f in __CFRunLoopRun #55 0x9415e0f4 in CFRunLoopRunSpecific #56 0x9415df21 in CFRunLoopRunInMode #57 0x95c0f0fc in RunCurrentEventLoopInMode #58 0x95c0eeb1 in ReceiveNextEventCommon #59 0x95c0ed36 in BlockUntilNextEventMatchingListInMode #60 0x94738135 in _DPSNextEvent #61 0x94737976 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] #62 0x946f9bef in -[NSApplication run] #63 0x12c882ad in nsAppShell::Run at nsAppShell.mm:747 #64 0x155fce32 in nsAppStartup::Run at nsAppStartup.cpp:184 #65 0x0001258d in XRE_main at nsAppRunner.cpp:3564 #66 0x00002629 in main at nsBrowserApp.cpp:158 Now, looking over bug 287446, I think I can apply the same trick here again, but I have a few questions: 1. Is applying this trick here safe/correct? 2. Where should this trick be applied? Does nsTextEditorState::BindToFrame seem like a good place for that to happen? Should I make it localized (only wrapping nsPlaintextEditor::Init) or method scope? 3. How can we be sure that there aren't any more of these problems? It seems quite random, and I find it very challenging to try to reason about which places inside the code base this can happen.

> This failed check causes CreateAnonymousContent to not add any nodes to > aElements Hmm. ProcessChildren should probably check the rv from GetAnonymousContent and destroy the frames as needed in that case. > 1. Is applying this trick here safe/correct? Safe, yes. Correct in the sense that you don't have a better option. Note that the current code has such a trick applied; see the nsCxPusher in nsTextControlFrame::EnsureEditorInitializedInternal, which includes the Init() call on the editor. > Should I make it localized (only wrapping nsPlaintextEditor::Init) or method > scope? It probably doesn't matter much; I'd go with the smallest scope we're sure is big enough... > 3. How can we be sure that there aren't any more of these problems? We can't. The right solution is to move editor to internal non-DOM APIs that don't perform security checks for things like this. We have existing bugs on that, I think.

(In reply to comment #15) > > This failed check causes CreateAnonymousContent to not add any nodes to > > aElements > > Hmm. ProcessChildren should probably check the rv from GetAnonymousContent and > destroy the frames as needed in that case. Well, at least in this case that won't probably be the right thing to do, because you would get the wrong thing on your screen. :-) > > 1. Is applying this trick here safe/correct? > > Safe, yes. Correct in the sense that you don't have a better option. > > Note that the current code has such a trick applied; see the nsCxPusher in > nsTextControlFrame::EnsureEditorInitializedInternal, which includes the Init() > call on the editor. Yes. That's why I felt comfortable with asking this question, otherwise I'd have been too shy to ask this! > > Should I make it localized (only wrapping nsPlaintextEditor::Init) or method > > scope? > > It probably doesn't matter much; I'd go with the smallest scope we're sure is > big enough... Great, will do. I'll also make sure to put lots of comments inside the patch as to why I'm using this. > > 3. How can we be sure that there aren't any more of these problems? > > We can't. The right solution is to move editor to internal non-DOM APIs that > don't perform security checks for things like this. We have existing bugs on > that, I think. Hmm, that kind of contradicts with the plans that I have in my mind (which I don't yet know how practical they are anyway), that is, moving editor to js land where all it has to work with are DOM APIs...

> because you would get the wrong thing on your screen. :-) Better than crashing! > moving editor to js land where all it has to work with are DOM APIs... If it's implemented in JS, then there is no problem since its subject principal will presumably be system. The problem is using DOM APIs from C++, where you pick up whatever subject principal happens to be on the JS stack.

(In reply to comment #17) > > because you would get the wrong thing on your screen. :-) > > Better than crashing! Sure. Filed bug 565152 for that. > > moving editor to js land where all it has to work with are DOM APIs... > > If it's implemented in JS, then there is no problem since its subject principal > will presumably be system. The problem is using DOM APIs from C++, where you > pick up whatever subject principal happens to be on the JS stack. That's reassuring. Thanks!

Many test fixes - still more to come.

Fixed two crashes on frame destruction, and also fixed a few other problems.

This is the correct patch!

Added support for cycle collection in order to prevent memory leaks.

This should fix all the remaining test failures. I still need to clean the patch up to make it ready for review, but in the mean time, I'm requesting feedback from jst on the cycle collection stuff, because I'm not sure if I've got it 100% right. I'll also look into splitting this up into smaller chunks for review, but I'm not sure how much that would help, because mostly this patch refactors code existing in the text control frame to nsTextEditorState.

(In reply to comment #23) > This should fix all the remaining test failures. Spoke too soon. :( Here's a summary of the try server runs: On Linux, everything seems to be fine (except for a random orange). There was a Talos crash, which seems unrelated: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1274406259.1274407947.17523.gz On Windows, there are a lot of problems. I got one crash in test_jQuery.html <http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1274406445.1274407024.14023.gz&buildtime=1274406445&buildname=WINNT%205.2%20tryserver%20debug%20test%20mochitests-2%2f5&fulltext=1#err0> and four other crashes which seem to be cycle-collection related (but have a very similar stack to the other crash) like this one: <http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1274406446.1274407900.17414.gz&buildtime=1274406446&buildname=WINNT%205.2%20tryserver%20debug%20test%20mochitests-1%2f5&fulltext=1#err4>. These crashes look like coming from code which tries to release the same object twice, but I'm currently out of ideas as to what really causes them. I suspect I haven't got something right with regard to supporting cycle collection on nsTextEditorState, although I can't tell why it only shows up on Windows. On Mac, things seem to be green so far on a bunch of Talos machines. Unit test results are yet to come.

The Mac builds were green across the board, except for a mochitest-chrome leak which was actually bug 538462. The Windows optimized builds were all green as well. I'm investigating the crashes on Windows debug build right now.

I have run the *entire* Mochitest suite on a Windows debug build based on the same revision from mozilla-central and the same patch as the one I pushed to try three times since last night, and I've not been able to reproduce any crash (all three runs were 100% green.) I really need some help for determining what's going on here.

Try downloading the tryserver build and running the tests with that exact build?

(In reply to comment #27) > Try downloading the tryserver build and running the tests with that exact > build? That didn't also help. I've pushed a debugging patch to try to get a log of refcount changes on nsTextEditorState objects, to try to see what goes wrong there.

I think I've figured out the issue. The issue seems to be that I returned mRefCnt after calling |delete this| inside the Release function, and apprently on Windows debug builds, it's possible that the actual backing pages for a deleted object be freed immediately, causing the further hit to try to read the refcount value to cause an access violation. I'm trying to rip of some parts of the patch into other chunks, and then I'll submit the patch for review (given that the submitted try server runs are all green.)

I've added a few tests to test the correctness of setting the value on text controls when the frame will be destroyed as part of the value being set (with bug 336104 landing).

This part of the patch makes the editor object safe for multiple initializations. This means that with this patch, you can call nsEditor::Init to initialize the editor, followed by nsEditor::PreDestroy to uninitialize it, followed by nsEditor::Init to safely reinitialize it. This is needed so that we won't need to create a new editor object every time that the text editor state object is bound to a new frame. Changes to mViewManager make sure that it's not leaked when we reinitialize the editor. The rest of the changes just make sure that the correct set of operations done as part of the Init call will be undone as part of the PreDestroy call, and necessary cleanups are made to make a future call to Init possible.

Comment on attachment 446993 [details] [diff] [review] Part 2 - Make sure that setting the value in cases which causes the frame to be destroyed works correctly nice!

We should get rid of the nsViewManager reference. It's easy to get the view manager when you need it. Could be done in another bug.

(In reply to comment #33) > We should get rid of the nsViewManager reference. It's easy to get the view > manager when you need it. Could be done in another bug. Filed bug 567701 for that.

This is the gist of the work, moving everything that we can from the text control frame to the text editor state object (nsTextEditorState). nsTextEditorState implements the common stuff between input text controls and textareas. I've included some comments in nsTextEditorState.h, which hopefully can help in describing the design decisions for how this object works, so I recommend reading those comments before starting to review. I'm so sorry for the huge size of this patch, but this patch is mostly moving the code around, and I don't think I can split this into smaller chunks in any meaningful way. Let me know if you need me to describe why a given part of the patch works the way it does.

+ * Get the default value the text editor "of the" + NS_IMETHOD GetTextEditorDefaultValue(nsAString& aValue) = 0; I'm not sure what it means to get the default value of the text editor. Do you mean of the text control? Maybe this should just be "GetDefaultValue"? But what exactly does the "default value" mean here? I mean, every control presumably has a current value, so what is the default value and when would it be used instead of the current value? + NS_IMETHOD BindToTextControl(nsTextControlFrame* aFrame) = 0; + NS_IMETHOD UnbindFromTextControl(nsTextControlFrame* aFrame) = 0; Maybe BindToFrame/UnbindFromFrame? + /** + * Get the editor object associated with the text editor. + * aEditor is set to null if the control does not support an editor + * (for example, if it is a checkbox.) + */ + NS_IMETHOD GetTextEditor(nsIEditor** aEditor) = 0; Why not just return nsIEditor* directly, without adding a ref? + NS_IMETHOD GetSelectionController(nsISelectionController** aSelCon) = 0; Likewise, why not return value directly here? Can all of the NS_IMETHOD methods actually fail, and have those failures handled by the caller? I mean, can't some of these be void? GetWrapPropertyEnum should probably be non-inline. + // Pull IsSingleLineTextContorl into our scope, otherwise it'd be hidden IsSingleLineTextControl + * In mInputData, the mState field is used if the input is a text or password + * and mValue is used otherwise. We have to be careful when handling it + * on a type change. I think this comment should say "when IsSingleLineTextControl returns true" For all these methods in nsHTMLInputElement that do + if (!IsSingleLineTextControl(PR_FALSE)) { + return NS_ERROR_FAILURE; + } + + NS_ENSURE_TRUE(mInputData.mState, NS_ERROR_FAILURE); I think we should have a GetEditorState helper method that returns the state object if there is one, otherwise null. Why do we need both FrameDestroyed and UnbindFromFrame? + // Rsstore the value changed flag Restore + const nsresult rv = mInputData.mState->SetValue(aValue, aUserInput); Don't use 'const' on locals, it's unnecessary clutter. + if (NS_FAILED(rv)) { + // Rsstore the value changed flag + SetValueChanged(valueChanged); Can SetValue really have failed? + PRInt32 tmp = mType; + mType = newType; + isNewTypeSingleLine = IsSingleLineTextControl(PR_FALSE); + mType = tmp; ick. better to have IsSingleLineTextControlInternal that takes the type as a parameter. But why not just do "if (IsSingleLineTextControl(...)) { FreeData(); } ... if (IsSingleLineTextControl(...)) { mInputData.mState = new nsTextEditorState(this); }"? + nsITextControlElement* const mTextCtrlElement; I'm tempted to remove this, since you can get it through the frame, or passed in as a parameter to the methods on the state object. But maybe that adds too much verbosity. + nsRefPtr<nsTextInputSelectionImpl> mSelCon; + nsCOMPtr<nsIContent> mRootNode; + nsCOMPtr<nsIContent> mPlaceholderDiv; + nsTextInputListener* mTextListener; Since these only exist while the frame is bound, wouldn't it make sense to keep storing them in the frame and access them through the frame? + nsAutoPtr<nsCString> mValue; + mutable nsString mCachedValue; // Caches non-hard-wrapped value on a multiline control. Why do we need both of these? Why is mValue a CString? + nsAutoRefCnt mRefCnt; Use NS_INLINE_DECL_REFCOUNTING? + PRBool mFocusValueInit; + PRBool mOuterTransaction; + PRBool mInited; PRPackedBool In a couple of places in nsTextControlFrame we do this: + nsCOMPtr<nsIEditor> editor; + nsresult rv = GetEditor(getter_AddRefs(editor)); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr<nsIDOMElement> rootElement; - mEditor->GetRootElement(getter_AddRefs(rootElement)); + editor->GetRootElement(getter_AddRefs(rootElement)); Would it be simpler to call nsTextEditorState::GetRootNode now? + // exiting newline characters inside it, instead of relying on the Existing? The complexity of getting and setting values is still quite sad, but I guess to fix that we need to switch over to not using <br> in textareas.

> + NS_IMETHOD GetTextEditorDefaultValue(nsAString& aValue) = 0; > > I'm not sure what it means to get the default value of the text editor. Do you > mean of the text control? Maybe this should just be "GetDefaultValue"? But what > exactly does the "default value" mean here? I mean, every control presumably > has a current value, so what is the default value and when would it be used > instead of the current value? I guess I didn't do a very good job of picking names there. Maybe GetTextControlDefaultValue is a better choice (GetDefaultValue will clash with existing symbols). The notion of a default value means the value which was specified for the control in HTML. For input elements, this is the value attribute. For textareas, it's the contents of the text node under the element. See nsHTMLInputElement::GetDefaultValue and nsHTMLTextAreaElement::GetDefaultValue. > + /** > + * Get the editor object associated with the text editor. > + * aEditor is set to null if the control does not support an editor > + * (for example, if it is a checkbox.) > + */ > + NS_IMETHOD GetTextEditor(nsIEditor** aEditor) = 0; > > Why not just return nsIEditor* directly, without adding a ref? Many of the callers still need an addref, but yes, I guess deferring that to the callers makes sense. > + NS_IMETHOD GetSelectionController(nsISelectionController** aSelCon) = 0; > > Likewise, why not return value directly here? Done. > Can all of the NS_IMETHOD methods actually fail, and have those failures > handled by the caller? I mean, can't some of these be void? As it turns out, the only ones which can fail, and the callers can handle them in a meaningful way are CreateEditor and BindToFrame, so I made the rest of them void. > GetWrapPropertyEnum should probably be non-inline. Moved its implementation to nsTextEditorState.cpp. > For all these methods in nsHTMLInputElement that do > + if (!IsSingleLineTextControl(PR_FALSE)) { > + return NS_ERROR_FAILURE; > + } > + > + NS_ENSURE_TRUE(mInputData.mState, NS_ERROR_FAILURE); > I think we should have a GetEditorState helper method that returns the state > object if there is one, otherwise null. I did that. Unfortunately the code doesn't look that much cleaner though. :( > Why do we need both FrameDestroyed and UnbindFromFrame? As discussed on IRC, I originally came up with those two to map the former to nsTextControlFrame::DestroyFrom and the latter to nsTextControlFrame::PreDestroy. But it seems that those two methods can be merged, so I merged FrameDestroyed into UnbindFromFrame as well. > + const nsresult rv = mInputData.mState->SetValue(aValue, aUserInput); > > Don't use 'const' on locals, it's unnecessary clutter. Is that a general comment, or only about this particular case. I'm not sure if that's completely true for all local variables. Sometimes having const locals can prevent future mistakes, sometimes it's necessary for passing things down to other functions, etc. > + if (NS_FAILED(rv)) { > + // Rsstore the value changed flag > + SetValueChanged(valueChanged); > > Can SetValue really have failed? Not in a meaningful way. I removed the code to restore the value changed flag. > + PRInt32 tmp = mType; > + mType = newType; > + isNewTypeSingleLine = IsSingleLineTextControl(PR_FALSE); > + mType = tmp; > > ick. better to have IsSingleLineTextControlInternal that takes the type as a > parameter. > > But why not just do "if (IsSingleLineTextControl(...)) { FreeData(); } ... if > (IsSingleLineTextControl(...)) { mInputData.mState = new > nsTextEditorState(this); }"? Actually that code had a bug. What I really wanted for the first condition was |newTypeSingleLine && !currentTypeSingleLine| to optimize the case where type is changed from text to search, or something. I went ahead with the IsSingleLineTextControlInternal suggestion. > + nsITextControlElement* const mTextCtrlElement; > > I'm tempted to remove this, since you can get it through the frame, or passed > in as a parameter to the methods on the state object. But maybe that adds too > much verbosity. Well, to be fair, the current code is already much more verbose than necessary. But the main reason I decided to hold on to an nsITextControlElement was to avoid a bunch of QI's. If you still want me to remove this, I'll do it, anyway. > + nsRefPtr<nsTextInputSelectionImpl> mSelCon; > + nsCOMPtr<nsIContent> mRootNode; > + nsCOMPtr<nsIContent> mPlaceholderDiv; > + nsTextInputListener* mTextListener; > > Since these only exist while the frame is bound, wouldn't it make sense to keep > storing them in the frame and access them through the frame? I could do that, but I'm hoping that we can cleanup things even more in the future, so that we can actually reuse these objects between different frames, that's why I chose to move them to the editor state object. The current logic of where to store some of this stuff is still a bit convoluted. But I think from a design perspective, they actually belong to the text editor state object, more than they would belong to the frame object. And there are some more cleanups possible if these objects live in the content (e.g. nsTextInputListener::GetKeyBindings) but I didn't want to do them all in this patch, since it was already too large. > + nsAutoPtr<nsCString> mValue; > + mutable nsString mCachedValue; // Caches non-hard-wrapped value on a > multiline control. > > Why do we need both of these? mCachedValue is coming from bug 518122. Removing it would cause a perf regression. But it can be removed once we get rid of the br nodes inside textareas, I think. > Why is mValue a CString? Historical reasons (probably). For some reason we used to store it as a UTF-8 string in nsHTMLInputElement, and I don't feel comfortable changing that without knowing why we did it in the first place. > + nsAutoRefCnt mRefCnt; > > Use NS_INLINE_DECL_REFCOUNTING? Done. > In a couple of places in nsTextControlFrame we do this: > > + nsCOMPtr<nsIEditor> editor; > + nsresult rv = GetEditor(getter_AddRefs(editor)); > NS_ENSURE_SUCCESS(rv, rv); > > nsCOMPtr<nsIDOMElement> rootElement; > - mEditor->GetRootElement(getter_AddRefs(rootElement)); > + editor->GetRootElement(getter_AddRefs(rootElement)); > > Would it be simpler to call nsTextEditorState::GetRootNode now? It would be, but wouldn't that be wrong? Because this way we're guaranteed to initialize the editor if needed. > The complexity of getting and setting values is still quite sad, but I guess to > fix that we need to switch over to not using <br> in textareas. Yes, that would certainly help, but maybe not that much, unfortunately.

(In reply to comment #37) > I guess I didn't do a very good job of picking names there. Maybe > GetTextControlDefaultValue is a better choice (GetDefaultValue will clash with > existing symbols). > > The notion of a default value means the value which was specified for the > control in HTML. For input elements, this is the value attribute. For > textareas, it's the contents of the text node under the element. See > nsHTMLInputElement::GetDefaultValue and nsHTMLTextAreaElement::GetDefaultValue. How about GetDefaultValueFromContent? > > + const nsresult rv = mInputData.mState->SetValue(aValue, aUserInput); > > > > Don't use 'const' on locals, it's unnecessary clutter. > > Is that a general comment, or only about this particular case. I'm not sure if > that's completely true for all local variables. Sometimes having const locals > can prevent future mistakes, sometimes it's necessary for passing things down > to other functions, etc. It's a general comment. 'const' at the outermost level of types for local variables is only useful as a local annotation within the function, so isn't much use, and it's something extra to read for anyone reading the code ... especially because it's something programmers hardly ever do within or outside Mozilla, so anyone who sees it is going to be surprised and look for something more going on. IMHO. > The current logic of where to store some of this stuff is still a bit > convoluted. But I think from a design perspective, they actually belong to the > text editor state object, more than they would belong to the frame object. OK. > > In a couple of places in nsTextControlFrame we do this: > > > > + nsCOMPtr<nsIEditor> editor; > > + nsresult rv = GetEditor(getter_AddRefs(editor)); > > NS_ENSURE_SUCCESS(rv, rv); > > > > nsCOMPtr<nsIDOMElement> rootElement; > > - mEditor->GetRootElement(getter_AddRefs(rootElement)); > > + editor->GetRootElement(getter_AddRefs(rootElement)); > > > > Would it be simpler to call nsTextEditorState::GetRootNode now? > > It would be, but wouldn't that be wrong? Because this way we're guaranteed to > initialize the editor if needed. OK, how about adding a method GetRootNodeAndInitializeEditor? > > The complexity of getting and setting values is still quite sad, but I guess to > > fix that we need to switch over to not using <br> in textareas. > > Yes, that would certainly help, but maybe not that much, unfortunately. Is there a comment somewhere that explains where the "current value" lives at different times? I think it's something like this: a) on initial load, until the editable value is modified or it has a frame, the value is the default value, which is available in the DOM ('value' attribute or <textarea> content) b) if the value has been edited but the element currently has no editor, the value is mValue in the text editor state c) if the element has an editor, the value is a function of the editor's anonymous text is that right?

(In reply to comment #38) > (In reply to comment #37) > > I guess I didn't do a very good job of picking names there. Maybe > > GetTextControlDefaultValue is a better choice (GetDefaultValue will clash with > > existing symbols). > > > > The notion of a default value means the value which was specified for the > > control in HTML. For input elements, this is the value attribute. For > > textareas, it's the contents of the text node under the element. See > > nsHTMLInputElement::GetDefaultValue and nsHTMLTextAreaElement::GetDefaultValue. > > How about GetDefaultValueFromContent? Done. > > > In a couple of places in nsTextControlFrame we do this: > > > > > > + nsCOMPtr<nsIEditor> editor; > > > + nsresult rv = GetEditor(getter_AddRefs(editor)); > > > NS_ENSURE_SUCCESS(rv, rv); > > > > > > nsCOMPtr<nsIDOMElement> rootElement; > > > - mEditor->GetRootElement(getter_AddRefs(rootElement)); > > > + editor->GetRootElement(getter_AddRefs(rootElement)); > > > > > > Would it be simpler to call nsTextEditorState::GetRootNode now? > > > > It would be, but wouldn't that be wrong? Because this way we're guaranteed to > > initialize the editor if needed. > > OK, how about adding a method GetRootNodeAndInitializeEditor? Done. > > > The complexity of getting and setting values is still quite sad, but I guess to > > > fix that we need to switch over to not using <br> in textareas. > > > > Yes, that would certainly help, but maybe not that much, unfortunately. > > Is there a comment somewhere that explains where the "current value" lives at > different times? > > I think it's something like this: > a) on initial load, until the editable value is modified or it has a frame, the > value is the default value, which is available in the DOM ('value' attribute or > <textarea> content) > b) if the value has been edited but the element currently has no editor, the > value is mValue in the text editor state > c) if the element has an editor, the value is a function of the editor's > anonymous text > > is that right? That's right, and you're right that this needs documentation. I added some comments to nsTextEditorState.h explaining this.

That can't be right since it doesn't cover the case where the element has a frame but the value hasn't been edited :-). Your text in nsTextEditorState.h is ambiguous about that as well. Could we say a) if the element has a frame, the current value is the frame's anonymous text b) otherwise, if the element's state's mValue is non-null, that is the current value c) otherwise, the current value is the default value obtained from the DOM

(In reply to comment #40) > That can't be right since it doesn't cover the case where the element has a > frame but the value hasn't been edited :-). Your text in nsTextEditorState.h is > ambiguous about that as well. The existence of a frame is not significant in that case. In other words, if the value is not changed through the DOM, and an editor has not been initialized for the control (which happens when the element is first being created by the parser, for example), case 1 applies. If the value is not changed through the DOM, but an editor has been initialized for it, then case 3 applies. Note that an editor cannot be initialized for a text control if it does not have a frame bound to it. So, we're bound to have a frame in case 3. Now, if we're in case 3, and the frame goes away for some reason, then case 4 starts to apply. But reading those comments again, it's still confusing. The perfect way to document this behavior is using a state transition diagram, really. I'm not sure how to convert such a diagram to a text comment, and keep it short and simple. :/ > Could we say > a) if the element has a frame, the current value is the frame's anonymous text No. For example, if we have an <input> which is displayed on a page, but has not had its editor initialized yet, one of the first two cases apply to it. > b) otherwise, if the element's state's mValue is non-null, that is the current > value mValue can be non-null in some cases where it's not being used, for example, in this test case: <input> var i = document.querySelector("input"); i.value = "xxx"; i.focus(); // now, mValue is non-null, but it is not used i.style.display = "none"; document.body.offsetWidth; // now, mValue is non-null, but it is used Perhaps we could change this behavior in another bug. I just inherited it from the current way that things work, but it's wasteful. > c) otherwise, the current value is the default value obtained from the DOM This is not true because (a) and (b) are not true. :-)

While running the browser-chrome private browsing tests locally, I noticed that sometimes both the value and placeholder are displayed at the same time in the location bar with this patch. The problem was that I didn't notify the placeholder class change for single-line text controls (for which mMutationObserver is always null), which caused that visual effect. This patch fixes the issue, and adds a reftest which triggers the bug to make sure that it's fixed and won't regress in the future. I'm submitting this fix as a separate patch on top of part 4, because it's a contained fix, and don't want to cause roc to have to go through the entire patch once again.

BTW, I'll probably fold all of the patches in this bug before landing, since the chunks are only useful for reviews...

I had seen crashes on try when running test_datepicker.xul with a stack like this: 0 xul.dll!nsEditor::GetShouldTxnSetSelection() [nsEditor.cpp:37ed3857a827 : 4254 + 0x3] eip = 0x104f9ada esp = 0x0012beec ebp = 0x0012bef0 ebx = 0x00000001 esi = 0x003677cf edi = 0x00000000 eax = 0xdddddddd ecx = 0xdddddddd edx = 0x0012bfe8 efl = 0x00010286 Found by: given as instruction pointer in context 1 xul.dll!nsAutoTxnsConserveSelection::nsAutoTxnsConserveSelection(nsEditor *) [nsEditorUtils.h:37ed3857a827 : 144 + 0x9] eip = 0x1026e6ab esp = 0x0012bef8 ebp = 0x0012befc Found by: call frame info 2 xul.dll!nsTextEditRules::WillInsertText(int,nsISelection *,int *,int *,nsAString_internal const *,nsAString_internal *,int) [nsTextEditRules.cpp:37ed3857a827 : 765 + 0x14] eip = 0x10504077 esp = 0x0012bf04 ebp = 0x0012c0b8 Found by: call frame info 3 xul.dll!nsTextEditRules::WillDoAction(nsISelection *,nsRulesInfo *,int *,int *) [nsTextEditRules.cpp:37ed3857a827 : 325 + 0x2f] eip = 0x10502ea1 esp = 0x0012c0c0 ebp = 0x0012c0e4 Found by: call frame info 4 xul.dll!nsPlaintextEditor::InsertText(nsAString_internal const &) [nsPlaintextEditor.cpp:37ed3857a827 : 770 + 0x3f] eip = 0x1025abd1 esp = 0x0012c0ec ebp = 0x0012c210 Found by: call frame info 5 xul.dll!nsTextEditorState::SetValue(nsAString_internal const &,int) [nsTextEditorState.cpp:37ed3857a827 : 1717 + 0x2b] eip = 0x107c2d4f esp = 0x0012c218 ebp = 0x0012c3b4 Found by: call frame info 6 xul.dll!nsHTMLInputElement::SetValueInternal(nsAString_internal const &,int) [nsHTMLInputElement.cpp:37ed3857a827 : 1316 + 0x12] eip = 0x108d92dd esp = 0x0012c3bc ebp = 0x0012c3d0 Found by: call frame info 7 xul.dll!nsHTMLInputElement::SetValue(nsAString_internal const &) [nsHTMLInputElement.cpp:37ed3857a827 : 1000 + 0x10] eip = 0x108d85c5 esp = 0x0012c3d8 ebp = 0x0012c3e0 Found by: call frame info 8 xul.dll!nsIDOMHTMLInputElement_SetValue [dom_quickstubs.cpp:37ed3857a827 : 14974 + 0x19] eip = 0x10497ce8 esp = 0x0012c3e8 ebp = 0x0012c434 Found by: call frame info 9 mozjs.dll!JSScopeProperty::set(JSContext *,JSObject *,int *) [jsscope.h:37ed3857a827 : 1015 + 0x45] eip = 0x00626204 esp = 0x0012c43c ebp = 0x0012c458 Found by: call frame info 10 mozjs.dll!js_NativeSet [jsobj.cpp:37ed3857a827 : 4678 + 0x13] eip = 0x00625f56 esp = 0x0012c460 ebp = 0x0012c498 Found by: call frame info 11 mozjs.dll!js_Interpret [jsops.cpp:37ed3857a827 : 1701 + 0x150] eip = 0x005facf2 esp = 0x0012c4a0 ebp = 0x0012ca64 Found by: call frame info 12 mozjs.dll!js_Invoke [jsinterp.cpp:37ed3857a827 : 831 + 0x8] eip = 0x005ec237 esp = 0x0012ca6c ebp = 0x0012cb38 Found by: call frame info 13 mozjs.dll!js_InternalInvoke [jsinterp.cpp:37ed3857a827 : 882 + 0x14] eip = 0x005ece22 esp = 0x0012cb40 ebp = 0x0012cb60 Found by: call frame info 14 mozjs.dll!js_InternalGetOrSet [jsinterp.cpp:37ed3857a827 : 919 + 0x1e] eip = 0x005ecf4e esp = 0x0012cb68 ebp = 0x0012cb88 Found by: call frame info 15 mozjs.dll!JSScopeProperty::set(JSContext *,JSObject *,int *) [jsscope.h:37ed3857a827 : 1006 + 0x22] eip = 0x00626168 esp = 0x0012cb90 ebp = 0x0012cbbc Found by: call frame info 16 mozjs.dll!js_SetPropertyHelper [jsobj.cpp:37ed3857a827 : 4991 + 0x13] eip = 0x00626feb esp = 0x0012cbc4 ebp = 0x0012cc34 Found by: call frame info 17 mozjs.dll!js_Interpret [jsops.cpp:37ed3857a827 : 1821 + 0x1e] eip = 0x005fb43c esp = 0x0012cc3c ebp = 0x0012d200 Found by: call frame info 18 mozjs.dll!js_Invoke [jsinterp.cpp:37ed3857a827 : 831 + 0x8] eip = 0x005ec237 esp = 0x0012d208 ebp = 0x0012d2d4 Found by: call frame info 19 mozjs.dll!js_InternalInvoke [jsinterp.cpp:37ed3857a827 : 882 + 0x14] eip = 0x005ece22 esp = 0x0012d2dc ebp = 0x0012d2fc Found by: call frame info 20 mozjs.dll!JS_CallFunctionValue [jsapi.cpp:37ed3857a827 : 4922 + 0x1e] eip = 0x0058b8ad esp = 0x0012d304 ebp = 0x0012d324 Found by: call frame info 21 xul.dll!nsXBLProtoImplAnonymousMethod::Execute(nsIContent *) [nsXBLProtoImplMethod.cpp:37ed3857a827 : 331 + 0x22] eip = 0x10ad1091 esp = 0x0012d32c ebp = 0x0012d3c8 Found by: call frame info 22 xul.dll!nsXBLPrototypeBinding::BindingAttached(nsIContent *) [nsXBLPrototypeBinding.cpp:37ed3857a827 : 487 + 0x11] eip = 0x107a839d esp = 0x0012d3d0 ebp = 0x0012d3d8 Found by: call frame info 23 xul.dll!nsXBLBinding::ExecuteAttachedHandler() [nsXBLBinding.cpp:37ed3857a827 : 976 + 0x11] eip = 0x107a5096 esp = 0x0012d3e0 ebp = 0x0012d3e8 Found by: call frame info 24 xul.dll!nsXBLBinding::ExecuteAttachedHandler() [nsXBLBinding.cpp:37ed3857a827 : 973 + 0x11] eip = 0x107a5078 esp = 0x0012d3f0 ebp = 0x0012d3f4 Found by: call frame info 25 xul.dll!nsBindingManager::ProcessAttachedQueue(unsigned int) [nsBindingManager.cpp:37ed3857a827 : 1024 + 0xe] eip = 0x1070cd3c esp = 0x0012d3fc ebp = 0x0012d408 Found by: call frame info 26 xul.dll!PresShell::InitialReflow(int,int) [nsPresShell.cpp:37ed3857a827 : 2544 + 0x16] eip = 0x102b4ec6 esp = 0x0012d410 ebp = 0x0012d4e0 Found by: call frame info 27 xul.dll!nsXULDocument::StartLayout() [nsXULDocument.cpp:37ed3857a827 : 2048 + 0x1f] eip = 0x10379196 esp = 0x0012d4e8 ebp = 0x0012d540 Found by: call frame info 28 xul.dll!nsXULDocument::DoneWalking() [nsXULDocument.cpp:37ed3857a827 : 3195 + 0x7] eip = 0x1037c398 esp = 0x0012d548 ebp = 0x0012d5c0 Found by: call frame info 29 xul.dll!nsXULDocument::ResumeWalk() [nsXULDocument.cpp:37ed3857a827 : 3144 + 0xa] eip = 0x1037c115 esp = 0x0012d5c8 ebp = 0x0012d6ac Found by: call frame info 30 xul.dll!nsXULDocument::OnStreamComplete(nsIStreamLoader *,nsISupports *,unsigned int,unsigned int,unsigned char const *) [nsXULDocument.cpp:37ed3857a827 : 3602 + 0xd] eip = 0x1037d595 esp = 0x0012d6b4 ebp = 0x0012d76c Found by: call frame info 31 xul.dll!nsStreamLoader::OnStopRequest(nsIRequest *,nsISupports *,unsigned int) [nsStreamLoader.cpp:37ed3857a827 : 125 + 0x3d] eip = 0x10063576 esp = 0x0012d774 ebp = 0x0012d794 Found by: call frame info 32 xul.dll!nsHttpChannel::OnStopRequest(nsIRequest *,nsISupports *,unsigned int) [nsHttpChannel.cpp:37ed3857a827 : 5321 + 0x49] eip = 0x10108548 esp = 0x0012d79c ebp = 0x0012d7e0 Found by: call frame info 33 xul.dll!nsInputStreamPump::OnStateStop() [nsInputStreamPump.cpp:37ed3857a827 : 578 + 0x32] eip = 0x1007477e esp = 0x0012d7e8 ebp = 0x0012d80c Found by: call frame info 34 xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream *) [nsInputStreamPump.cpp:37ed3857a827 : 403 + 0xa] eip = 0x10074160 esp = 0x0012d814 ebp = 0x0012d81c Found by: call frame info 35 xul.dll!nsInputStreamReadyEvent::Run() [nsStreamUtils.cpp:37ed3857a827 : 112 + 0x27] eip = 0x110337ea esp = 0x0012d824 ebp = 0x0012d830 Found by: call frame info 36 xul.dll!nsThread::ProcessNextEvent(int,int *) [nsThread.cpp:37ed3857a827 : 547 + 0x18] eip = 0x1106436a esp = 0x0012d838 ebp = 0x0012d86c Found by: call frame info 37 xul.dll!NS_ProcessNextEvent_P(nsIThread *,int) [nsThreadUtils.cpp:37ed3857a827 : 250 + 0x15] eip = 0x11003023 esp = 0x0012d874 ebp = 0x0012d888 Found by: call frame info 38 xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [MessagePump.cpp:37ed3857a827 : 118 + 0xd] eip = 0x1129c8ed esp = 0x0012d890 ebp = 0x0012d8bc Found by: call frame info 39 xul.dll!MessageLoop::RunInternal() [message_loop.cc:37ed3857a827 : 216 + 0x1e] eip = 0x110a9a26 esp = 0x0012d8c4 ebp = 0x0012d8e0 ebx = 0x7ffff000 Found by: call frame info 40 xul.dll!MessageLoop::RunHandler() [message_loop.cc:37ed3857a827 : 199 + 0x7] eip = 0x110a9962 esp = 0x0012d8e8 ebp = 0x0012d918 Found by: call frame info 41 xul.dll!MessageLoop::Run() [message_loop.cc:37ed3857a827 : 173 + 0x7] eip = 0x110a9843 esp = 0x0012d920 ebp = 0x0012d948 ebx = 0x0012baf8 Found by: call frame info 42 xul.dll!nsBaseAppShell::Run() [nsBaseAppShell.cpp:37ed3857a827 : 175 + 0xb] eip = 0x10e62830 esp = 0x0012d950 ebp = 0x0012d954 Found by: call frame info 43 xul.dll!nsAppShell::Run() [nsAppShell.cpp:37ed3857a827 : 239 + 0x8] eip = 0x10e61ea2 esp = 0x0012d95c ebp = 0x0012f8a8 Found by: call frame info 44 xul.dll!nsAppStartup::Run() [nsAppStartup.cpp:37ed3857a827 : 192 + 0x1b] eip = 0x10c5656a esp = 0x0012f8b0 ebp = 0x0012f8bc Found by: call frame info 45 xul.dll!XRE_main [nsAppRunner.cpp:37ed3857a827 : 3620 + 0x24] eip = 0x10013058 esp = 0x0012f8c4 ebp = 0x0012fed0 Found by: call frame info 46 firefox.exe!NS_internal_main(int,char * *) [nsBrowserApp.cpp:37ed3857a827 : 158 + 0x11] eip = 0x00402532 esp = 0x0012fed8 ebp = 0x0012ff34 Found by: call frame info 47 firefox.exe!wmain [nsWindowsWMain.cpp:37ed3857a827 : 120 + 0xc] eip = 0x00401cce esp = 0x0012ff3c ebp = 0x0012ff68 Found by: call frame info 48 firefox.exe!__tmainCRTStartup [crtexe.c : 594 + 0x18] eip = 0x004075b6 esp = 0x0012ff70 ebp = 0x0012ffb8 Found by: call frame info 49 firefox.exe!wmainCRTStartup [crtexe.c : 413 + 0x4] eip = 0x0040740d esp = 0x0012ffc0 ebp = 0x0012ffc0 ebx = 0x0012baf8 Found by: call frame info 50 kernel32.dll + 0x2f23a eip = 0x77e6f23b esp = 0x0012ffc8 ebp = 0x0012fff0 Found by: call frame info Today I finally managed to reproduce this. The problem was that IsPreformatted's call to FlushPendingNotifications causes the PrepareEditor event to run, which proceeded to call nsPlaintextEditor::Init, which caused the nsTextEditRules object on the stack being deleted. The correct fix here is to add a script blocker on the stack in nsTextEditorState::SetValue to make sure that any pending PrepareEditor calls are not executed until InsertText is fully executed. This is a fallout from bug 336104. I've filed bug 568362 to deal with the more general case of this problem. Also, I noticed that we don't protect nsTextEditorObjects from receiving multiple PrepareEditor calls. This patch adds this protection as well.

Comment on attachment 447434 [details] [diff] [review] Part 4: the gist I am not a content peer, but I think bz is (or should be!) so I'll do sr here.

Although if we can give the review to someone other than Boris, that would be a very good idea. jst or smaug maybe.

Comment on attachment 447434 [details] [diff] [review] Part 4: the gist jst, could you please review the content parts of this patch?

I fixed a problem in the cycle collection participation macros which caused crashes on try Windows and Linux builds. With this patch, I've got 3 green runs on this patch on try.

These are some useful reftests for us to have which test our handling of values in face of input type changes.

Comment on attachment 448555 [details] [diff] [review] Part 4: the gist The sheer size of this does make it very hard to review, but I did go through it fairly carefully and it looks good to me. The one type of nit I do have is the following: - In nsHTMLInputElement::GetTextEditor(): + if (state) { + return state->GetEditor(); + } else { + return nsnull; + } +} Remove the else-after-return here, and in all other cases, there's several in this file below this point. I didn't spot any outside of this file tho... r=jst with that.

Addressed the comment, folded all patches and landed as: http://hg.mozilla.org/mozilla-central/rev/2437636244f3

Fixed build bustage: http://hg.mozilla.org/mozilla-central/rev/2875f9a693ae

Wonderful!