As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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]
:
: Markus Stange [:mstange]
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]
smichaud: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description User image 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image 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 User image Masayuki Nakano [:masayuki] 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 User image 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 User image Masayuki Nakano [:masayuki] 2011-03-04 16:31:35 PST
I see, thank you for your information.
Comment 13 User image Al Billings [:abillings] 2011-03-22 17:44:48 PDT
Are there any steps to reproduce for this bug?
Comment 14 User image 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 User image Masayuki Nakano [:masayuki] 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 User image Masayuki Nakano [:masayuki] 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.