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

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Jesse Ruderman]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Actually this bug can be fixed with a flush.

(I'll file a new bug to clean up scrollable view usage.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

> nsTextControlFrame shouldn't own the editor

Also possible, yes.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a stack

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: alternative patch

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

Comment 14

[Boris Zbarsky [: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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 258321 [details] [diff] [review]
possible patch

So this wouldn't probably be enough for the potential crash in InitEditor().

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: no handling for InitEditor

Roc, maybe you have time to look at this.

(InitEditor is still a hypothetical place for a crash anyway.)

Comment 19

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

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?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

This caused one chrome test to fail. 

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: mScrollableViewProvider = nsnull from Init to constructor

I checked in this patch with the simple fix.

Comment 22

[Daniel Veditz [:dveditz]]

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

Comment 23

[Martijn Wargers (dead)]

Attachment: 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

Comment 24

[Daniel Veditz [:dveditz]]

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

Comment 25

[Martijn Wargers (dead)]

Filed as bug 386254, sorry for the delay.

Comment 26

[Jesse Ruderman]

Crashtest checked in.

Comment 27

[Bob Clary [:bc] (inactive)]

$ hg log layout/forms/crashtests/373586-1.xhtml
changeset:   9399:825d223e8ce4
user:        [e-mail]
date:        Mon Dec 17 23:19:21 2007 -0800
summary:     Add crashtest

Comment 28

[Bob Clary [:bc] (inactive)]

martijn, is it necessary to keep testcase 2 private?

Comment 29

[Martijn Wargers (dead)]

Comment on attachment 267577 [details]
testcase2

No, not necessary anymore.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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.

Comment 31

[Bob Clary [:bc] (inactive)]

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?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Yes, if your manifest includes a comment pointing to the bug that would enable reenabling the test.

Alternately, we could land these as mochitests....