534785 - Move the ownership of editor and selection controller from the text frame to the content node

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

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...

Comment 2

[(no longer active)]

Attachment: WIP 1

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[(no longer active)]

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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!

Comment 6

[(no longer active)]

(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.

Comment 7

[(no longer active)]

Attachment: WIP 2

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.

Comment 8

[(no longer active)]

Attachment: WIP 3

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

Comment 9

[(no longer active)]

Attachment: WIP 4

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.

Comment 10

[(no longer active)]

Attachment: Part 1 - Back out the weak frame checks to facilitate landing of bug 221820

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.

Comment 11

[(no longer active)]

Attachment: WIP 5

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.

Comment 12

[(no longer active)]

Attachment: WIP 6

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.

Comment 13

[(no longer active)]

Attachment: WIP 7

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.

Comment 14

[(no longer active)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[(no longer active)]

(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...

Comment 17

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 18

[(no longer active)]

(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!

Comment 19

[(no longer active)]

Attachment: WIP 8

Many test fixes - still more to come.

Comment 20

[(no longer active)]

Attachment: WIP 9

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

Comment 21

[(no longer active)]

Attachment: WIP 9

This is the correct patch!

Comment 22

[(no longer active)]

Attachment: WIP 10

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

Comment 23

[(no longer active)]

Attachment: WIP 11

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.

Comment 24

[(no longer active)]

(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.

Comment 25

[(no longer active)]

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.

Comment 26

[(no longer active)]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 28

[(no longer active)]

(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.

Comment 29

[(no longer active)]

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.)

Comment 30

[(no longer active)]

Attachment: Part 2 - Make sure that setting the value in cases which causes the frame to be destroyed works correctly

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).

Comment 31

[(no longer active)]

Attachment: Part 3 - Make editor safe for multiple initializations

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 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 34

[(no longer active)]

(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.

Comment 35

[(no longer active)]

Attachment: Part 4: the gist

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.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * 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.

Comment 37

[(no longer active)]

Attachment: Part 4: the gist

> +  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.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 39

[(no longer active)]

Attachment: Part 4: the gist

(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.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 41

[(no longer active)]

(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.  :-)

Comment 42

[(no longer active)]

Attachment: Part 5 - Notify the placeholder class change for single-line text controls in time

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.

Comment 43

[(no longer active)]

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

Comment 44

[(no longer active)]

Attachment: Part 6 - Fix a crash

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 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 47

[(no longer active)]

Comment on attachment 447434 [details] [diff] [review]
Part 4: the gist

jst, could you please review the content parts of this patch?

Comment 48

[(no longer active)]

Attachment: Part 4: the gist

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.

Comment 49

[(no longer active)]

Attachment: Part 7 - some more reftests

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

Comment 50

[Johnny Stenback (:jst)]

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.

Comment 51

[(no longer active)]

Addressed the comment, folded all patches and landed as:

http://hg.mozilla.org/mozilla-central/rev/2437636244f3

Comment 52

[(no longer active)]

Fixed build bustage:

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

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wonderful!