221820 - CreateBogusNodeIfNeeded is extremely expensive creating Input fields

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Ivan Eisen]

I am investigating why a form containing lots of input fields is so slow (over
10X slower than IE6).  I discovered that there is a call in
nsTextEditRules::Init(nsPlaintextEditor *aEditor, PRUint32 aFlags)
(CreateBogusNodeIfNeeded) that is consuming 10% of the time bringing up my test
case.  In the code I see a message that states //this method handles null
selection, which should never happen anyway can this call be removed?

See the code below:
 NS_IMETHODIMP

120 nsTextEditRules::Init(nsPlaintextEditor *aEditor, PRUint32 aFlags)
121 {
122   if (!aEditor) { return NS_ERROR_NULL_POINTER; }
123 
124   mEditor = aEditor;  // we hold a non-refcounted reference back to our editor
125   // call SetFlags only aftet mEditor has been initialized!
126   SetFlags(aFlags);
127   nsCOMPtr<nsISelection> selection;
128   mEditor->GetSelection(getter_AddRefs(selection));
129   NS_ASSERTION(selection, "editor cannot get selection");
130 
131   // remember our root node
132   nsCOMPtr<nsIDOMElement> bodyElement;
133   nsresult res = mEditor->GetRootElement(getter_AddRefs(bodyElement));
134   if (NS_FAILED(res)) return res;
135   if (!bodyElement) return NS_ERROR_NULL_POINTER;
136   mBody = do_QueryInterface(bodyElement);
137   if (!mBody) return NS_ERROR_FAILURE;
138 
139   // put in a magic br if needed
140   res = CreateBogusNodeIfNeeded(selection);   // this method handles null  
                           election, which should never happen anyway
141   if (NS_FAILED(res)) return res;
142

Comment 1

[Ivan Eisen]

Attachment: Document contains over 3000 Input fields

This testcase takes over 10X more time in Mozilla than IE6.0.

Comment 2

[Ivan Eisen]

Further investigation suggests that the Collapse call which is made twice after
the CreateBogusNodeIfNeeded takes about 70%of the time in CreateBogusNodeIfNeeded.

The aSelection->Collapse is made directly in CreateBogusNodeIfNeeded.

Also Collapse is reached after CreateBogusNodeIfNeeded calls InsertNode which calls
DoTransaction which calls the Collapse routine.

Comment 3

[Mike Holderness]

Loading the testcase in M 1.4 on NT 4/sp6a, memory
use (as measured by NT Task Manager) went up 30 MB.

Didn't get a timing. But it took nearly 4 times as
long to take down as it took to put up - much more
than a minute. And it only appeared to give 10 MB 
back (this may be a statement about NT TM, not 
about the world). 

So: nasty suspicion there's a Windows VM issue in
here too. 

Comment 4

[Joe Francis]

The comment abut null selection s meant to explain why we dont check for null
selection before calling CreateBogusNodeIfNeeded().  It does *not* mean that
CreateBogusNodeIfNeeded() is unneeded.  It is very much needed for form widgets,
else you will not get a caret to draw when you give them focus.

We may be able to get rid of a Collapse(), call, however.

I thought that the editors for form widgets werent' even getting created until
they got focus.  I know Buster did some work on that years ago.  Lazy editor
creation for widgets should eliminate this as a performance issue.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Joe, lazy editor creation never existed for text widgets.  I'm looking at
revision 1.2 of nsGfxTextControlFrame2.cpp here (back when it existed and was in
the "not yet part of build" stage), and the editor is created and inited
basically at frame construction time.

Lazy editor creation for text widgets would be a wonderful thing indeed...

Comment 6

[Boris Zbarsky [:bzbarsky]]

So... it looks like this could be done, maybe.  We would have to do a few things:

1)  Move the editor creation (and as much else as possible) out of
    nsTextControlFrame::CreateAnonymousContent and into InitEditor().
2)  Move the call to InitEditor() from nsTextControlFrame::PostCreateFrames() to
    nsTextControlFrame::GetEditor()
3)  Change users of mEditor to call InitEditor() as needed
4)  Fix things so that the text is painted even without the editor hooked up

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This does what I suggest in comment 6.	I left the selection controller magic
as it is, because I don't quite understand how it all hooks together and
because it looks like it may sometimes have to trigger editor instantiation
(via the GetEditor()) calls.  So I wasn't sure it would be safe to create all
that stuff inside the InitEditor() method.  If it's safe to move it over there,
that would be quite nice.

The main part of the patch just moves editor creation from
CreateAnonymousContent() to InitEditor(), adds calls to InitEditor() in various
places that want to ensure an editor exists, and sets up a lightweight
mechanism to display the current value when there is no editor (a textnode,
basically).

The HTMLInputElement and HTMLTextareaElement changes just make sure the the
frame is notified of value changes no matter whether it owns the value, since
now the frame may not own the value but may still be wanting to show the new
value.	I think these changes are safe, since I left the code on frame
destruction that sets the new value in the content node (since the frame itself
checks whether mUseEditor is true).

I'm not sure what the performance impact of this patch is, since I have no way
to measure that.  I did test the memory impact.  Without this patch, rendering
the provided testcase with 3300 inputs takes about 24MB of RAM here (that is,
loading that page increases memory usage by 24MB).  With the patch, that number
becomes 18MB.  That's still about 6kb per text input, which seems quite
excessive... so I would definitely like to spin off a bug about lazily
instantiating some of that selection stuff if this patch lands as-is.

Needless to say, I tested this somewhat (last 3 hours or so), and I've made
sure that things like XUL textfields, textfields on websites that call focus()
(eg google), password inputs, file inputs, textareas are still working
correctly.  Per forms standard operating procedure, this patch upload is being
done in a patched build.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133049 [details] [diff] [review]
Proposed patch

Joe, would you review?	Roc, would you sr?

Ideally, I'd like to have jkeiser look at this too, but I'm not sure he can...
:(

roc, I left the PostCreateFrames() method on nsIAnonymousContentCreator, but I
can remove that (and remove the call in the frame constructor), since no one
else uses it.  Let me know.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix up whitespace handling

This changes the handling of whitespace when we have no editor to be more like
what it should be.

This patch still has one problem -- clicking on a text input to focus it
positions the caret at the end of the text, not at the position where the click
happened.  I suspect this is due to the fact that the frame that the click
actually happened on (the textframe for our textnode) got wiped out when we set
up the editor, so something couldn't figure out what coords the click had
happened at... We really do need to fix that before this patch can go in.  I
won't be able to debug it till Wednesday, though, so if someone has time to
figure it out that would be great.

Comment 10

[Joe Francis]

Buster's (buster%netscape.com) work on this problem was for bug 14727.  The
checkins he made (initially) for this were landed 10/30/1999 09:33 to:
mozilla/ layout/ html/ forms/ src/ nsGfxTextControlFrame.cpp      rev 3.79
mozilla/ layout/ html/ forms/ src/ nsGfxAutoTextControlFrame.cpp  rev 1.7
mozilla/ layout/ html/ forms/ src/ nsGfxTextControlFrame.h        rev 3.31

The reason you don't see this work in revision history is that it predates the
existence of nsGfxTextControlFrame2.cpp. I doubt any of this is useful, but I
had to make sure I wasn't going nuts.  :-)

He later turned off the work for password fields and textareas, leaving it on
only for textfields.  The problem for password fields was that the dom
initialization was leaving the text in the clear, and the problem with textareas
was probably translating mouseclicks to the right offset in the editor (I
messaged you on irc about this earlier).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> The problem for password fields was that the dom initialization was leaving
> the text in the clear

Yeah... I handle that in my patch, but I still need to test that it handles the
mouseclick offset thing right.

> and the problem with textareas was probably translating mouseclicks to the
> right offset in the editor

The problem I was having in comment 9, exactly.  I didn't get your irc message,
so if you could email it to me that would be great.  Thanks for pointing me to
the old impl of this stuff.  I wonder why it wasn't carried over to ver2 of the
code.  :(

I'll have an updated patch for this that leaves textareas as-is (for the same
reasons as Buster) and only changes text and maybe password inputs by Wednesday
at the latest; possibly earlier.  I'd really like to land this for 1.6a.... ;)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My memory is that it wasn't carried over because it had a lot of bugs.  The
problem it was trying to solve was a performance/memory problem.  Version 2 of
the patch solved that memory and performance problem by adding the ability to
instantiate an editor on a part of a document instead of creating a docshell and
a document inside every textfield.  It's probably worth having a look at the
list of bugs fixed by the ender-lite landing.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The ender-lite landing was bug 34896.)

Comment 14

[Joe Francis]

bz: Here were my irc comments:

4:16 PM:     floppy: the main issue is multiline fields
4:16 PM:     floppy: when you draw them sans editor, they will likely be one big
textnode
4:16 PM:     floppy: with dom newlines embedded
4:17 PM:     floppy: but when they are shoved in an editor,
4:17 PM:     floppy: they will be multiple textnodes seperated by br nodes
4:18 PM:     floppy: so you really need a way to calculate your selection click
point into a character stream offset, and then a way to translate that character
stream offset into a {node,offset} dompoint for the editor
4:18 PM:     floppy: i know i revewed code to do this, either in find, or
spellcheck, or accessibility
4:18 PM:     floppy: it's out there somewhere

Comment 15

[Boris Zbarsky [:bzbarsky]]

David, I'll make sure to test the dependancies of bug 34896 with this patch.

Joe, what I may end up doing is making editor lazy for textfields, then filing a
separate bug on the textarea case....

Comment 16

[Joe Francis]

bz: sounds good.  I remember some talk about textfields allowing multiple lines
in some situations, but I think that never happened.  Akkana might remember...
I'm thinking it was a linux issue.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Current iteration

This solves all the problems mentioned so far except it still regresses bug
35184.	The problem is that once the editor has been instantiated, it handles
D&D to itself.. but D&D can't happen till the editor has been instantiated. 
One possible solution here is to instantiate the editor onmouseover in addition
to the various other places it can happen.  Thoughts?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

>instantiate the editor onmouseover

hm... shouldn't onmouseup be enough, if this is just for drag and drop?

Comment 19

[Boris Zbarsky [:bzbarsky]]

No, because the D&D gives insertion point feedback before the drop (try it, in a
current build).

Comment 20

[Joe Francis]

bz, I take it you don't need a review this weekend after all, unless the D&D
problem gets solved?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yeah; I'm not going to have time to really solve it before freeze... if someone
has time to pick this up and drive it in, that would be wonderful.

Comment 22

[Ivan Eisen]

We tried the patch and our performance got much worse.  The problem stems from
the fact that nsTextControlFrame::Reflow() and nsTextControlFrame::GetPrefSize()
both still repeatedly call InitEditor().   

Our application of the patch may be flawed (we forced it onto Moz 1.5);  however,
running it we found that the mUseEditor variable is not set to TRUE.  Thus,
we  continually execute InitEditor() over and over (Reflows).  Even if we
hardcode mUseEditor TRUE where it used to be set at the beginning of the routine
our times still don't benefit from this fix due to the Reflow() and
GetPrefSize()    
calls to InitEditor().

Ivan

Comment 23

[Boris Zbarsky [:bzbarsky]]

> however, running it we found that the mUseEditor variable is not set to TRUE

Then your application of the patch was flawed indeed.  See the following lines
in the diff:

  +  // Turn on mUseEditor so that subsequent calls will use the
  +  // editor.
  +  mUseEditor = PR_TRUE;

Note that calling InitEditor() after the first time should be pretty much a
no-op...  Could you possibly test this patch against trunk from the date I
posted it?  I doubt it applies to current trunk (I think I've had to merge a few
things since I posted the patch).

Comment 24

[Ivan Eisen]

Okay,  I fixed our patch and also removed the InitEdit() calls from
nsTextControlFrame::Reflow() and nsTextControlFrame::GetPrefSize()(Rev. 3.129).
 It seems to work well and is much faster. Fix improved time for 300 text fields
from
1.5 seconds to .9 seconds.   

Thanks,
Ivan

Comment 25

[Boris Zbarsky [:bzbarsky]]

I almost have this working.  One problem -- the text input inside <textbox>
elements in XUL does not seem to receive NS_DRAGDROP_START events, so I still
can't drag/drop to the URL bar.  I'm not really sure why it works with an editor
instantiated, but I'd think that the text control there would get a dragstart
event.... Neil, any idea what's up there?

Comment 26

[[e-mail]]

After applying your patch d&d doesn't work for me to any text input element.
(it's ok after I focus it, of course...)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with almost-working D&D

<sigh>.  Yes, of course.  See comment 17.

In any case, here is the patch that makes D&D work for normal inputs but not
for the URL bar, if it will help debug the XUL issue.  I never get the
DRAGDROP_ENTER event for the input inside the URL bar.

Comment 28

[[e-mail]]

> In any case, here is the patch that makes D&D work for normal inputs but not
> for the URL bar, if it will help debug the XUL issue.  I never get the
> DRAGDROP_ENTER event for the input inside the URL bar.

Well, the event does occur, but it's never targetted at the input, which is
where you're trying to catch it. If you use userContent.css to turn borders off
for all inputs you'll find it tricky to D&D into them too, the events are all
going to the inner DIV. Now there are two alternatives here: 1) do we need that
DIV [before we create the editor] 2) can we intercept mouse events before we
have created the editor. For 2) I'm thinking of how xul's ButtonBoxFrame works.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Ready for reviews, I think

Neil, good catch!  Yes, we do need mDivContent, and we don't want to intercept
all mouse events (that would break click-placement of the caret, I think).  But
we can certainly attach a capturing dragenter listener to mDivContent, and that
makes things quite happy.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Oops, that wasn't the right patch....

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134889 [details] [diff] [review]
Oops, that wasn't the right patch....

Joe?  David?  What do you think?

Comment 32

[[e-mail]]

With the last patch I'm seeing a trivial display issue that only seems to apply
to XUL textboxes (again!) - if you drag text over a XUL textbox for the first
time, then an extra caret appears - I think the newly created editor must be
assuming that it has the focus, or something...

Just found another bug - while a javascript prompt() seems to work OK, it
doesn't work if you pass a default value (2nd arg). Tabbing doesn't help,
although strangely enough shift tabbing works (eventually).

Comment 33

[Boris Zbarsky [:bzbarsky]]

Hmm..  I did see the painting issue, but I also get that in a vanilla nightly
(in fact, dragging to the URL bar has all sorts of painting issues in a vanilla
Linux nightly)...

I'll look into the prompt() thing.  Thanks for catching that (and if you can
attach a simple testcase that shows that problem, that would rock).

Comment 34

[Boris Zbarsky [:bzbarsky]]

I'm also seeing a possible problem with onchange events (eg in the dynamic
filter stuff in mailnews)... will look at that too.

Comment 35

[[e-mail]]

OK, so this is very weird - if I add a dump('\7') to textbox.xml's capturing
focus event listener, it doesn't fire when I tab to the textbox... if I then
switch windows and switch back it does fire, but I still see no caret...

OK, so more testing... it seems that the open location dialog calls select()
which forces the creation of the editor. The prompt dialog does not, which means
that it's actually the text listener that creates the editor, because it listens
for focus too. However three questions arise: 1. does this make the frame focus
listener irrelevant? 2. should the drag listener be incorporated into the text
listener? 3. What the hell is focus doing on the textbox? It's supposed to be
handled by JS in dialog.xml, but there's no dialog JS on my stack :-(

Comment 36

[Boris Zbarsky [:bzbarsky]]

> 1. does this make the frame focus listener irrelevant?

I couldn't tell, which is why I hacked both.

> 2. should the drag listener be incorporated into the text listener?

Possibly.  Lazily instantiating some of those listeners is a followup bug to
this one, methinks...

> 3. What the hell is focus doing on the textbox?

dialog.xml sets the focus off a timeout (see
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/dialog.xml#138).
 It looks like advanceFocusIntoSubtree() calls nsFocusController::MoveFocus
which calls nsEventStateManager::ShiftFocus which calls
nsEventStateManager::ShiftFocusInternal... etc, etc.  At some point the
nsTextInputListener::Focus() method _is_ called and the editor is
instantiated... I'm not sure why things aren't working out right there.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134889 [details] [diff] [review]
Oops, that wasn't the right patch....

Oh, and it looks like the mailnews thing was just us sucking over remote X as
usual.	So the prompt() thing is the remaining issue....

bryner, any idea why the focus goes bye-bye there?

Comment 38

[[e-mail]]

>>3. What the hell is focus doing on the textbox?
>dialog.xml sets the focus off a timeout
Except there was no timeout on my backtrace, the only JS was the prompt().
(I double checked by putting the prompt on a timeout.)

Comment 39

[[e-mail]]

Ah, the dialog's timeout fires before the window activates...

Comment 40

[[e-mail]]

The caret issues turn out not be caused by this patch, so you can relax :-)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Ok, interesting.  So the focus listener is notified when the window activates,
not when the timeout fires.  Since the timeout in fact focuses it, I'd think the
focus listener would be notified...

Comment 42

[Joe Francis]

bz: be sure to request me (yet again) when you are ready for review.

Comment 43

[[e-mail]]

I think my previous comment was somewhat confusing.
The dnd ghost caret is the only regression I se with the patch.
The other issue I was able to duplicate without the patch.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Well... when I click on the "URL" field, in a non-patched build, I get a prompt;
when I move my mouse to the prompt window, I get a blinking cursor in the
"value" field. In a patched build, I don't get that.  Are you saying you've
reproduced that broken behavior in a non-patched build?

Comment 45

[[e-mail]]

Now, I'm sure I had different behaviour when I tried this last week, and I've
got a somewhat patched tree, but I'm now seeing an extra invisible tab stop in
the forward tabbing order, and with the patch the focus originally goes to this
"extra" stop, but without the patch the focus is initially correct.

Comment 46

[[e-mail]]

OK, so I thought my problem was that I had changed the focus timeout in
dialog.xml but the problem does occur with the original dialog.xml (although I
erroneously thought that reverting my dialog.xml had fixed the problem) however
I was able to work around the problem by setting the timeout to 1000 (1 second).

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with various sorts of text controls, for practice

Comment 48

[[e-mail]]

OK, I think I found another regression - when initially focussing a textbox
using a controlling label's access key then the caret does not appear in the
textbox, although the select all key mysteriously fixes the focus too.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Attachment: Current state of things

I'm removing this from my tree, because merging it all the time is getting annoying.  If someone wants to take this up, please do.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Also needed for "current state of things"

Forgot the content part....

Comment 51

[Boris Zbarsky [:bzbarsky]]

I've ended up using most of this patch to fix bug 335615.  The things I left out of that bug are:

1) mDragDropListener stuff
2) Setting up the textnode with value in CreateAnonymousContents
3) Corresponding change to take out !defaultValue.IsEmpty() under InitEditor
4) Various InitEditor calls
5) UpdateValueDisplay implementation and callers.

Comment 52

[(no longer active)]

Attachment: WIP 1

This is a WIP patch which takes into account all of the points in comment 51 except item 1.  I have debugged a handful of crashes with this patch, but I'm getting a crash which I have not been able to find the reason for so far.

The crash happens in <http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug424698.html> on line 78.  I have been able to reduce that test case, which I'll post in a moment, and will comment more on that.

There is also another strange problem: in textareas and password elements, when I initially click on them to give them the focus, the caret doesn't get displayed in them, and the only way I have been able to get the caret to show up is to move around using the arrow keys.  This is strange in the sense that AFAICT, the current patch should not have any effect on textarea and password elements, they should be initialized eagerly like before.

Comments/suggestions/early drop-by reviews/etc. appreciated!

Comment 53

[(no longer active)]

Attachment: Crash testcase

Here is the crash test case that I have reduced from test_bug424698.html.  When I load this under the debugger, it seems that the mEditor member of the nsTextEditorFocusListener object is corrupted (it's not null, but it's definitely not pointing to a valid nsEditor object.  Here is the stack for the crash:

0   libxpcom_core.dylib           	0x003a50bd nsQueryInterface::operator()(nsID const&, void**) const + 31 (nsCOMPtr.cpp:47)
1   libgklayout.dylib             	0x03840c56 nsCOMPtr<nsIEditor>::assign_from_qi(nsQueryInterface, nsID const&) + 20 (nsCOMPtr.h:1189)
2   libgklayout.dylib             	0x03840cb0 nsCOMPtr<nsIEditor>::nsCOMPtr(nsQueryInterface) + 50 (nsCOMPtr.h:572)
3   libgklayout.dylib             	0x03dc8a78 nsTextEditorFocusListener::Blur(nsIDOMEvent*) + 322 (nsEditorEventListeners.cpp:1131)
4   libgklayout.dylib             	0x03ad1d01 DispatchToInterface(nsIDOMEvent*, nsIDOMEventListener*, unsigned int (nsIDOMEventListener::*)(nsIDOMEvent*), nsID const&) + 182 (nsEventListenerManager.cpp:172)
5   libgklayout.dylib             	0x03ad3d19 nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsPIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) + 1099 (nsEventListenerManager.cpp:1169)
6   libgklayout.dylib             	0x03b01042 nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, int, nsCxPusher*) + 412 (nsEventDispatcher.cpp:251)
7   libgklayout.dylib             	0x03b01292 nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, int, nsCxPusher*) + 248 (nsEventDispatcher.cpp:290)
8   libgklayout.dylib             	0x03b01f2b nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsPIDOMEventTarget>*) + 2409 (nsEventDispatcher.cpp:584)
9   libgklayout.dylib             	0x03cde8ab nsFocusManager::SendFocusOrBlurEvent(unsigned int, nsIPresShell*, nsIDocument*, nsISupports*, unsigned int, int) + 679 (nsFocusManager.cpp:1668)
10  libgklayout.dylib             	0x03cdeff8 nsFocusManager::Blur(nsPIDOMWindow*, nsPIDOMWindow*, int) + 1372 (nsFocusManager.cpp:1416)
11  libgklayout.dylib             	0x03ce3ea2 nsFocusManager::ClearFocus(nsIDOMWindow*) + 376 (nsFocusManager.cpp:508)
12  libgklayout.dylib             	0x03b1afda nsGenericHTMLElement::Blur() + 132 (nsGenericHTMLElement.cpp:2886)
13  libgklayout.dylib             	0x03b90e57 nsHTMLTextAreaElement::Blur() + 17 (nsHTMLTextAreaElement.cpp:293)
14  libxpcom_core.dylib           	0x0044a140 NS_InvokeByIndex_P + 99 (xptcinvoke_unixish_x86.cpp:179)
15  libxpconnect.dylib            	0x01557a1b XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 6823 (xpcwrappednative.cpp:2727)
16  libxpconnect.dylib            	0x01564a3f XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) + 778 (xpcwrappednativejsops.cpp:1756)
17  libmozjs.dylib                	0x00199b24 js_Invoke + 2511 (jsinterp.cpp:1376)
18  libmozjs.dylib                	0x00185af8 js_Interpret + 98072 (jsops.cpp:2305)
19  libmozjs.dylib                	0x00198d96 js_Execute + 1169 (jsinterp.cpp:1618)
20  libmozjs.dylib                	0x00108b2e JS_EvaluateUCScriptForPrincipals + 331 (jsapi.cpp:5058)
21  libgklayout.dylib             	0x03cd27a0 nsJSContext::EvaluateString(nsAString_internal const&, void*, nsIPrincipal*, char const*, unsigned int, unsigned int, nsAString_internal*, int*) + 1002 (nsJSEnvironment.cpp:1705)
22  libgklayout.dylib             	0x03a77eb4 nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&) + 1074 (nsScriptLoader.cpp:738)
23  libgklayout.dylib             	0x03a781e2 nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) + 402 (nsScriptLoader.cpp:651)
24  libgklayout.dylib             	0x03a7a7a1 nsScriptLoader::ProcessScriptElement(nsIScriptElement*) + 4959 (nsScriptLoader.cpp:598)
25  libgklayout.dylib             	0x03a75a45 nsScriptElement::MaybeProcessScript() + 387 (nsScriptElement.cpp:193)
26  libgklayout.dylib             	0x03b6f4e9 nsHTMLScriptElement::MaybeProcessScript() + 31 (nsHTMLScriptElement.cpp:555)
27  libgklayout.dylib             	0x03b6d7f7 nsHTMLScriptElement::DoneAddingChildren(int) + 33 (nsHTMLScriptElement.cpp:481)
28  libgklayout.dylib             	0x03ba2c55 HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement*, int) + 287 (nsHTMLContentSink.cpp:2955)
29  libgklayout.dylib             	0x03ba3e12 SinkContext::CloseContainer(nsHTMLTag, int) + 1388 (nsHTMLContentSink.cpp:1013)
30  libgklayout.dylib             	0x03ba4511 HTMLContentSink::CloseContainer(nsHTMLTag) + 301 (nsHTMLContentSink.cpp:2267)
31  libhtmlpars.dylib             	0x03016d75 CNavDTD::CloseContainer(nsHTMLTag, int) + 625 (CNavDTD.cpp:2704)
32  libhtmlpars.dylib             	0x0301c367 CNavDTD::HandleEndToken(CToken*) + 965 (CNavDTD.cpp:1627)
33  libhtmlpars.dylib             	0x0301ab24 CNavDTD::HandleToken(CToken*) + 1722 (CNavDTD.cpp:718)
34  libhtmlpars.dylib             	0x0301ca8b CNavDTD::BuildModel(nsITokenizer*, int, int, nsCString const*) + 911 (CNavDTD.cpp:301)
35  libhtmlpars.dylib             	0x030274e9 nsParser::BuildModel() + 289 (nsParser.cpp:2446)
36  libhtmlpars.dylib             	0x0302eec2 nsParser::ResumeParse(int, int, int) + 546 (nsParser.cpp:2344)
37  libhtmlpars.dylib             	0x0302e476 nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 790 (nsParser.cpp:2975)
38  libdocshell.dylib             	0x051f4f93 nsDocumentOpenInfo::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 99 (nsURILoader.cpp:306)
39  libnecko.dylib                	0x0292c650 nsBaseChannel::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned int, unsigned int) + 116 (nsBaseChannel.cpp:708)
40  libnecko.dylib                	0x0294046d nsInputStreamPump::OnStateTransfer() + 759 (nsInputStreamPump.cpp:508)
41  libnecko.dylib                	0x029409c0 nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 138 (nsInputStreamPump.cpp:398)
42  libxpcom_core.dylib           	0x003f66da nsInputStreamReadyEvent::Run() + 100 (nsStreamUtils.cpp:113)
43  libxpcom_core.dylib           	0x0042b5a4 nsThread::ProcessNextEvent(int, int*) + 676 (nsThread.cpp:527)
44  libxpcom_core.dylib           	0x003ae923 NS_ProcessPendingEvents_P(nsIThread*, unsigned int) + 146 (nsThreadUtils.cpp:200)
45  libwidget_mac.dylib           	0x035437c3 nsBaseAppShell::NativeEventCallback() + 181 (nsBaseAppShell.cpp:122)
46  libwidget_mac.dylib           	0x034f4797 nsAppShell::ProcessGeckoEvents(void*) + 497 (nsAppShell.mm:499)
47  com.apple.CoreFoundation      	0x96f778cb __CFRunLoopDoSources0 + 1563
48  com.apple.CoreFoundation      	0x96f7538f __CFRunLoopRun + 1071
49  com.apple.CoreFoundation      	0x96f74864 CFRunLoopRunSpecific + 452
50  com.apple.CoreFoundation      	0x96f74691 CFRunLoopRunInMode + 97
51  com.apple.HIToolbox           	0x95d6af0c RunCurrentEventLoopInMode + 392
52  com.apple.HIToolbox           	0x95d6acc3 ReceiveNextEventCommon + 354
53  com.apple.HIToolbox           	0x95d6ab48 BlockUntilNextEventMatchingListInMode + 81
54  com.apple.AppKit              	0x9530eac5 _DPSNextEvent + 847
55  com.apple.AppKit              	0x9530e306 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 156
56  com.apple.AppKit              	0x952d049f -[NSApplication run] + 821
57  libwidget_mac.dylib           	0x034f2ec5 nsAppShell::Run() + 291 (nsAppShell.mm:851)
58  libtoolkitcomps.dylib         	0x0591342a nsAppStartup::Run() + 148 (nsAppStartup.cpp:182)
59  XUL                           	0x0001123b XRE_main + 12109 (nsAppRunner.cpp:3491)
60  org.mozilla.firefox           	0x00002629 main + 714 (nsBrowserApp.cpp:158)
61  org.mozilla.firefox           	0x000022aa start + 54

The crash happens on this code: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsEditorEventListeners.cpp#1130>.

I have not been able to figure out what the source problem is, and I think I need help with debugging this.

Another (perhaps unrelated) thing I've noticed with a patched build is that I'm hitting this assertion on every page with a non-empty textarea or password element: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4150>, such as:

data:text/html,<input type=password value=x>
data:text/html,<textarea>x</textarea>

This code is called every time if we're initializing an editor and want to set its value before attaching it to a frame (which is what happens in InitEditor).  I think that we need to turn off that assertion, but I hate to do that before I fully understand that what I'm doing is safe.

Comment 54

[(no longer active)]

Attachment: WIP 2

So, with Boris' help, this version doesn't crash on Mochitests, and does not have any of the problems mentioned in comment 52.  There are still a few test failures that I'm debugging, though.

Comment 55

[(no longer active)]

Attachment: WIP 3

I'm cutting down the number of Mochitest failures, and some of them are proving to be extremely tricky.  So far, I'm down to 17 Mochitest failures (log pasted below):

60587 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug420499.xul | Caret should be visible when context menu open
60779 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Typing should work - got "Test", expected "TestFoo"
60783 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Shouldn't have lost our typed value - got "Test", expected "TestFoo"
60784 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Typing should still work - got "Test", expected "TestFooBar"
60796 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug345267.html | Should only delete one char - got "abcdefghijklm", expected "abcdefghijkl"
60797 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug345267.html | Should only delete one char again - got "abcdefghijklm", expected "abcdefghijk"
60833 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug446663.html | bug446663_b edit - got "1", expected "123"
60835 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug446663.html | bug446663_b undo - got "1", expected "123"
60854 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug477700.html | Typing should work - got "", expected "Test"
60855 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug477700.html | Undo should work - got "\n", expected ""
60856 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug477700.html | Redo should work - got "", expected "Test"
140429 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form9 username - got "", expected "form9userAB"
140432 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form9 username - got "A", expected "form9userAAB"
140434 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking length of expected menu - got 0, expected 1
140435 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking menu entry #0 - got undefined, expected "form9userAAB"
140436 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form9 username - got "A", expected "form9userAAB"
140437 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form9 password - got "", expected "form9pass"

Comment 56

[(no longer active)]

A lot of the Mochitest failures happen when sending keyboard events to a text box.  Basically here is what happens.

When nsIHTMLInputElement::Focus gets called, it doesn't seem to cause a selection on the element, so later down the road when nsEditor is trying to handle the keypress event, this check <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#4082> fails, because the active selection is empty (it has an empty range array), which causes the check to fail, and the keyboard event never reaches to the editor, doesn't update its internal state, so what the tests see is an input control which doesn't catch any keyboard input, and thus they fail.

I have not determined the reason for this yet, but if anyone has any ideas, please let me know!

Comment 57

[[e-mail]]

For comparison, Editor normally initialises the selection as follows:
nsTypedSelection::Collapse
InsertElementTxn::DoTransaction
nsEditor::DoTransaction
nsEditor::InsertNode
nsTextEditRules::CreateBogusNodeIfNeeded
nsTextEditRules::Init
nsPlaintextEditor::InitRules
nsPlaintextEditor::EndEditorInit
nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger
nsPlaintextEditor::Init

Comment 58

[Boris Zbarsky [:bzbarsky]]

Sounds like editor needs to init a selection even if not creating a bogus node?

Comment 59

[(no longer active)]

Attachment: WIP 4

Another work-in-progress.  In this patch I started to initialize the editor on most events excluding mouse-in/over/out and mutation events for which we're sure an underlying editor is not necessary, and removed a number of special cases which the previous code used to handle to initialize the editor on focus and on the dragenter event, as they are no longer needed.

This patch cuts down the number of Mochitest failures to 8:

60631 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug420499.xul | Caret should be visible when context menu open
61006 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Typing should work - got "Test", expected "TestFoo"
61010 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Shouldn't have lost our typed value - got "Test", expected "TestFoo"
61011 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug287446.html | Typing should still work - got "Test", expected "TestFooBar"
61023 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug345267.html | Should only delete one char - got "abcdefghijklm", expected "abcdefghijkl"
61024 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug345267.html | Should only delete one char again - got "abcdefghijklm", expected "abcdefghijk"
61057 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug446663.html | bug446663_b edit - got "1", expected "123"
61059 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug446663.html | bug446663_b undo - got "1", expected "123"

Next on my list is trying comment 58.

Comment 60

[(no longer active)]

(In reply to comment #57)
> For comparison, Editor normally initialises the selection as follows:
> nsTypedSelection::Collapse
> InsertElementTxn::DoTransaction
> nsEditor::DoTransaction
> nsEditor::InsertNode
> nsTextEditRules::CreateBogusNodeIfNeeded
> nsTextEditRules::Init
> nsPlaintextEditor::InitRules
> nsPlaintextEditor::EndEditorInit
> nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger
> nsPlaintextEditor::Init

With my lazy init patch, here is the stack for initializing the selection:

#0	0x0516284f in nsTypedSelection::Collapse at nsSelection.cpp:5210
#1	0x057607af in InsertElementTxn::DoTransaction at InsertElementTxn.cpp:140
#2	0x014ea3f7 in nsTransactionItem::DoTransaction at nsTransactionItem.cpp:225
#3	0x014eec77 in nsTransactionManager::BeginTransaction at nsTransactionManager.cpp:954
#4	0x014ed793 in nsTransactionManager::DoTransaction at nsTransactionManager.cpp:119
#5	0x0574a484 in nsEditor::DoTransaction at nsEditor.cpp:738
#6	0x05749421 in nsEditor::InsertNode at nsEditor.cpp:1449
#7	0x0572fd31 in nsTextEditRules::CreateBogusNodeIfNeeded at nsTextEditRules.cpp:1367
#8	0x05732a6a in nsTextEditRules::AfterEdit at nsTextEditRules.cpp:279
#9	0x057263cb in nsPlaintextEditor::EndOperation at nsPlaintextEditor.cpp:1797
#10	0x0572d06f in nsAutoRules::~nsAutoRules at nsEditorUtils.h:122
#11	0x05726976 in nsPlaintextEditor::DeleteSelection at nsPlaintextEditor.cpp:773
#12	0x051a6dd9 in nsTextControlFrame::SetValue at nsTextControlFrame.cpp:2709
#13	0x051a7e16 in nsTextControlFrame::EnsureEditorInitialized at nsTextControlFrame.cpp:1546

Comment 61

[(no longer active)]

Attachment: WIP 5

This version passes the entire unit test suite successfully.  I think this is mostly ready for review; I'll just try to think of a few more types of general tests for it like the existing reftest, and I'll submit it for review tomorrow, hopefully.

Comment 62

[(no longer active)]

Attachment: Patch (v1)

This is finally ready for review.  Boris, are you the appropriate person to ask review for on this patch?

Comment 63

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can review, sure, but someone else should also look over at least the text control frame changes.

I'm a little confused about nsTextEditRules::WillInsertText change.  Why is it needed?

I guess you don't need a fix for bug 240933 here because you're not using this for <textarea>?  Do things work right if you click on some random part of the control (i.e. is the caret positioned correctly after the click)?  I assume you checked that the various issues my original patch attempt had don't appear with this patch?

Comment 64

[(no longer active)]

Comment on attachment 416498 [details] [diff] [review]
Patch (v1)

(In reply to comment #63)
> Hmm.  I can review, sure, but someone else should also look over at least the
> text control frame changes.

Sure, I'll ask roc for a review on those parts.  I'm also switching the sr request to dbaron, since roc would be a reviewer here...

> I'm a little confused about nsTextEditRules::WillInsertText change.  Why is it
> needed?

This is just a refactoring.  I'm moving some of the code inline in that function to nsTextEditRules::HandleNewLines, which I also call from nsTextControlFrame::UpdateValueDisplay, so that things like |<input type="text" value="aaa&#10;bbb">| work correctly.

> I guess you don't need a fix for bug 240933 here because you're not using this
> for <textarea>?

That's right.  Once I fix that bug (which is probably my next thing to work on in editor land), we can easily start to lazy init textarea's as well.

>  Do things work right if you click on some random part of the
> control (i.e. is the caret positioned correctly after the click)?

Yes.

> I assume you
> checked that the various issues my original patch attempt had don't appear with
> this patch?

Yes, I did.  I've been using a build with this patch for the past few days, and AFAICT everything is working correctly.

Comment 65

[(no longer active)]

(In reply to comment #10)
> He later turned off the work for password fields and textareas, leaving it on
> only for textfields.  The problem for password fields was that the dom
> initialization was leaving the text in the clear

I'm looking into another patch to lazy init the password fields as well, and AFAICT the only problem with that mentioned on this bug is this comment, which I'm not sure how to interpret.

I'm planning on storing the value as usual in SetValue calls and passing only the asterisk characters to the textnode under the anonymous div until the editor for those fields is initialized.  Is there something wrong with this approach?

Comment 66

[Boris Zbarsky [:bzbarsky]]

> Is there something wrong with this approach?

No, that sounds good.

Comment 67

[(no longer active)]

Attachment: Lazily initialize password fields as well

This patch brings lazy initialization to password fields.  This should be applied on top of the previous patch.  I'll submit a folded patch for review in a moment.

Comment 68

[(no longer active)]

Attachment: Patch (v2)

Folded patch for review.

Comment 69

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

Basically, looks great!

+  // Here is the logic.  We only need to perform this check for text input
+  // controls because they are lazily initialized.  We don't need to initialize
+  // the control for certain types of events, because we know that those events
+  // are safe to be handled without the editor being initialized.  These events
+  // include: mousein/move/out, and DOM mutation events.
+  if ((mType == NS_FORM_INPUT_TEXT ||
+       mType == NS_FORM_INPUT_PASSWORD) &&
+      aVisitor.mEvent->eventStructType != NS_MUTATION_EVENT) {
+    PRBool isSafeMouseEvent = PR_FALSE;
+    if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT) {
+      switch (aVisitor.mEvent->message) {
+      case NS_MOUSE_MOVE:
+      case NS_MOUSE_ENTER:
+      case NS_MOUSE_EXIT:
+      case NS_MOUSE_ENTER_SYNTH:
+      case NS_MOUSE_EXIT_SYNTH:
+        isSafeMouseEvent = PR_TRUE;
+        break;
+      }
+    }

I would write this as a helper function, PRBool NeedToInitializeEditorForEvent().

+  /** Echo's the insertion text into the password buffer, and converts

"Echoes"

+  // instantiating the editor for all text fields, even though if they

"Even if they"

+  if (IsFocusedContent(GetContent()))
+    SetFocus(PR_TRUE, PR_FALSE);

{} around the if body

GetEditor, SelectAllContents, SetSelectionRange, SetSelectionStart, SetSelectionEnd, DOMPointToOffset, OffsetToDOMPoint, GetSelectionRange, and GetPhonetic all can destroy 'this' now. How can we be confident that this won't cause any problems? Would it make sense to move some of these to nsHTMLInputElement?

Comment 70

[(no longer active)]

(In reply to comment #69)
> I would write this as a helper function, PRBool
> NeedToInitializeEditorForEvent().

Done.

> +  /** Echo's the insertion text into the password buffer, and converts
> 
> "Echoes"
> 
> +  // instantiating the editor for all text fields, even though if they
> 
> "Even if they"
> 
> +  if (IsFocusedContent(GetContent()))
> +    SetFocus(PR_TRUE, PR_FALSE);
> 
> {} around the if body

Fixed them all.

> GetEditor, SelectAllContents, SetSelectionRange, SetSelectionStart,
> SetSelectionEnd, DOMPointToOffset, OffsetToDOMPoint, GetSelectionRange, and
> GetPhonetic all can destroy 'this' now. How can we be confident that this won't
> cause any problems? Would it make sense to move some of these to
> nsHTMLInputElement?

I assume you mean moving the call to EnsureEditorInitialized into the counterpart nsHTMLInputElement methods.  How is that going to make us any safer than the current situation?  EnsureEditorInitialized currently makes sure to return an error code if the frame is destroyed while it's doing its job; doesn't that provide us with enough protection?

Comment 71

[(no longer active)]

Attachment: Patch (v2.1)

Oh, I meant to actually attach the patch as well...

Comment 72

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

I mean moving some or all of those methods into nsHTMLInputElement.

Comment 73

[(no longer active)]

OK, here is a summary of these methods.

* GetEditor: nsHTMLInputElement and nsHTMLTextAreaElement both provide a GetEditor method which forwards its job to nsGenericHTMLElement::GetEditor, which in turn calls nsTextFrameControl::GetEditor.  I'm not sure if moving this method to nsGenericHTMLElement is effective here, since we have to initialize the editor if it's not been initialized anyway, which might destroy the text control frame.

* SelectAllContents is actually only called for <select> elements: <http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.cpp#1912>.

* GetPhonetic needs access to mEditor, so even if we move it to nsHTMLInputElement, we would still need to initialize the editor.

* OffsetToDOMPoint and DOMPointToOffset are internal nsTextControlFrame methods (used by the below methods), so I don't think it makes much sense to move them out of it.

* SetSelection{Start,End,Range} are needed both by nsHTMLInputElement and nsTextAreaElement, so if we move them to the content classes, we would have to duplicate their code.

What do you think?

Comment 74

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

What if we had the editor and selection controller owned by the element? Could we then have the frame just call back to the element to initialize them and get them?

Comment 75

[(no longer active)]

We could do that, but I'm not sure if we really want that.  nsHTMLInputElement is more than just <input type=text> and <input type=password>.  Other input types don't need an editor, and I think that's basically the reason why the editor and selection controller are owned by the frame right now (that, and the fact that they're needed for textarea's as well.)

Comment 76

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

We could create a "text editor state" object to hold the editor and selection controller (and other stuff), and give nsHTMLInputElement and nsHTMLTextAreaElement a pointer to such an object, and make that object available to the frame via a method on nsITextControlElement.

Generally speaking, having things owned by content is better than having them owned by frames. For example, this would let us fix the bug that anything that causes frame reconstruction (e.g. dragging a tab from one window to another) forgets the selection.

It would be nice to not have to do those changes for this bug, but the alternative of auditing calls to those methods to make sure they're safe might be wasted effort in the long run.

Comment 77

[(no longer active)]

OK, makes sense.  I filed bug 534785 about this, and assigned it to myself.  If you want to make sure that the patch on that bug (which is not written yet!) lands before this one, please make this bug dependent on bug 534785.

Comment 78

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

In nsHTMLInputElement::NeedToInitializeEditorForEvent, the test aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT is not needed.

Comment 79

[(no longer active)]

Attachment: Patch (v2.2)

(In reply to comment #78)
> In nsHTMLInputElement::NeedToInitializeEditorForEvent, the test
> aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT is not needed.

OK, I removed it.

Comment 80

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you summarize what this patch is changing and why?

Comment 81

[(no longer active)]

(In reply to comment #80)
> Could you summarize what this patch is changing and why?

Sure.  This bug is about initializing the editor behind <input type=text|password> fields lazily (when user focuses the input field), instead of on page load.

The patch contains two refactorings which expose some existing code to external callers, in order to avoid duplicating the same functionality around the code base.  These refactorings are the new methods nsPlaintextEditor::GetDefaultEditorPrefs and nsTextEditRules::HandleNewLines.

I have also made the existing nsTextEditRules::FillBufWithPWChars method public so that my code can use it outside of nsTextEditRules.

Furthermore, this patch adds a new method to nsITextControlFrame, called EnsureEditorInitialized, which is supposed to make sure that the editor associated with a text control frame is initialized.  This method is to be called when a situation which requires the editor being initialized arises (such as a Focus call on the content node, or receiving an event which requires the editor to be initialized in order for the event to be processed correctly.  The reason I had to add this to nsITextControlFrame is that content is using that abstraction for calls into the frame, and does not access nsTextControlFrame directly.

I think that is everything from the sr perspective.

Comment 82

[(no longer active)]

Attachment: Patch (v2.3)

The previous version of the patch had a bug.  With an <input type=password> (with an empty value), if you clicked on it to initialize the editor, the editor was being initialized with a space character inside it.  This bug is fixed in this revision of the patch.

Comment 83

[(no longer active)]

Attachment: Patch (v2.4)

Patch updated to tip.  The changes made to the tree caused some other tests to fail, but this patch fixes all of these issues, instead of a reproducible leak on a11y tests, which I'll try to debug with David tomorrow.

Here is the leak log:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3680 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3680 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AtomImpl with size 20 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AtomImpl with size 20 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 5 instances of gfxFont with size 72 bytes each (360 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 5 instances of gfxFont with size 72 bytes each (360 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of gfxFontEntry with size 40 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of gfxFontEntry with size 40 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4 instances of gfxGlyphExtents with size 40 bytes each (160 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 4 instances of gfxGlyphExtents with size 40 bytes each (160 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 18 instances of gfxTextRunFactory with size 8 bytes each (144 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 18 instances of gfxTextRunFactory with size 8 bytes each (144 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsFontCache with size 8 bytes each (48 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsFontCache with size 8 bytes each (48 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsScreenCocoa with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsScreenCocoa with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsScreenManagerCocoa with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsScreenManagerCocoa with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 43 instances of nsStringBuffer with size 8 bytes each (344 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 43 instances of nsStringBuffer with size 8 bytes each (344 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 38 instances of nsTArray_base with size 4 bytes each (152 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 38 instances of nsTArray_base with size 4 bytes each (152 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsThebesDeviceContext with size 88 bytes each (528 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsThebesDeviceContext with size 88 bytes each (528 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 18 instances of nsThebesFontMetrics with size 68 bytes each (1224 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 18 instances of nsThebesFontMetrics with size 68 bytes each (1224 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsThebesRenderingContext with size 64 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsThebesRenderingContext with size 64 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsViewManager with size 80 bytes each (480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of nsViewManager with size 80 bytes each (480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsVoidArray with size 4 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsVoidArray with size 4 bytes

Comment 84

[(no longer active)]

I've tried to inspect the leak using the instructions at: <http://www.mozilla.org/performance/leak-tutorial.html>.  All paths seem to end to nsViewManager, but when I run the leak detection tool on that, its refcount tree is actually balanced (which should mean there is no leak.)  In the refcount logs for nsViewManager, there are quite a few stacks including accessibility stuff, but I've inespected them all manually just to check my sanity, and the AddRef/Release counts on all of them are actually balanced (and the refcount of the nsViewManager object actually reaches zero at the end.)

I also noticed that I'm getting lots of "Bad accessible tree" warnings, so that's where I'm continuing debugging this problem now, because I don't have any more ideas on the leaks at the moment.

Comment 85

[(no longer active)]

This seems like to be a reentrancy issue, caused by the accessibility module calling into nsTextControlFrame::EnsureEditorInitialized, like this:

#0	0x005fa9b2 in nsTextControlFrame::EnsureEditorInitialized at nsTextControlFrame.cpp:1403
#1	0x005f1d0b in nsTextControlFrame::GetEditor at nsTextControlFrame.cpp:1923
#2	0x008c49e0 in nsGenericHTMLElement::GetEditorInternal at nsGenericHTMLElement.cpp:3387
#3	0x008c4a60 in nsGenericHTMLElement::GetEditor at nsGenericHTMLElement.cpp:3375
#4	0x009019d0 in nsHTMLInputElement::GetEditor at nsHTMLInputElement.cpp:258
#5	0x012fce43 in nsHTMLTextFieldAccessible::GetAssociatedEditor at nsHTMLFormControlAccessible.cpp:582
#6	0x0131abec in nsHyperTextAccessible::CacheChildren at nsHyperTextAccessible.cpp:216
#7	0x012dc00d in nsAccessible::EnsureChildren at nsAccessible.cpp:3048
#8	0x012dc02b in nsAccessible::GetChildCount at nsAccessible.cpp:2947
#9	0x012ddafc in nsAccessible::GetChildCount at nsAccessible.cpp:633
#10	0x012b1145 in nsAccessNode::Init at nsAccessNode.cpp:194
#11	0x0131deb0 in nsAccessibleWrap::Init at nsAccessibleWrap.mm:69
#12	0x012eb429 in nsLinkableAccessible::Init at nsBaseWidgetAccessible.cpp:237
#13	0x012d1dfd in nsAccessibilityService::InitAccessible at nsAccessibilityService.cpp:1238
#14	0x012d54e6 in nsAccessibilityService::GetAccessible at nsAccessibilityService.cpp:1443
#15	0x012d1660 in nsAccessibilityService::GetAccessibleInWeakShell at nsAccessibilityService.cpp:1225
#16	0x012ded08 in nsAccessible::GetFirstAvailableAccessible at nsAccessible.cpp:3103
#17	0x01315600 in nsHyperTextAccessible::DOMPointToHypertextOffset at nsHyperTextAccessible.cpp:641
#18	0x012be0a9 in nsDocAccessible::CreateTextChangeEventForNode at nsDocAccessible.cpp:1536
#19	0x012bec4f in nsDocAccessible::InvalidateCacheSubtree at nsDocAccessible.cpp:2116
#20	0x012d1b64 in nsAccessibilityService::InvalidateSubtreeFor at nsAccessibilityService.cpp:2049
#21	0x007f8041 in nsGenericElement::doRemoveChildAt at nsGenericElement.cpp:3295
#22	0x007f841e in nsGenericElement::RemoveChildAt at nsGenericElement.cpp:3267
#23	0x005f4bfc in nsTextControlFrame::UpdateValueDisplay at nsTextControlFrame.cpp:2821
#24	0x005faa12 in nsTextControlFrame::EnsureEditorInitialized at nsTextControlFrame.cpp:1408
#25	0x005f1d0b in nsTextControlFrame::GetEditor at nsTextControlFrame.cpp:1923
#26	0x008c49e0 in nsGenericHTMLElement::GetEditorInternal at nsGenericHTMLElement.cpp:3387
#27	0x008c4ab0 in nsGenericHTMLFormElement::GetDesiredIMEState at nsGenericHTMLElement.cpp:2334
#28	0x008ac705 in nsIMEStateManager::GetNewIMEState at nsIMEStateManager.cpp:203
#29	0x008ae277 in nsIMEStateManager::OnChangeFocus at nsIMEStateManager.cpp:131
#30	0x00a847cb in nsFocusManager::Focus at nsFocusManager.cpp:1618
#31	0x00a85b35 in nsFocusManager::SetFocusInner at nsFocusManager.cpp:1126
#32	0x00a862de in nsFocusManager::SetFocus at nsFocusManager.cpp:413
#33	0x012e415c in nsAccessible::TakeFocus at nsAccessible.cpp:1241
#34	0x01439f0a in NS_InvokeByIndex_P at xptcinvoke_unixish_x86.cpp:179
#35	0x00195253 in XPCWrappedNative::CallMethod at xpcwrappednative.cpp:2727
#36	0x001a21ff in XPC_WN_CallMethod at xpcwrappednativejsops.cpp:1756
#37	0x0333a4bd in js_Invoke at jsinterp.cpp:1376
#38	0x03326345 in js_Interpret at jsops.cpp:2297
#39	0x0333a506 in js_Invoke at jsinterp.cpp:1384
#40	0x0333aabd in js_InternalInvoke at jsinterp.cpp:1439
#41	0x032a542b in JS_CallFunctionValue at jsapi.cpp:5120
#42	0x00a727d0 in nsJSContext::CallEventHandler at nsJSEnvironment.cpp:2168
#43	0x00aab73f in nsGlobalWindow::RunTimeout at nsGlobalWindow.cpp:8095
#44	0x00aabd06 in nsGlobalWindow::TimerCallback at nsGlobalWindow.cpp:8439
#45	0x0142601c in nsTimerImpl::Fire at nsTimerImpl.cpp:427
#46	0x01426254 in nsTimerEvent::Run at nsTimerImpl.cpp:519
#47	0x0141fa8a in nsThread::ProcessNextEvent at nsThread.cpp:527
#48	0x013af051 in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:200
#49	0x012904f3 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:125
#50	0x01244fd7 in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:510
#51	0x9578e8cb in __CFRunLoopDoSources0
#52	0x9578c38f in __CFRunLoopRun
#53	0x9578b864 in CFRunLoopRunSpecific
#54	0x9578b691 in CFRunLoopRunInMode
#55	0x97217f0c in RunCurrentEventLoopInMode
#56	0x97217cc3 in ReceiveNextEventCommon
#57	0x97217b48 in BlockUntilNextEventMatchingListInMode
#58	0x95cdeac5 in _DPSNextEvent
#59	0x95cde306 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
#60	0x95ca049f in -[NSApplication run]
#61	0x01243639 in nsAppShell::Run at nsAppShell.mm:863
#62	0x00fcb808 in nsAppStartup::Run at nsAppStartup.cpp:182
#63	0x00122158 in XRE_main at nsAppRunner.cpp:3476
#64	0x000025fd in main at nsBrowserApp.cpp:158

Comment 86

[(no longer active)]

Attachment: Lock nsTextControlFrame::EnsureEditorInitialized

This patch locks the nsTextControlFrame::EnsureEditorInitialized method against reentrancy.  I've confirmed that this fixes the a11y leaks, but it causes them to fail.  I'm not sure how to investigate the failures, because it requires internal knowledge of a11y and also seems to be accumulative (failure in one test leads to more failures in the rest of the test suite.)  Need to discuss this with David Bolter tomorrow.

Comment 87

[(no longer active)]

Attachment: Lock nsTextControlFrame::EnsureEditorInitialized

I just realized that I attached the wrong version of the locking patch.  Here's the correct version.

Comment 88

[(no longer active)]

Attachment: Patch (v2.5)

I merged the lock patch with the main one and made it assert in debug builds, so that we can catch reentrancy problems in debug builds.  The reason that I didn't leave it on for non-debug builds is that I'm not sure that there is any correct way of handling the reentrancy in a safe way -- we should just make sure that it doesn't happen at all.

A fix to bug 542364 would also help to enable us narrow down the number of possible reentrancy cases for the EnsureEditorInitialized function.

The a11y leak is being tracked in bug 542824.  Other than that, I think this patch is ready to land once it gets reviewed.  I would appreciate timely reviews here since it has the potential of rotting very soon.

Comment 89

[(no longer active)]

Comment on attachment 424115 [details] [diff] [review]
Patch (v2.5)

I'm trying to refactor this patch into a number of smaller patches.  I will file bugs for some of them which can land on their own and mark them as blocking this one.

Comment 90

[(no longer active)]

Attachment: Part 1 - Call EnsureEditorInitialized from all places where editor needs to exist

This should be applied on top of the patch in bug 542919.

This patch makes sure that EnsureEditorInitialized is called from every place where we need the editor to be initialized.

Comment 91

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

This patch toggles the initialization behavior for text and password input fields to be lazy.

Comment 92

[(no longer active)]

Attachment: Part 3 - Debug-only assertion for reentrancy detection

Make sure that the EnsureEditorInitialized function does not get called while another invocation is in progress further down the stack.  This patch doesn't have any effect on non-debug builds.

Comment 93

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

A comment change here.

Comment 94

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

Ah, I attached the patch without refreshing again... :(

Comment 95

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

There was an accessibility test failure with the previous version of Part 2.  We tracked down the failure to be caused by the fact that the a11y code saw a different native anonymous content tree for the input fields without an editor than those for which the editor was initialized.

Specifically, nsPlaintextEditor uses a text node under the anonymous div to store the value of the control, unless the value is empty, in which case it removes the node and replaces it with a bogus br node.

This version of Part 2 imitates this exact behavior for the cases where the editor is not yet initialized, so that the a11y module doesn't see anything different with the tree before and after the editor is initialized.

However, I believe that the mere fact that the a11y module tries to navigate the native anonymous content tree is a bad thing, and we should fix it.  We need to figure out what information does a11y need to support IAccessibleText, and provide an API for it to retrieve that information, so that it doesn't need to navigate the nodes under the native anonymous tree.

David Bolter said that he'll file a bug for adding the API that a11y needs, and posts the bug # here for future reference.

At this stage, all of the patches in this bug are final and ready for review.

Comment 96

[Boris Zbarsky [:bzbarsky]]

Man.  We really need to fix bug 240933...  At least for single-line inputs.  :(

Comment 97

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 424659 [details] [diff] [review]
Part 1 - Call EnsureEditorInitialized from all places where editor needs to exist

>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>+  PRBool NeedToInitializeEditorForEvent(nsEventChainPreVisitor& aVisitor) const;

Textarea will need this too at some point, right?  Can we just put it on nsGenericHTMLFormElement?  Or are we trying to avoid making that virtual GetType() call?

>+++ b/editor/libeditor/text/nsTextEditRules.cpp
>+  // Ensure that the selection has been initialized at this point

How about:

  // If the selection hasn't been set up yet, set it up collapsed to the end of
  // our editable content.

or something?

>+++ b/layout/forms/nsTextControlFrame.cpp
>+  nsresult rv = EnsureEditorInitialized();
>+  NS_ENSURE_SUCCESS(rv, rv);

So...  |this| might be dead after that call.  You need nsWeakFrame checks
around all callsites to EnsureEditorInitialized, I think.

> nsTextControlFrame::SelectAllContents()
> {
>+  nsresult rv = EnsureEditorInitialized();

Why write rv here?  You don't do anything with it afterward.

r=me with the above fixed.

Comment 98

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 424661 [details] [diff] [review]
Part 3 - Debug-only assertion for reentrancy detection

>+++ b/layout/forms/nsTextControlFrame.h
>+  friend class EditorInitializerLock;

This will fail with some compilers, because that class is not forward-declared.  Forward-declare it at the top of the file?

Is there a reason to not just keep track of a nesting depth counter (incremented/decremented in an RAII way like your boolean) and then assert if it's nonzero on entry?  Either that, or rename this thing to EditorInitializerEntryTracker or something.  "lock" suggests that there's some sort of actual locking involved, which there isn't.

Comment 99

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425470 [details] [diff] [review]
Part 2 - Make the editor initialization lazy (initialize only when needed)

>+++ b/layout/forms/nsTextControlFrame.cpp
>+  // Make sure we clear out the non-breaking space before we initialize the editor
>+  UpdateValueDisplay(PR_FALSE, PR_TRUE);

As I asked on irc, is this still needed?

>@@ -2704,16 +2720,22 @@ nsTextControlFrame::SetValue(const nsASt
>+    // The only time mEditor is non-null but mUseEditor is false is when the
>+    // frame is being torn down.  If that's what's going on, don't bother with
>+    // updating the display.
>+    if (!mEditor) {
>+      UpdateValueDisplay(PR_TRUE);

So this will get the value from the node, even though we have it here, right? Is it worth refactoring somehow to avoid that?

>+nsTextControlFrame::UpdateValueDisplay(PRBool aNotify,
>+      NS_ASSERTION(0, "Invalid child node found");

NS_ERROR, please.  Or NS_NOTREACHED?

>+  nsAutoString defaultValue;

Maybe just |value|?  It's not really a defaultValue.

>+      mAnonymousDiv->RemoveChildAt(0, PR_TRUE, PR_TRUE);
>+      mAnonymousDiv->AppendChildTo(brNode, PR_TRUE);

Shouldn't those use aNotify?  And do we really need mutation events here?

>+      mAnonymousDiv->RemoveChildAt(0, PR_TRUE, PR_TRUE);
>+      mAnonymousDiv->AppendChildTo(textNode, PR_TRUE);

And here.

Should there be some asserts that mAnonymousDiv has no more than 1 kid?

>+++ b/layout/forms/nsTextControlFrame.h
>+   * Update the textnode under our anonymous div to show the new
>+   * value. This should pnly be called when we have no editor yet.

s/pnly/only/

>+   * @throws NS_ERROR_UNEXPECTED is the div has no text content

s/is/if/

I know the curent style of this file is no blank lines between the previous method and the "/**" but that style is silly.  Put in some blank lines, please.

r=bzbarsky with the above issues addressed.

Comment 100

[(no longer active)]

Attachment: Part 1 - Call EnsureEditorInitialized from all places where editor needs to exist [Checkin: comment 121]

(In reply to comment #97)
> (From update of attachment 424659 [details] [diff] [review])
> >+++ b/content/html/content/src/nsHTMLInputElement.cpp
> >+  PRBool NeedToInitializeEditorForEvent(nsEventChainPreVisitor& aVisitor) const;
> 
> Textarea will need this too at some point, right?  Can we just put it on
> nsGenericHTMLFormElement?  Or are we trying to avoid making that virtual
> GetType() call?

I can move this method into the nsTextEditorState class once it's materialized.  For now, I prefer to leave this like this (and move it in a refactoring patch as part of the work on lazy textarea init.)

> >+++ b/layout/forms/nsTextControlFrame.cpp
> >+  nsresult rv = EnsureEditorInitialized();
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> So...  |this| might be dead after that call.  You need nsWeakFrame checks
> around all callsites to EnsureEditorInitialized, I think.

As discussed on IRC, this won't be an issue when bug 534785 gets fixed.

Comment 101

[(no longer active)]

Attachment: Part 3 - Debug-only assertion for reentrancy detection

(In reply to comment #98)
> (From update of attachment 424661 [details] [diff] [review])
> >+++ b/layout/forms/nsTextControlFrame.h
> >+  friend class EditorInitializerLock;
> 
> This will fail with some compilers, because that class is not forward-declared.
>  Forward-declare it at the top of the file?

Done.

> Is there a reason to not just keep track of a nesting depth counter
> (incremented/decremented in an RAII way like your boolean) and then assert if
> it's nonzero on entry?  Either that, or rename this thing to
> EditorInitializerEntryTracker or something.  "lock" suggests that there's some
> sort of actual locking involved, which there isn't.

We really don't need to track the depth number, so I just renamed things.

Comment 102

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

(In reply to comment #99)
> (From update of attachment 425470 [details] [diff] [review])
> >+++ b/layout/forms/nsTextControlFrame.cpp
> >+  // Make sure we clear out the non-breaking space before we initialize the editor
> >+  UpdateValueDisplay(PR_FALSE, PR_TRUE);
> 
> As I asked on irc, is this still needed?

I actually tried that, and it seems essential in fact!  I think the plain text editor is not good about handling the existing content under the editor correctly, and much less so with the presence of a bogus node there as well.

> >@@ -2704,16 +2720,22 @@ nsTextControlFrame::SetValue(const nsASt
> >+    // The only time mEditor is non-null but mUseEditor is false is when the
> >+    // frame is being torn down.  If that's what's going on, don't bother with
> >+    // updating the display.
> >+    if (!mEditor) {
> >+      UpdateValueDisplay(PR_TRUE);
> 
> So this will get the value from the node, even though we have it here, right?
> Is it worth refactoring somehow to avoid that?

Yes, I guess so.  I did that in the new version of the patch.

> >+nsTextControlFrame::UpdateValueDisplay(PRBool aNotify,
> >+      NS_ASSERTION(0, "Invalid child node found");
> 
> NS_ERROR, please.  Or NS_NOTREACHED?

Done.

> >+  nsAutoString defaultValue;
> 
> Maybe just |value|?  It's not really a defaultValue.

Agreed.

> >+      mAnonymousDiv->RemoveChildAt(0, PR_TRUE, PR_TRUE);
> >+      mAnonymousDiv->AppendChildTo(brNode, PR_TRUE);
> 
> Shouldn't those use aNotify?  And do we really need mutation events here?
> 
> >+      mAnonymousDiv->RemoveChildAt(0, PR_TRUE, PR_TRUE);
> >+      mAnonymousDiv->AppendChildTo(textNode, PR_TRUE);
> 
> And here.

Fixed.

> Should there be some asserts that mAnonymousDiv has no more than 1 kid?

Yes, there should.  I added one to UpdateValueDisplay.

There are still a bunch of test failures for the test added in bug 518122.  I'm investigating those issues.

Comment 103

[(no longer active)]

Attachment: Part 4 - Fix the newline handling for text fields without an editor

The test failures indeed revealed a bug in the newline handling for text fields without an editor initialized.  This patch fixes the problem.

Comment 104

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426606 [details] [diff] [review]
Part 4 - Fix the newline handling for text fields without an editor

Hmm.  Why are the nsHTMLInputElement changese needed, exactly?

And can we use '\r' and '\n' instead of 10 and 13?

Comment 105

[(no longer active)]

Attachment: Part 4 - Fix the newline handling for text fields without an editor

(In reply to comment #104)
> (From update of attachment 426606 [details] [diff] [review])
> Hmm.  Why are the nsHTMLInputElement changese needed, exactly?

With the eager initialized editor, when setting the value on the editor, PlatformToDOMLineBreaks is called in order to store the value with \n line endings instead of \r or \r\n line endings, so that the editor wouldn't have to deal with carriage returns later on.  So the last hunk in that file is needed to make sure that the editor will see the value in the form that it expects when being initialized.

When retrieving the value from the editor, the new lines in that value have already been handled by the editor according to the platform prefs using nsTextEditRules::HandleNewLines.  The code path in the second hunk of that file is executed when retrieving the value in case the editor is not yet initialized.  There, we make sure that we return the value with converted new lines exactly like it would have been returned if there was actually an editor behind the scenes.

> And can we use '\r' and '\n' instead of 10 and 13?

Sure.

Comment 106

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426633 [details] [diff] [review]
Part 4 - Fix the newline handling for text fields without an editor

So we used to actually do different things with values depending on whether the node had a frame?  It might be worth writing a test for that...

This patch looks ok, but this need to HandleNewLines on each GetValue is unfortunate.  Can we not simply make sure mValue has the right thing going on with linebreaks on set?  That's when editor normalized the linebreaks, right?

In particular, does your new patch do the same thing as the old code if I have a text input, set its value to something with newlines in it, then change the type to checkbox or something, and then examine the value?

r=bzbarsky modulo the above question

Comment 107

[(no longer active)]

(In reply to comment #106)
> (From update of attachment 426633 [details] [diff] [review])
> So we used to actually do different things with values depending on whether the
> node had a frame?  It might be worth writing a test for that...

Will do.

> This patch looks ok, but this need to HandleNewLines on each GetValue is
> unfortunate.  Can we not simply make sure mValue has the right thing going on
> with linebreaks on set?  That's when editor normalized the linebreaks, right?

We can't really do that in a clean way, since the control's internal value doesn't really change -- it's only the editor which eats it before sets it as the text in the textnode.

> In particular, does your new patch do the same thing as the old code if I have
> a text input, set its value to something with newlines in it, then change the
> type to checkbox or something, and then examine the value?

Yes, except the fact that it fixes the bug where the value would be the default value if there is a text frame.

Comment 108

[Boris Zbarsky [:bzbarsky]]

> since the control's internal value doesn't really change

Well... it used to before, no, since the editor was where the value lived?

Comment 109

[(no longer active)]

Attachment: Part 5 - Make sure that the behavior of the value property is well-defined for all cases

This adds a test for comment 106.  It tries to ensure that setting the value property has a uniform behavior for initialized, non-initialized, and frame-less input controls.  The test examines the case where there's a newline in the value to ensure the proper handling of the newline throughout the way.

Asking sicking's review as well to make sure that this is what we actually want.

Comment 110

[(no longer active)]

(In reply to comment #108)
> > since the control's internal value doesn't really change
> 
> Well... it used to before, no, since the editor was where the value lived?

Only when the editor actually existed.  Let me demonstrate.  This test case shows a newline in the third alert before my patches, which happens because the frame (and the editor) have been destroyed, but with my patch, none of the alerts show a newline, which is consistent behavior.

data:text/html,<input id=x type=text><script>var x=document.getElementById('x');x.value="hello\nworld";alert(x.value);x.style.display="none";alert(x.value);x.value="test\nvalue";alert(x.value);</script>

Comment 111

[Boris Zbarsky [:bzbarsky]]

> Only when the editor actually existed.

Sure.  And the new code should make it look like it happens always, right?  So why not actually change the value once of set instead of doing extra work on every value get?  Maybe I'm just missing the reason that can't be done?

Comment 112

[(no longer active)]

(In reply to comment #111)
> > Only when the editor actually existed.
> 
> Sure.  And the new code should make it look like it happens always, right?  So
> why not actually change the value once of set instead of doing extra work on
> every value get?  Maybe I'm just missing the reason that can't be done?

Well, currently this happens on nsTextControlFrame::GetText.  Doing that when setting the value would cause things like this to break:

data:text/html,<input id=x type=checkbox><script>var x=document.getElementById("x");x.value="a\nb";x.type="text";x.type="checkbox";alert(x.value);</script>

And it breaks a bunch of other things (I remember that it caused test failures -- need to make a temp patch to does it and submit to try if you want to see the full list of failing tests.)

Comment 113

[Boris Zbarsky [:bzbarsky]]

It still seems like filtering only when setting value on a _text_ input or when changing type to text would work...  But then changing type twice wouldn't roundtrip the value, right?  Then again, isn't that broken anyway, when an editor is around?

Comment 114

[(no longer active)]

(In reply to comment #113)
> It still seems like filtering only when setting value on a _text_ input or when
> changing type to text would work...  But then changing type twice wouldn't
> roundtrip the value, right?

Yes, because we would be throwing the original value away.

>  Then again, isn't that broken anyway, when an
> editor is around?

No, not with my current patch.  Specifically, the test case in comment 112 alerts a\nb as expected with my patch.

Comment 115

[(no longer active)]

(In reply to comment #114)
> No, not with my current patch.  Specifically, the test case in comment 112
> alerts a\nb as expected with my patch.

And it works the same way if you do:

data:text/html,<input id=x type=checkbox><script>var x=document.getElementById("x");x.value="a\nb";x.type="text";x.focus();x.type="checkbox";alert(x.value);</script>

and hence initialize the editor when the input's type is text.

Comment 116

[Boris Zbarsky [:bzbarsky]]

So what in your patch prevents the dataloss due to round-tripping through the editor?

Comment 117

[(no longer active)]

(In reply to comment #116)
> So what in your patch prevents the dataloss due to round-tripping through the
> editor?

It makes this block actually work <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#671> (and mValue has the correct value there.)

Comment 118

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  That makes sense.

Comment 119

[(no longer active)]

(In reply to comment #118)
> Ah, I see.  That makes sense.

Does this imply an r+?  ;-)

Comment 120

[Boris Zbarsky [:bzbarsky]]

> Does this imply an r+?  ;-)

Yes, it does.

Comment 121

[(no longer active)]

Part 1 landed as <http://hg.mozilla.org/mozilla-central/rev/70b1ccb14325>.

Comment 122

[(no longer active)]

(In reply to comment #121)
> Part 1 landed as <http://hg.mozilla.org/mozilla-central/rev/70b1ccb14325>.

I backed this out because I suspect that this might have caused a crash on Windows mochitests:

http://hg.mozilla.org/mozilla-central/rev/03a61f4254a3

Failure log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1266532728.1266535683.5721.gz&fulltext=1#err0

Comment 123

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 427204 [details] [diff] [review]
Part 5 - Make sure that the behavior of the value property is well-defined for all cases

>+    is(input.getAttribute("value"), newValue, "The default value for the input control should change as well");

This test seems wrong. We shouldn't be changing the "value" markup attribute when the user sets .type, should we?

Or does the HTML5 spec define this? If so, where?

Comment 124

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 427204 [details] [diff] [review]
Part 5 - Make sure that the behavior of the value property is well-defined for all cases

Unsetting review request until questions in previous comment are answered.

What I think should happen when .type is changed from (for example) "text" to "checkbox", is just that .value changes from mapping to an internal mValue, to mapping to (s/g)etAttribute("value").

We should not copy the internal mValue to getAttribute("value").

In other words, when setting .type, I think that can change .value.

Comment 125

[(no longer active)]

(In reply to comment #124)
> (From update of attachment 427204 [details] [diff] [review])
> Unsetting review request until questions in previous comment are answered.
> 
> What I think should happen when .type is changed from (for example) "text" to
> "checkbox", is just that .value changes from mapping to an internal mValue, to
> mapping to (s/g)etAttribute("value").
> 
> We should not copy the internal mValue to getAttribute("value").
> 
> In other words, when setting .type, I think that can change .value.

Apparently HTML5 specifies what should happen here.  Please see bug 549475 for more info.

Comment 126

[(no longer active)]

Attachment: Part 2 - Make the editor initialization lazy (initialize only when needed)

This patch has a slight change to make sure that we never call nsTextControlFrame::UpdateValueDisplay on textarea's.

This revision of my patch queue shows the change:
http://hg.mozilla.org/users/ehsan.akhgari_gmail.com/mq/rev/16d8fad31125

Comment 127

[(no longer active)]

Attachment: Part 5 - Make lazy editor initialization compatible with placeholder values

This patch makes sure that lazy initialization of input fields plays nicely with placeholder values.

Comment 128

[(no longer active)]

Attachment: Part 6 - Fix a failing test

This simple patch makes a test pass.  The failure was happening because the frame (and editor) were being destructed when the overflow:hidden element was blurred, and later when it was recreated, the editor was not being initialized, so the controllers for that field lacked the editor as their command context.

The focus() call just triggers reconstruction of the editor, to fix the context for the command controller.  This fix is correct because in practice, the user can't invoke the editor commands on a textbox unless it is focused.

Comment 129

[(no longer active)]

Attachment: Part 7 - Wrap the calls to nsTextControlFrame::EnsureEditorInitialized in a weak frame check

This patch wrap all of the calls to nsTextControlFrame::EnsureEditorInitialized in a weak frame check, to make sure that we handle the cases where the frame might be destroyed as part of the EnsureEditorInitialized operations.

This is only a temporary measure in order to make it possible for us to land this without bug 534785.  I'll attach a patch to that bug as the first part of the patches in that bug to back this patch out.  That would land as part of the landing of bug 534785.

With this patch, all the unit test failures have been addressed, and this bug is ready to land once the remaining patches get reviewed!

Comment 130

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436602 [details] [diff] [review]
Part 2 - Make the editor initialization lazy (initialize only when needed)

s/textarea's/textareas/ and looks good.

Comment 131

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436605 [details] [diff] [review]
Part 5 - Make lazy editor initialization compatible with placeholder values

Why is the CreateAnonymousContent change here needed?

Comment 132

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436608 [details] [diff] [review]
Part 6 - Fix a failing test

r=bzbarsky.  I assume the commit comment here (and on other patches in this bug) will be updated to include bug # and so forth at some point?

Comment 133

[(no longer active)]

(In reply to comment #131)
> (From update of attachment 436605 [details] [diff] [review])
> Why is the CreateAnonymousContent change here needed?

Because by the time that nsTextControlFrame::UpdateValueDisplay is called, the anonymous div for the placeholder value should exist, otherwise we can't update the display of the placeholder.

Comment 134

[(no longer active)]

(In reply to comment #132)
> (From update of attachment 436608 [details] [diff] [review])
> r=bzbarsky.  I assume the commit comment here (and on other patches in this
> bug) will be updated to include bug # and so forth at some point?

Yes!  Those commit messages are only notes to myself stating what each patch does, and will change entirely when landing!

Comment 135

[Boris Zbarsky [:bzbarsky]]

> Because by the time that nsTextControlFrame::UpdateValueDisplay is called

Ah, I see.  Can UpdateValueDisplay be called when the control has focus?

Comment 136

[(no longer active)]

(In reply to comment #135)
> > Because by the time that nsTextControlFrame::UpdateValueDisplay is called
> 
> Ah, I see.  Can UpdateValueDisplay be called when the control has focus?

No.  If the control is focused, the editor is initialized for it, and UpdateValueDisplay will no longer be necessary, so it won't be called.

Comment 137

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 436605 [details] [diff] [review]
Part 5 - Make lazy editor initialization compatible with placeholder values

r=bzbarsky

Comment 138

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0a2f12f04244
http://hg.mozilla.org/mozilla-central/rev/9cb9891e26c0
http://hg.mozilla.org/mozilla-central/rev/ab4b5f314d06
http://hg.mozilla.org/mozilla-central/rev/10eaa55f5e3d
http://hg.mozilla.org/mozilla-central/rev/26ecdf01c65a
http://hg.mozilla.org/mozilla-central/rev/ec0aae9c570d
http://hg.mozilla.org/mozilla-central/rev/40038cc9f245

Comment 139

[(no longer active)]

Backed out because of the random orange of bug 557689:

http://hg.mozilla.org/mozilla-central/rev/309acf8ab20f

Comment 140

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a7ba85888b19
http://hg.mozilla.org/mozilla-central/rev/5e906315f519
http://hg.mozilla.org/mozilla-central/rev/20d96fa137dd
http://hg.mozilla.org/mozilla-central/rev/d1f52a5b7d45
http://hg.mozilla.org/mozilla-central/rev/4d3458388aaf
http://hg.mozilla.org/mozilla-central/rev/4d3458388aaf
http://hg.mozilla.org/mozilla-central/rev/d11bd430a5a1
http://hg.mozilla.org/mozilla-central/rev/c0e83dac1a73