Closed Bug 373586 Opened 14 years ago Closed 14 years ago

Crash [@ nsTextInputSelectionImpl::ScrollSelectionIntoView] [@ nsTextControlFrame::SetValue] with XBL, textarea

Categories

(Core :: XBL, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical?] post 1.8 branch)

Crash Data

Attachments

(6 files, 1 obsolete file)

Loading this testcase makes a debug build of Firefox crash:
* dereferencing 0xdddddddd from nsTextControlFrame::SetValue
or
* dereferencing something close to zero from nsTextInputSelectionImpl::ScrollSelectionIntoView
Flags: blocking1.9?
Whiteboard: [sg:critical?]
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.
Blocks: 363253
Keywords: regression
OS: Mac OS X → All
hmm, keeping pointers to scrollable view in nsIFrame, nsFrameSelection 
objects etc. isn't that good idea.
Assignee: general → Olli.Pettay
Attached patch possible patchSplinter Review
Actually this bug can be fixed with a flush.

(I'll file a new bug to clean up scrollable view usage.)
Attachment #258321 - Flags: superreview?(roc)
Attachment #258321 - Flags: review?(roc)
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.
Attached file a stack
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?
Attached patch alternative patch (obsolete) — Splinter Review
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 :)
Attachment #258344 - Flags: review?(bzbarsky)
> 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().
Attachment #258321 - Flags: superreview?(roc)
Attachment #258321 - Flags: review?(roc)
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.)
Attachment #258344 - Attachment is obsolete: true
Attachment #258360 - Flags: review?(roc)
Attachment #258344 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
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?
Attachment #258360 - Flags: superreview+
Attachment #258360 - Flags: review?(roc)
Attachment #258360 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This caused one chrome test to fail. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I checked in this patch with the simple fix.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Since this is fixed and doesn't affect shipping releases we should be able to unhide this bug now, right?
Whiteboard: [sg:critical?] → [sg:critical?] post 1.8 branch
Flags: blocking1.9?
Group: security
Flags: wanted1.8.1.x-
Attached file testcase2
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.
Attachment #267577 - Attachment is private: true
Blocks: 386254
Filed as bug 386254, sorry for the delay.
Crashtest checked in.
Flags: in-testsuite+
$ 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.
Attachment #267577 - Attachment is private: false
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....
Crash Signature: [@ nsTextInputSelectionImpl::ScrollSelectionIntoView] [@ nsTextControlFrame::SetValue]
You need to log in before you can comment on or make changes to this bug.