636281 - crash [@ -[ChildView setMarkedText:selectedRange:]]

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This bug was filed from the Socorro interface and is 
report bp-d7fe25bb-2e2e-416d-8ef7-65c492110216 and 
report bp-27247c52-d1ae-4830-a7da-99a5c2110221.
============================================================= 

There's a low-grade crash on 1.9.0, 1.9.1, and 1.9.2 in -[ChildView setMarkedText:selectedRange:] related to IME.

The reports finger http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/0c159bd1d600/widget/src/cocoa/nsChildView.mm#l4808, which looks suspicious because there's no null-check of mGeckoChild there (or anywhere in |setMarkedText:selectedRange:| at all).

mGeckoChild *is* null-checked there on the trunk; Steven added it in bug 538251 because of crashes that appeared to have this very signature. 

It's not clear what the source of the bug is ultimately, but null-checking should help; can we take the patch from bug 538251 on the branches?

Comment 1

[Steven Michaud [:smichaud] (Retired)]

The IME code is quite different on the 1.9.2 branch from what it is on
the trunk, so I (or someone) would need to rewrite my patch.  But
adding null-checks of mGeckoChild to IME code on the branch (as
needed) does seem like a good idea.

I won't be able to get to this for a while -- I'm totally swamped by
other bugs.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I'll take this. Is this needed for 1.9.0? I don't remember the status of 1.9.0.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0 for 1.9.2

I checked all IME related methods. The other methods check mGeckoChild first if they use mGeckoChild. Therefore, the query selection event dispatching is the crash point. So, this patch should be okay for 1.9.2.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

Looks good to me.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

Fixes a low frequency crash bug and the risk is low because similar check is in trunk.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

I confirmed that this patch can be landed on 1.9.1 branch too.

The related methods wasn't changed during 1.9.1 and 1.9.2. And IME works fine with this patch on 1.9.1 too.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

1.9.0 branch is same as 1.9.1 and 1.9.2. I confirmed that this patch doesn't break IME handling on 1.9.0 and checked the related method's code.

So, we can land this patch to all branches.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> Comment on attachment 514709 [details] [diff] [review]
> Patch v1.0 for 1.9.2
> 
> 1.9.0 branch is same as 1.9.1 and 1.9.2. I confirmed that this patch doesn't
> break IME handling on 1.9.0 and checked the related method's code.

Thanks Masayuki!  If you'd like me to handle the 1.9.0 branch landing once this has approval, just let me know.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bd8d194972df
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ad4a06aec979

(In reply to comment #8)
> If you'd like me to handle the 1.9.0 branch landing once this
> has approval, just let me know.

Yes, please.

Comment 11

[Daniel Veditz [:dveditz]]

The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I see, thank you for your information.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Are there any steps to reproduce for this bug?

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I never could deduce any STR from the crash reports I looked at (besides "doing something with IME"), so unless Masayuki discovered some, no :(

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I *guess* that some web pages might close its window by composition events or text event. And also the using IME calls [NSTextInput setMarkedText:selectedRange] after our nsChildView is destroyed.

However, closing window at such input events doesn't make sense. So, I'm not sure the STR.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

This fix must not be necessary for 1.9.0.x anymore.