Last Comment Bug 748216 - Avoid dynamic allocation of nsHTMLTextAreaElement::mState
: Avoid dynamic allocation of nsHTMLTextAreaElement::mState
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: :Ehsan Akhgari (out sick)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-23 21:17 PDT by :Ehsan Akhgari (out sick)
Modified: 2012-04-26 10:43 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (8.68 KB, patch)
2012-04-24 09:09 PDT, :Ehsan Akhgari (out sick)
no flags Details | Diff | Review
Patch (v1) (12.51 KB, patch)
2012-04-24 10:36 PDT, :Ehsan Akhgari (out sick)
bugs: review+
Details | Diff | Review

Description :Ehsan Akhgari (out sick) 2012-04-23 21:17:28 PDT
We currently allocate mState dynamically.  This is stupid, since this object will *always* need to have the mState initialized properly, and it's inefficient.

The reason that I did this was that I did not know how to traverse mState for cycle collection as a simple member variable.  Here's the related code: <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#359>

The reason I did this was that I did not know how to properly traverse this member if it's not a pointer.  Doing the obvious (see below) seems to lead to cycle collector faults:

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_PTR(&tmp->mState, nsTextEditorState, "mState")

Here's the stack for a sample cycle collector fault:

Fault in cycle collector: zero refcount (ptr: 0x127070ff0)
###!!! ABORT: cycle collector fault: file /Users/ehsanakhgari/moz/mozilla-central/xpcom/base/nsCycleCollector.cpp, line 1252
Fault(char const*, void const*)+0x00000079 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F82339]
Fault(char const*, PtrInfo*)+0x00000020 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F82070]
GCGraphBuilder::DescribeRefCountedNode(unsigned int, unsigned long, char const*)+0x00000043 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F82383]
nsTextEditorState::cycleCollection::Traverse(void*, nsCycleCollectionTraversalCallback&)+0x00000058 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x00B135D8]
GCGraphBuilder::Traverse(PtrInfo*)+0x0000007A [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F81FFA]
nsCycleCollector::MarkRoots(GCGraphBuilder&)+0x00000079 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F83079]
nsCycleCollector::BeginCollection(nsICycleCollectorListener*)+0x0000019A [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F8439A]
nsCycleCollectorRunner::Run()+0x00000147 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F89A47]
nsThread::ProcessNextEvent(bool, bool*)+0x00000558 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F64A88]
NS_ProcessNextEvent_P(nsIThread*, bool)+0x0000009F [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01ED613F]
nsThread::ThreadFunc(void*)+0x00000127 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x01F63727]
_pt_root+0x00000163 [/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/libnspr4.dylib +0x00038B93]
_pthread_start+0x0000014B [/usr/lib/libSystem.B.dylib +0x00039FD6]
###!!! ABORT: cycle collector fault: file /Users/ehsanakhgari/moz/mozilla-central/xpcom/base/nsCycleCollector.cpp, line 1252

Can someone please tell me how to do this properly?
Comment 1 Andrew McCreight [:mccr8] 2012-04-23 22:00:10 PDT
It looks like the nsTextEditorState is reporting to the CC that its ref count is 0.  It gets its refcount like this: |tmp->mRefCnt.get()|.  Are you failing to add ref mState somehow?  What are you changing its type to?
Comment 2 Andrew McCreight [:mccr8] 2012-04-23 22:03:53 PDT
Ah, right, you are putting it inline.  That is a little weird.  I'm not sure what the right way to deal with that is.  Maybe you can manually addref it?
Comment 3 :Ehsan Akhgari (out sick) 2012-04-23 22:12:28 PDT
nsHTMLTextAreaElement holds the only reference to nsTextEditorState.  Kyle told me on IRC that I should add a Traverse method to nsTextEditorState (like nsINode::Traverse) and just call that directly.  That requires me to not be sleepy, so I'll try that tomorrow.
Comment 4 Andrew McCreight [:mccr8] 2012-04-23 22:17:13 PDT
Ah, yes, that sounds right.  nsBidiUtils or something like that does that.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-24 02:28:35 PDT
Don't use refcounted classes as inline or stack variables.
Comment 6 :Ehsan Akhgari (out sick) 2012-04-24 09:09:54 PDT
Created attachment 617892 [details] [diff] [review]
WIP

This is the patch I have so far, with several failed attempts at getting Traverse right commented out.  Nothing has worked for me so far.
Comment 7 Andrew McCreight [:mccr8] 2012-04-24 09:19:28 PDT
What you need to do is define a new method nsTextEditorState::Traverse( nsCycleCollectionTraversalCallback &cb).  The body of this function is the same as the current CC traverse function, which you should make invoke the new Traverse function.  The problem you are having is that the CC traverse function invokes a callback describing the object, but you don't want to do that, you just want to report the children.
Comment 8 Andrew McCreight [:mccr8] 2012-04-24 09:19:57 PDT
(then call this new method from nsHTMLTextAreaElement's CC traverse function)
Comment 9 :Ehsan Akhgari (out sick) 2012-04-24 10:36:10 PDT
Created attachment 617934 [details] [diff] [review]
Patch (v1)
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-04-24 10:43:56 PDT
Comment on attachment 617934 [details] [diff] [review]
Patch (v1)

>commit f6b98bcb1e8400150e9f3512b8500e5bcd2b5531
>Author: Ehsan Akhgari <ehsan.akhgari@gmail.com>
>Date:   Tue Apr 24 00:19:26 2012 -0400
>
>    Bug 748216 - Avoid dynamic allocation of nsHTMLTextAreaElement::mState
>
>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>index 843b9c0..c223c9e 100644
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -573,8 +573,7 @@ nsHTMLInputElement::nsHTMLInputElement(already_AddRefed<nsINodeInfo> aNodeInfo,
>   , mCanShowInvalidUI(true)
> {
>   mInputData.mState = new nsTextEditorState(this);
>-  NS_ADDREF(mInputData.mState);
>-  
>+
>   if (!gUploadLastDir)
>     nsHTMLInputElement::InitUploadLastDir();
> 
>@@ -605,7 +604,8 @@ nsHTMLInputElement::FreeData()
>     mInputData.mValue = nsnull;
>   } else {
>     UnbindFromFrame(nsnull);
>-    NS_IF_RELEASE(mInputData.mState);
>+    delete mInputData.mState;
>+    mInputData.mState = nsnull;
>   }
> }
> 
>@@ -630,7 +630,7 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsHTMLInputElement,
>                                                   nsGenericHTMLFormElement)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mControllers)
>   if (tmp->IsSingleLineTextControl(false)) {
>-    NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mInputData.mState, nsTextEditorState)
>+    tmp->mInputData.mState->Traverse(cb);
>   }
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMARRAY(mFiles)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFileList)
>@@ -644,6 +644,9 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsHTMLInputElement,
>     tmp->mFileList->Disconnect();
>     tmp->mFileList = nsnull;
>   }
>+  if (tmp->IsSingleLineTextControl(false)) {
>+    tmp->mInputData.mState->Unlink();
>+  }
>   //XXX should unlink more?
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>                                                               
>@@ -2396,7 +2399,6 @@ nsHTMLInputElement::HandleTypeChange(PRUint8 aNewType)
>   if (isNewTypeSingleLine && !isCurrentTypeSingleLine) {
>     FreeData();
>     mInputData.mState = new nsTextEditorState(this);
>-    NS_ADDREF(mInputData.mState);
>   } else if (isCurrentTypeSingleLine && !isNewTypeSingleLine) {
>     FreeData();
>   }
>diff --git a/content/html/content/src/nsHTMLTextAreaElement.cpp b/content/html/content/src/nsHTMLTextAreaElement.cpp
>index 39157e4..76505af 100644
>--- a/content/html/content/src/nsHTMLTextAreaElement.cpp
>+++ b/content/html/content/src/nsHTMLTextAreaElement.cpp
>@@ -259,7 +259,7 @@ protected:
>   bool                     mCanShowValidUI;
> 
>   /** The state of the text editor (selection controller and the editor) **/
>-  nsRefPtr<nsTextEditorState> mState;
>+  nsTextEditorState mState;
> 
>   NS_IMETHOD SelectAll(nsPresContext* aPresContext);
>   /**
>@@ -333,7 +333,7 @@ nsHTMLTextAreaElement::nsHTMLTextAreaElement(already_AddRefed<nsINodeInfo> aNode
>     mDisabledChanged(false),
>     mCanShowInvalidUI(true),
>     mCanShowValidUI(true),
>-    mState(new nsTextEditorState(this))
>+    mState(this)
> {
>   AddMutationObserver(this);
> 
>@@ -352,11 +352,12 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(nsHTMLTextAreaElement)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsHTMLTextAreaElement,
>                                                 nsGenericHTMLFormElement)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mControllers)
>+  tmp->mState.Unlink();
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsHTMLTextAreaElement,
>                                                   nsGenericHTMLFormElement)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mControllers)
>-  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_MEMBER(mState, nsTextEditorState)
>+  tmp->mState.Traverse(cb);
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> NS_IMPL_ADDREF_INHERITED(nsHTMLTextAreaElement, nsGenericElement) 
>@@ -498,76 +499,76 @@ nsHTMLTextAreaElement::GetValue(nsAString& aValue)
> void
> nsHTMLTextAreaElement::GetValueInternal(nsAString& aValue, bool aIgnoreWrap) const
> {
>-  mState->GetValue(aValue, aIgnoreWrap);
>+  mState.GetValue(aValue, aIgnoreWrap);
> }
> 
> NS_IMETHODIMP_(nsIEditor*)
> nsHTMLTextAreaElement::GetTextEditor()
> {
>-  return mState->GetEditor();
>+  return mState.GetEditor();
> }
> 
> NS_IMETHODIMP_(nsISelectionController*)
> nsHTMLTextAreaElement::GetSelectionController()
> {
>-  return mState->GetSelectionController();
>+  return mState.GetSelectionController();
> }
> 
> NS_IMETHODIMP_(nsFrameSelection*)
> nsHTMLTextAreaElement::GetConstFrameSelection()
> {
>-  return mState->GetConstFrameSelection();
>+  return mState.GetConstFrameSelection();
> }
> 
> NS_IMETHODIMP
> nsHTMLTextAreaElement::BindToFrame(nsTextControlFrame* aFrame)
> {
>-  return mState->BindToFrame(aFrame);
>+  return mState.BindToFrame(aFrame);
> }
> 
> NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::UnbindFromFrame(nsTextControlFrame* aFrame)
> {
>   if (aFrame) {
>-    mState->UnbindFromFrame(aFrame);
>+    mState.UnbindFromFrame(aFrame);
>   }
> }
> 
> NS_IMETHODIMP
> nsHTMLTextAreaElement::CreateEditor()
> {
>-  return mState->PrepareEditor();
>+  return mState.PrepareEditor();
> }
> 
> NS_IMETHODIMP_(nsIContent*)
> nsHTMLTextAreaElement::GetRootEditorNode()
> {
>-  return mState->GetRootNode();
>+  return mState.GetRootNode();
> }
> 
> NS_IMETHODIMP_(nsIContent*)
> nsHTMLTextAreaElement::CreatePlaceholderNode()
> {
>-  NS_ENSURE_SUCCESS(mState->CreatePlaceholderNode(), nsnull);
>-  return mState->GetPlaceholderNode();
>+  NS_ENSURE_SUCCESS(mState.CreatePlaceholderNode(), nsnull);
>+  return mState.GetPlaceholderNode();
> }
> 
> NS_IMETHODIMP_(nsIContent*)
> nsHTMLTextAreaElement::GetPlaceholderNode()
> {
>-  return mState->GetPlaceholderNode();
>+  return mState.GetPlaceholderNode();
> }
> 
> NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::UpdatePlaceholderText(bool aNotify)
> {
>-  mState->UpdatePlaceholderText(aNotify);
>+  mState.UpdatePlaceholderText(aNotify);
> }
> 
> NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::SetPlaceholderClass(bool aVisible, bool aNotify)
> {
>-  mState->SetPlaceholderClass(aVisible, aNotify);
>+  mState.SetPlaceholderClass(aVisible, aNotify);
> }
> 
> nsresult
>@@ -578,7 +579,7 @@ nsHTMLTextAreaElement::SetValueInternal(const nsAString& aValue,
>   // nsTextControlFrame::UpdateValueDisplay retrieves the correct value
>   // if needed.
>   SetValueChanged(true);
>-  mState->SetValue(aValue, aUserInput);
>+  mState.SetValue(aValue, aUserInput);
> 
>   return NS_OK;
> }
>@@ -605,8 +606,8 @@ nsHTMLTextAreaElement::SetValueChanged(bool aValueChanged)
>   bool previousValue = mValueChanged;
> 
>   mValueChanged = aValueChanged;
>-  if (!aValueChanged && !mState->IsEmpty()) {
>-    mState->EmptyValue();
>+  if (!aValueChanged && !mState.IsEmpty()) {
>+    mState.EmptyValue();
>   }
> 
>   if (mValueChanged != previousValue) {
>@@ -848,8 +849,8 @@ nsHTMLTextAreaElement::GetSelectionStart(PRInt32 *aSelectionStart)
>   PRInt32 selEnd;
>   nsresult rv = GetSelectionRange(aSelectionStart, &selEnd);
> 
>-  if (NS_FAILED(rv) && mState->IsSelectionCached()) {
>-    *aSelectionStart = mState->GetSelectionProperties().mStart;
>+  if (NS_FAILED(rv) && mState.IsSelectionCached()) {
>+    *aSelectionStart = mState.GetSelectionProperties().mStart;
>     return NS_OK;
>   }
>   return rv;
>@@ -858,8 +859,8 @@ nsHTMLTextAreaElement::GetSelectionStart(PRInt32 *aSelectionStart)
> NS_IMETHODIMP
> nsHTMLTextAreaElement::SetSelectionStart(PRInt32 aSelectionStart)
> {
>-  if (mState->IsSelectionCached()) {
>-    mState->GetSelectionProperties().mStart = aSelectionStart;
>+  if (mState.IsSelectionCached()) {
>+    mState.GetSelectionProperties().mStart = aSelectionStart;
>     return NS_OK;
>   }
> 
>@@ -884,8 +885,8 @@ nsHTMLTextAreaElement::GetSelectionEnd(PRInt32 *aSelectionEnd)
>   PRInt32 selStart;
>   nsresult rv = GetSelectionRange(&selStart, aSelectionEnd);
> 
>-  if (NS_FAILED(rv) && mState->IsSelectionCached()) {
>-    *aSelectionEnd = mState->GetSelectionProperties().mEnd;
>+  if (NS_FAILED(rv) && mState.IsSelectionCached()) {
>+    *aSelectionEnd = mState.GetSelectionProperties().mEnd;
>     return NS_OK;
>   }
>   return rv;
>@@ -894,8 +895,8 @@ nsHTMLTextAreaElement::GetSelectionEnd(PRInt32 *aSelectionEnd)
> NS_IMETHODIMP
> nsHTMLTextAreaElement::SetSelectionEnd(PRInt32 aSelectionEnd)
> {
>-  if (mState->IsSelectionCached()) {
>-    mState->GetSelectionProperties().mEnd = aSelectionEnd;
>+  if (mState.IsSelectionCached()) {
>+    mState.GetSelectionProperties().mEnd = aSelectionEnd;
>     return NS_OK;
>   }
> 
>@@ -960,8 +961,8 @@ nsHTMLTextAreaElement::GetSelectionDirection(nsAString& aDirection)
>   }
> 
>   if (NS_FAILED(rv)) {
>-    if (mState->IsSelectionCached()) {
>-      DirectionToName(mState->GetSelectionProperties().mDirection, aDirection);
>+    if (mState.IsSelectionCached()) {
>+      DirectionToName(mState.GetSelectionProperties().mDirection, aDirection);
>       return NS_OK;
>     }
>   }
>@@ -971,14 +972,14 @@ nsHTMLTextAreaElement::GetSelectionDirection(nsAString& aDirection)
> 
> NS_IMETHODIMP
> nsHTMLTextAreaElement::SetSelectionDirection(const nsAString& aDirection) {
>-  if (mState->IsSelectionCached()) {
>+  if (mState.IsSelectionCached()) {
>     nsITextControlFrame::SelectionDirection dir = nsITextControlFrame::eNone;
>     if (aDirection.EqualsLiteral("forward")) {
>       dir = nsITextControlFrame::eForward;
>     } else if (aDirection.EqualsLiteral("backward")) {
>       dir = nsITextControlFrame::eBackward;
>     }
>-    mState->GetSelectionProperties().mDirection = dir;
>+    mState.GetSelectionProperties().mDirection = dir;
>     return NS_OK;
>   }
> 
>@@ -1293,7 +1294,7 @@ nsHTMLTextAreaElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> 
>     if (aName == nsGkAtoms::readonly) {
>       UpdateEditableState(aNotify);
>-      mState->UpdateEditableState(aNotify);
>+      mState.UpdateEditableState(aNotify);
>     }
>     UpdateState(aNotify);
>   }
>@@ -1525,20 +1526,20 @@ NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::GetTextEditorValue(nsAString& aValue,
>                                           bool aIgnoreWrap) const
> {
>-  mState->GetValue(aValue, aIgnoreWrap);
>+  mState.GetValue(aValue, aIgnoreWrap);
> }
> 
> NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::SetTextEditorValue(const nsAString& aValue,
>                                           bool aUserInput)
> {
>-  mState->SetValue(aValue, aUserInput);
>+  mState.SetValue(aValue, aUserInput);
> }
> 
> NS_IMETHODIMP_(void)
> nsHTMLTextAreaElement::InitializeKeyboardEventListeners()
> {
>-  mState->InitializeKeyboardEventListeners();
>+  mState.InitializeKeyboardEventListeners();
> }
> 
> NS_IMETHODIMP_(void)
>@@ -1559,7 +1560,7 @@ nsHTMLTextAreaElement::OnValueChanged(bool aNotify)
> NS_IMETHODIMP_(bool)
> nsHTMLTextAreaElement::HasCachedSelection()
> {
>-  return mState->IsSelectionCached();
>+  return mState.IsSelectionCached();
> }
> 
> void
>diff --git a/content/html/content/src/nsTextEditorState.cpp b/content/html/content/src/nsTextEditorState.cpp
>index 1371e15..d6f3ced 100644
>--- a/content/html/content/src/nsTextEditorState.cpp
>+++ b/content/html/content/src/nsTextEditorState.cpp
>@@ -970,22 +970,24 @@ nsTextEditorState::Clear()
>   NS_IF_RELEASE(mTextListener);
> }
> 
>-NS_IMPL_CYCLE_COLLECTION_CLASS(nsTextEditorState)
>-NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsTextEditorState, AddRef)
>-NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(nsTextEditorState, Release)
>-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_NATIVE(nsTextEditorState)
>+void nsTextEditorState::Unlink()
>+{
>+  nsTextEditorState* tmp = this;
>   tmp->Clear();
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mSelCon)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mEditor)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mRootNode)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mPlaceholderDiv)
>-NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NATIVE_BEGIN(nsTextEditorState)
>+}
>+
>+void nsTextEditorState::Traverse(nsCycleCollectionTraversalCallback& cb)
>+{
>+  nsTextEditorState* tmp = this;
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR_AMBIGUOUS(mSelCon, nsISelectionController)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mEditor)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mRootNode)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPlaceholderDiv)
>-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+}
> 
> nsFrameSelection*
> nsTextEditorState::GetConstFrameSelection() {
>diff --git a/content/html/content/src/nsTextEditorState.h b/content/html/content/src/nsTextEditorState.h
>index c16fff2..56813a8 100644
>--- a/content/html/content/src/nsTextEditorState.h
>+++ b/content/html/content/src/nsTextEditorState.h
>@@ -154,8 +154,8 @@ public:
>   explicit nsTextEditorState(nsITextControlElement* aOwningElement);
>   ~nsTextEditorState();
> 
>-  NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS(nsTextEditorState)
>-  NS_INLINE_DECL_REFCOUNTING(nsTextEditorState)
>+  void Traverse(nsCycleCollectionTraversalCallback& cb);
>+  void Unlink();
> 
>   nsIEditor* GetEditor();
>   nsISelectionController* GetSelectionController() const;
Comment 11 :Ehsan Akhgari (out sick) 2012-04-24 10:45:31 PDT
http://tbpl.mozilla.org/?tree=Try&rev=eec25295ecd7
Comment 12 Andrew McCreight [:mccr8] 2012-04-24 11:13:17 PDT
Comment on attachment 617934 [details] [diff] [review]
Patch (v1)

Review of attachment 617934 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +604,5 @@
>      mInputData.mValue = nsnull;
>    } else {
>      UnbindFromFrame(nsnull);
> +    delete mInputData.mState;
> +    mInputData.mState = nsnull;

Can you really delete an inline struct? And then assign null to it?
Comment 13 :Ehsan Akhgari (out sick) 2012-04-24 15:14:03 PDT
(In reply to Andrew McCreight [:mccr8] from comment #12)
> Comment on attachment 617934 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 617934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLInputElement.cpp
> @@ +604,5 @@
> >      mInputData.mValue = nsnull;
> >    } else {
> >      UnbindFromFrame(nsnull);
> > +    delete mInputData.mState;
> > +    mInputData.mState = nsnull;
> 
> Can you really delete an inline struct? And then assign null to it?

mInputData.mState is a nsTextEditorState*.  :-)

Here's the reason for this craziness.  <input> elements can be text boxes, *and* other things, and <textarea> elements can only be text boxes.  So, we need to have a dynamic nsTextEditorState for nsHTMLInputElement, which only exists when the <input> is actually a text box (and thanks to C++, we can only use a pointer for that), but for nsHTMLTextAreaElement, we should always have an associated nsTextEditorState, which is what this bug is about.
Comment 14 Andrew McCreight [:mccr8] 2012-04-24 15:17:14 PDT
Oh, I see.  Thanks for the explanation!
Comment 15 Mozilla RelEng Bot 2012-04-24 23:00:26 PDT
Try run for eec25295ecd7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eec25295ecd7
Results (out of 221 total builds):
    success: 177
    warnings: 39
    failure: 2
    other: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-eec25295ecd7
 Timed out after 12 hours without completing.
Comment 16 :Ehsan Akhgari (out sick) 2012-04-25 17:11:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac410348fac
Comment 17 Ed Morley [:emorley] 2012-04-26 10:43:13 PDT
https://hg.mozilla.org/mozilla-central/rev/6ac410348fac

Note You need to log in before you can comment on or make changes to this bug.