Last Comment Bug 636281 - crash [@ -[ChildView setMarkedText:selectedRange:]]
: crash [@ -[ChildView setMarkedText:selectedRange:]]
Status: RESOLVED FIXED
[qa-examined-192] [qa-examined-191]
: crash
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: 1.9.2 Branch
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-23 14:55 PST by Smokey Ardisson (offline for a while; not following bugs - do not email)
Modified: 2013-07-04 08:48 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.17-fixed
.19-fixed


Attachments
Patch v1.0 for 1.9.2 (880 bytes, patch)
2011-02-23 19:18 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Smokey Ardisson (offline for a while; not following bugs - do not email) 2011-02-23 14:55:15 PST
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) 2011-02-23 15:23:53 PST
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] (Mozilla Japan) 2011-02-23 18:12:48 PST
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] (Mozilla Japan) 2011-02-23 19:18:44 PST
Created attachment 514709 [details] [diff] [review]
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) 2011-02-23 19:24:31 PST
Comment on attachment 514709 [details] [diff] [review]
Patch v1.0 for 1.9.2

Looks good to me.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-02-23 20:11:05 PST
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] (Mozilla Japan) 2011-02-23 20:31:49 PST
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] (Mozilla Japan) 2011-02-23 21:39:33 PST
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) 2011-02-24 11:41:01 PST
(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] 2011-02-28 10:45:57 PST
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] (Mozilla Japan) 2011-02-28 20:35:06 PST
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] 2011-03-04 10:14:47 PST
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] (Mozilla Japan) 2011-03-04 16:31:35 PST
I see, thank you for your information.
Comment 13 Al Billings [:abillings] 2011-03-22 17:44:48 PDT
Are there any steps to reproduce for this bug?
Comment 14 Smokey Ardisson (offline for a while; not following bugs - do not email) 2011-03-23 14:11:49 PDT
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] (Mozilla Japan) 2011-03-23 18:23:18 PDT
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] (Mozilla Japan) 2013-07-04 08:48:25 PDT
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.

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