Loading this testcase makes a debug build of Firefox crash: * dereferencing 0xdddddddd from nsTextControlFrame::SetValue or * dereferencing something close to zero from nsTextInputSelectionImpl::ScrollSelectionIntoView

Any idea whether this is a regression, or just a bug?

Regressed between 2007-03-04_1651 and 2007-03-04_1750. Same regression range as bug 373122.

hmm, keeping pointers to scrollable view in nsIFrame, nsFrameSelection objects etc. isn't that good idea.

Actually this bug can be fixed with a flush. (I'll file a new bug to clean up scrollable view usage.)

Uh... why is the flush needed? What goes wrong without it? We want to be _removing_ flushes except as needed for correctness, not adding them to fix glitches...

Without flush SetValue in nsTextContentFrame will call editor's InsertText, which may cause a flush, and then the nsTextContentFrame object gets possibly deleted...

> which may cause a flush With what stack? And isn't that a problem that the frame should handle? I don't think we want content to have to worry about when exactly frames perform unsafe operations... it's fragile.

Well, IMO, nsTextControlFrame shouldn't own the editor, which can do lots of unsafe operations. Will upload the stack asap...

> nsTextControlFrame shouldn't own the editor Also possible, yes.

An uglier way to fix this is to remove nsIScrollableView members, have a kungfudeathgrip for editor and add few weakframe.IsAlive() tests to SetValue. So comparing to that the patch seems quite a bit nicer, but better ideas are of course very welcome :)

I'd prefer the weakframe and kungfudeathgrip to flushes... As for the stack, that's truly unfortunate. :( That's a problem for textcontrolframe init too, no?

Using strong refs, weakFrames and making sure that scroll view is always the right one (this includes the patch for bug 373674). That way the crash is fixed. Also to fix potential crashes related to InitEditor, instead of using nsIAnonymousContentCreator::PostCreateFrames nsIReflowCallback::ReflowFinished is used now. (We must not flush while creating frames for textcontrolframe.) ... hoping reflow callback works well enough here :)

> instead of using nsIAnonymousContentCreator::PostCreateFrames > nsIReflowCallback::ReflowFinished is used now. I'm not sure you can do that, actually. Testcases that involve inserting the node and then setting its value (before reflow happens) worry me. As well as various permutations thereof. In any case, for 1.9 I'm hoping to make editor init fully async so I wouldn't worry about it here too much... and for branches we need a heck of a lot of testing for that change. Note that I won't be in a position to review anything until April, basically.

Before calling InitEditor nsHTMLTextArea/InputElements handles the |value|. So things should be ok, but this definitely needs testing. I don't really know what else could be used than reflow callback.

Comment on attachment 258321 [details] [diff] [review] possible patch So this wouldn't probably be enough for the potential crash in InitEditor().

Like I said, I'm happy to handle InitEditor in a separate bug.

Roc, maybe you have time to look at this. (InitEditor is still a hypothetical place for a crash anyway.)

Comment on attachment 258360 [details] [diff] [review] no handling for InitEditor + nsCOMPtr<nsIPlaintextEditor> htmlEditor = do_QueryInterface(editor); Could you fix this misnamed variable while you're at it?

This caused one chrome test to fail.

I checked in this patch with the simple fix.

Since this is fixed and doesn't affect shipping releases we should be able to unhide this bug now, right?

This crashes current branch builds when clicking on the button. You need to run the testcase from chrome. I get a [@ nsTextControlFrame::SetValue] (talkback ID: TB32917810X). It doesn't crash on trunk, I think it's because of this patch. I think this is currently nr. 29 of the hit list of crashes on branch: http://talkback-public.mozilla.org/reports/firefox/FF2004/FF2004-topcrashers.html

Martijn please file a separate bug on comment 23, marked dependent on this bug.

Filed as bug 386254, sorry for the delay.

Crashtest checked in.

$ hg log layout/forms/crashtests/373586-1.xhtml changeset: 9399:825d223e8ce4 user: jruderman@hmc.edu date: Mon Dec 17 23:19:21 2007 -0800 summary: Add crashtest

martijn, is it necessary to keep testcase 2 private?

Comment on attachment 267577 [details] testcase2 No, not necessary anymore.

Do we want to land it as a crashtest too, then? Similarly, what about comment 23, comment 24, comment 25? Please unmark any relevant privacy flags, including the one on this comment if those are OK to open.

bz: I'm planning on landing it after I land a number of others I've been working on, but since it requires UniversalXPConnect, I'll set it to skip in the manifest. That is also what I'm planning on doing with several other tests which require elevated privileges. Does that sound ok?

Yes, if your manifest includes a comment pointing to the bug that would enable reenabling the test. Alternately, we could land these as mochitests....