Closed Bug 443024 Opened 12 years ago Closed 12 years ago

Crash [@ nsChildView::ResetInputState()] but really [@ nsTSMManager::CommitIME()] while doing Flash IME

Categories

(Core :: Widget: Cocoa, defect, P1, critical)

x86
macOS
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: smichaud, Assigned: smichaud)

References

()

Details

(5 keywords)

Crash Data

Attachments

(1 file, 1 obsolete file)

The following STR "works" (causes a crash) 100% of the time in FF3 and
today's mozilla-central and 1.9.0-branch nightlies, even though Flash
IME input doesn't work at all in FF3 and current 1.9.0-branch
nightlies.

For this STR you need to have Japanese IME enabled.  It also helps if
you know at least a little Japanese.

I tested on OS X 10.5.3.  Later I'll also test on 10.4.11.

1) Start Firefox and visit
   http://www.playercore.com/bugfiles/146162/AddReturnHTML.html.

2) Choose Hiragana input from the "Flags menu" (in the screen's upper
   right near the date).  Enter some text in the "Enter your text" box
   (I just entered "hiragana").

   Hiragana input only works properly in current mozilla-central
   nightlies (since the patch for bug 357670 was landed).  As you type
   the IME input window will open at the bottom of the screen.

   In FF3 and today's 1.9.0-branch nightly, as you type the text
   "hiragana" will appear in the "Enter your text" box.

3) Press the Back button to return to your home page.

4) Press the Forward button to return to the AddReturnHTML Flash
   "movie".

5) Make sure Hiragana input is still selected in the Flags menu (it
   should be).

6) Click on the Reload button to reload the page.

7) Click in the "Enter your text" box.

   You should now crash.

The crash actually happens in nsTSMManager::CommitIME().  But
sometimes the top part of the stack is missing, and the closest
meaningful symbol to the top is nsChildView::ResetInputState().
Assignee: joshmoz → smichaud
Summary: Crash in nsChildView::ResetInputState() while doing Flash IME → Crash [@ nsTSMManager::CommitIME()] but really [@ nsChildView::ResetInputState()] while doing Flash IME
Breakpad has about 1400 crashes in nsChildView::ResetInputState() over
the last two weeks.  This puts this bug's crash around #50 (overall)
in the topcrasher list.

I don't think it's important enough to block 1.9.0.1 at the last
minute.  It should definitely block 1.9.0.2.  But since it's not yet
possible to request blocking1.9.0.2, I'm temporarily just requesting
wanted1.9.0.x.
Flags: wanted1.9.0.x?
I'm working on a patch for this, and should (I hope) have one up later today or tomorrow.
Summary: Crash [@ nsTSMManager::CommitIME()] but really [@ nsChildView::ResetInputState()] while doing Flash IME → Crash [@ nsChildView::ResetInputState()] but really [@ nsTSMManager::CommitIME()] while doing Flash IME
(Following up comment #0)

> 5) Make sure Hiragana input is still selected in the Flags menu (it
>    should be).

Argh, I dropped some lines from this step.  The full step 5 should be:

5) Make sure Hiragana input is still selected in the Flags menu (it
   should be).

   Type some text in the "Enter your text" box.  It will appear as
   Roman letters in the box (you won't see the input window), even
   using today's mozilla-central nightly.  This is the bug reported at
   bug 357670 comment #110.
Note:  _None_ of Breakpad's reports for this crash have either
nsChildView::ResetInputState() or nsTSMManager::CommitIME() at the top
of the stack.

Probably the most efficient search is the one I used to get the
numbers in comment #1 -- look for "One of the top 10 Stack Frames" "is
exactly" "nsChildView::ResetInputState()".
Priority: -- → P1
Flags: blocking1.9.1?
Attached patch Fix (obsolete) — Splinter Review
The crash happens when nsTSMManager::sComposingView gets used after
it's been deleted.  The fix is very simple -- null out
nsTSMManager::sComposingView as its corresponding ChildView is
destroyed.
Attachment #327701 - Flags: review?(joshmoz)
Attachment #327701 - Flags: review?(masayuki)
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Attachment #327701 - Flags: review?(joshmoz) → review+
Attachment #327701 - Flags: superreview?(roc)
I don't hope nsTSMManager::sComposingView is public.
And the cleanup code should not be outside of nsTSMManager.

How about following code?:

- (void)widgetDestroyed

{
  nsTSMManager::OnDesctoryView(this);
...
}

...

void
nsTSMManager::OnDestroyView(NSView<mozView>* aDestroyingView)
{
  if (aDestroyingView != sComposingView)
    return;
  if (IsComposing()) // XXX at destroying, CancelIME() will be failed?
    CancelIME();
  EndComposing();
}
Attachment #327701 - Flags: superreview?(roc) → superreview?
Attachment #327701 - Flags: superreview?
Thanks, Masayuki.  Your suggestions make sense and I've followed them.

I'm not sure what you mean by CancelIME() "failing".  You won't see
crashes caused by sComposingView having been deleted -- because it
hasn't been deleted yet.  But yes, I suppose it may fail in some other
sense of the word.
Attachment #327701 - Attachment is obsolete: true
Attachment #327816 - Flags: review?(joshmoz)
Attachment #327701 - Flags: review?(masayuki)
Comment on attachment 327816 [details] [diff] [review]
Fix rev1 (follow Masayuki's suggestions)

I'm OK with this, though it would be nice to figure out the question on the comment added and remove the comment.
Attachment #327816 - Flags: review?(joshmoz) → review+
Attachment #327816 - Flags: review?(masayuki)
(Following up comment #7)

> Thanks, Masayuki.  Your suggestions make sense and I've followed
> them.

I did change your code so that EndComposing() is only called if
IsComposing() returns true -- I figured not doing this (i.e. always
calling EndComposing()) was simply a typo.
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Flags: blocking1.9.1+
Comment on attachment 327816 [details] [diff] [review]
Fix rev1 (follow Masayuki's suggestions)

Excellent! Thank you for your work!
Attachment #327816 - Flags: review?(masayuki) → review+
Attachment #327816 - Flags: superreview?(roc)
Attachment #327816 - Flags: superreview?(roc) → superreview?(vladimir)
Attachment #327816 - Flags: superreview?(vladimir) → superreview+
Landed on mozilla-central.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 327816 [details] [diff] [review]
Fix rev1 (follow Masayuki's suggestions)

This is a trivial fix for a major problem (a topcrasher), and has been
baking on mozilla-central for four days with no reported problems.

I think it should be landed on the 1.9.0 branch soon, so it has time
to bake there for a while before the 3.0.2 release.
Attachment #327816 - Flags: approval1.9.0.2?
Is there any hope of an automated test here? If not, can we get a Litmus test (or two) before landing on 1.9.0?
Flags: in-testsuite?
Flags: in-litmus?
> Is there any hope of an automated test here?

I don't know.

Trivial patches (which this one basically is) usually go in without
tests.  The only non-trivial part is the call to CancelIME().  But I'm
quite sure it'll be impossible to test that part of the patch (in
either automated or manual tests).  And it's just as likely that
leaving that call out will cause trouble as it is that leaving it in
will cause trouble.

The only non-trivial part of the patch (the call to CancelIME())
really does require exposure to lots of users.  So I'd like to get
this on to the 1.9.0 branch as soon as possible (and as far in advance
of the 3.0.2 release as possible).
> And it's just as likely that leaving that call out will cause
> trouble as it is that leaving it in will cause trouble.

I should have said it's *at least* as likely that leaving out the call
to CancelIME() will cause trouble as it is that leaving the call in
will cause trouble.

In other words, it's better (probably much better) to leave the call
in.  But either leaving it in or getting rid of it might cause
trouble.
(In reply to comment #14)
> Trivial patches (which this one basically is) usually go in without
> tests.

I disagree with this, but I understand that testing in Mozilla Cocoa-land right now isn't very possible. We should always have a test for patches before they land, *especially* if they're crash bugs like this one is.

Let's get a Litmus testcase added and then I'll approve.
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008072102 Minefield/3.1a1pre. I verified using the STR in Comment 0.
Status: RESOLVED → VERIFIED
>> Trivial patches (which this one basically is) usually go in without
>> tests.
>
> I disagree with this ...

Bug 444128 is a recent example of a crash bug with a trivial fix,
whose patch was approved for the 1.9.0 branch without any tests (or
any request for tests).  I'm not complaining -- I think this is the
correct approach.

It's true that this patch isn't quite as trivial ... but still :-)
> Let's get a Litmus testcase added and then I'll approve.

This sounds fine to me.
https://litmus.mozilla.org/show_test.cgi?id=5888 added to Litmus.
Flags: in-litmus? → in-litmus+
> https://litmus.mozilla.org/show_test.cgi?id=5888 added to Litmus.

Oops, Marcia.  I corrected step 5 in comment #3 above.
Comment on attachment 327816 [details] [diff] [review]
Fix rev1 (follow Masayuki's suggestions)

Approved for 1.9.0.2. Please land asap in CVS so we can get lots of baking. a=ss
Attachment #327816 - Flags: approval1.9.0.2? → approval1.9.0.2+
>> https://litmus.mozilla.org/show_test.cgi?id=5888 added to Litmus.
>
> Oops, Marcia.  I corrected step 5 in comment #3 above.

Sorry, Marcia.  Your steps are correct.  I hadn't noticed that my correction to step 5 became your step 6.
Landed on the 1.9.0 branch:

Checking in widget/src/cocoa/nsChildView.h;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.h,v  <--  nsChildView.h
new revision: 1.96; previous revision: 1.95
done
Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.362; previous revision: 1.361
done
Keywords: fixed1.9.0.2
Crash Signature: [@ nsChildView::ResetInputState()] [@ nsTSMManager::CommitIME()]
You need to log in before you can comment on or make changes to this bug.