71274 - Input Method crash in Carbonalized Mac mozilla build

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Frank Tang]

We find this bug when nhotta partially fix bug 71272. 
IT crash when we hit "ENTER" key after IME composition.
It crash in this line 550 (It is hard to figure out since the debugger got scrow 
up in the stack. but I find it crash there after I inject printf between all the 
lines. )
529 nsresult
530 nsTextEditorTextListener::HandleText(nsIDOMEvent* aTextEvent)
531 {
.....
548    textEvent->GetInputRange(&textRangeList);
549    textEvent->GetEventReply(&textEventReply);
550    textRangeList->AddRef();
What happen is textRangeList is nsnull when we hit ENTER in the IME mode.

The reason is in mac when we hit ENTER we will pass 0 as range count in the 
following line

/widget/src/mac/nsMacEventHandler.cpp, line 1925 -- res = 
HandleTextEvent(0,nsnull);

The reason the textRangeList is null is because in 

127 nsDOMEvent::nsDOMEvent(nsIPresContext* aPresContext, nsEvent* aEvent, const 
nsAReadableString& aEventType) 
128 {
.......
175     nsIPrivateTextRange** tempTextRangeList = new 
nsIPrivateTextRange*[((nsTextEvent*)aEvent)->rangeCount];
176     if (tempTextRangeList!=nsnull) {
......
186     
187     mTextRange = (nsIPrivateTextRangeList*) new 
nsPrivateTextRangeList(((nsTextEvent*)aEvent)->rangeCount,tempTextRangeList);
188     if (mTextRange!=nsnull)  mTextRange->AddRef();
189     }

the interesting thing is the mTextRange is create inside the line 176 if block 
so if tempTextRangeList is null this mTextRange will be nsnull.
Somehow in non Carbonalized build, when ((nsTextEvent*)aEvent)->rangeCount is 
zero , the new operator in line 175 still allocate some thing and return non 
null value. So we don't crash in non Carbonalized build , but in carbonalized 
build when we try to allocate a zero length array, it return null. Therefore 
line 187 does not get hit and so it crash.

The solution is to move line 187 and 188 out side the block- move the } in line 
189 before line 187.

I will attach a seperate diff later.

Comment 1

[nhottanscp]

Reassign to ftang.

Comment 2

[Frank Tang]

Here is the patch (by ftang)



Index: mozilla/content/events/src/nsDOMEvent.cpp

===================================================================

RCS file: /cvsroot/mozilla/content/events/src/nsDOMEvent.cpp,v

retrieving revision 1.103

diff -u -1 -5 -r1.103 nsDOMEvent.cpp

--- nsDOMEvent.cpp	2001/03/02 03:07:53	1.103

+++ nsDOMEvent.cpp	2001/03/08 19:39:29

@@ -171,34 +171,37 @@

 	  //

 	  // build the range list -- ranges need to be DOM-ified since the IME 

transaction

 	  //  will hold a ref, the widget representation isn't persistent

 	  //

 	  nsIPrivateTextRange** tempTextRangeList = new nsIPrivateTextRange*

[((nsTextEvent*)aEvent)->rangeCount];

 	  if (tempTextRangeList!=nsnull) {

 		for(PRUint16 i=0;i<((nsTextEvent*)aEvent)->rangeCount;i++) {

 			nsPrivateTextRange* tempPrivateTextRange = new 

nsPrivateTextRange((((nsTextEvent*)aEvent)->rangeArray[i]).mStartOffset,

 														

(((nsTextEvent*)aEvent)->rangeArray[i]).mEndOffset,

 														

(((nsTextEvent*)aEvent)->rangeArray[i]).mRangeType);

 			if (tempPrivateTextRange!=nsnull) {

 				tempPrivateTextRange->AddRef();

 				tempTextRangeList[i] = (nsIPrivateTextRange*

)tempPrivateTextRange;

 			}

 		}

-		

-		mTextRange = (nsIPrivateTextRangeList*) new 

nsPrivateTextRangeList(((nsTextEvent*)aEvent)->rangeCount,tempTextRangeList);

-		if (mTextRange!=nsnull)  mTextRange->AddRef();

 	  }

+	  // We need to create mTextRange even rangeCount is 0. 

+	  // if rangeCount is 0, mac carbon will return 0 for new and 

tempTextRangeList will be null. but we should still

+	  // create mTextRange, otherwise, we will crash it later when some code 

call GetInputRange and AddRef to the result

+	  mTextRange = (nsIPrivateTextRangeList*) new 

nsPrivateTextRangeList(((nsTextEvent*)aEvent)->rangeCount,tempTextRangeList);

+      if (mTextRange!=nsnull)  

+         mTextRange->AddRef();

   }

 

   NS_INIT_REFCNT();

 }

 

 nsDOMEvent::~nsDOMEvent() 

 {

   NS_ASSERT_OWNINGTHREAD(nsDOMEvent);

 

   nsCOMPtr<nsIPresShell> shell;

   if (mPresContext)

   { // we were arena-allocated, prepare to recycle myself    

     mPresContext->GetShell(getter_AddRefs(shell));

   }

   NS_IF_RELEASE(mPresContext);

Comment 3

[nhottanscp]

r=nhotta

Comment 4

[Brendan Eich [:brendan]]

Please attach a diff -u, so others can detach and patch -- the inline diff -u
here has had its newlines doubled, making it hard to feed to patch as well as
hard to read.

This is a dom change, so jst is the area super-reviewer, per
http://www.mozilla.org/hacking/reviewers.html.

/be

Comment 5

[Simon Fraser [no longer active]]

The double spacing is a bug in Bugzilla, which double-spaces repeated submissions 
on Mac (i.e. those submitted after a mid-air collision). I wish they'd fix it.

Comment 6

[Dawn Endico]

if you're referring to bug  71384, the problem is fixed on bugzilla's 
trunk and will be fixed on bugzilla.mozilla.org the next time we update.
(tomorrow)

Comment 7

[Frank Tang]

Attachment: -u diff

Comment 8

[Frank Tang]

jst - can you sr?

Comment 9

[Johnny Stenback (:jst)]

Attachment: Additional required patch

Comment 10

[Johnny Stenback (:jst)]

If you include (and get a review for) the patch I just attached before checking
in, then sr=jst

If you look at the diff I just attached, especially the very last part of it,
and look at the code past the end of the diff, it goes on and does:

  mList[aIndex]->AddRef();
  *aReturn = mList[aIndex];

  return NS_OK;

so if aLength == mLength == 0 we'd crash if mList is null. The whole ownership
model between these classes is kinda messed up, but I'll let that go for now...

Comment 11

[Frank Tang]

I agree with you. bad code exist for a long time ....

Comment 12

[Andreas Becker]

Changing QA Contact to [e-mail] and adding keywords intl and nsbeta1.

Comment 13

[Frank Tang]

fix and check in

Comment 14

[Yuying Long]

Verified on build:
ftp://ftp.mozilla.org/pub/mozilla/contrib/fizzillacfm/3_26_01.sit with
Carbonlib-Ja 1.2.5 installed.