Last Comment Bug 672577 - Crash [@ SelectionChangeEvent::Run]
: Crash [@ SelectionChangeEvent::Run]
Status: VERIFIED FIXED
[good first bug][mentor=jdm]
: crash, inputmethod
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla8
Assigned To: Pranay Choudhary
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-19 11:22 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-08-11 11:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for handling null pointers in SelectionChangeEvent::RUN and a couple of more places. (900 bytes, patch)
2011-07-30 03:21 PDT, Pranay Choudhary
blassey.bugs: review+
Details | Diff | Splinter Review
Updated Patch (incorporated review comments) (2.30 KB, patch)
2011-08-08 11:46 PDT, Pranay Choudhary
blassey.bugs: review+
Details | Diff | Splinter Review
Updated Patch (2.35 KB, patch)
2011-08-09 19:05 PDT, Pranay Choudhary
blassey.bugs: review+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-19 11:22:04 PDT
I'm hitting this crash on my LG Optimus Black phone.
Minimizing that crash from my phone is very difficult and time consuming. Perhaps someone can fix it easily without providing a simple testcase?

Perhaps I don't have IME enabled on Windows, so it doesn't crash there. Is there a way to enable it there?

https://crash-stats.mozilla.com/report/index/7386a4a1-b863-40d6-be79-b500a2110719
0 	libxul.so 	SelectionChangeEvent::Run 	content/events/src/nsIMEStateManager.cpp:504
1 	libxul.so 	nsContentUtils::AddScriptRunner 	nsCOMPtr.h:492
2 	libxul.so 	nsTextStateManager::NotifySelectionChanged 	content/events/src/nsIMEStateManager.cpp:523
3 	libxul.so 	nsTypedSelection::NotifySelectionListeners 	layout/generic/nsSelection.cpp:5762
4 	libxul.so 	nsFrameSelection::NotifySelectionListeners 	layout/generic/nsSelection.cpp:2288
5 	libxul.so 	nsFrameSelection::TakeFocus 	layout/generic/nsSelection.cpp:1891
6 	libxul.so 	nsFrameSelection::HandleClick 	layout/generic/nsSelection.cpp:1659
7 	libxul.so 	nsFrame::PeekBackwardAndForward 	layout/generic/nsFrame.cpp:2540
8 	libxul.so 	nsFrame::HandleMultiplePress 	layout/generic/nsFrame.cpp:2472
9 	libxul.so 	nsFrame::HandlePress 	layout/generic/nsFrame.cpp:2289
10 	libxul.so 	nsFrame::HandleEvent 	layout/generic/nsFrame.cpp:1847
11 	libxul.so 	nsPresShellEventCB::HandleEvent 	layout/base/nsPresShell.cpp:1493
12 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	nsCOMPtr.h:655
13 	libxul.so 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:674
14 	libxul.so 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:7072
15 	libxul.so 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6902
16 	libxul.so 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6734
17 	libxul.so 	nsViewManager::HandleEvent 	view/src/nsViewManager.cpp:1056
18 	libxul.so 	nsViewManager::DispatchEvent 	view/src/nsViewManager.cpp:1031
19 	libxul.so 	HandleEvent 	nsCOMPtr.h:492
20 	libxul.so 	mozilla::widget::PuppetWidget::DispatchEvent 	widget/src/xpwidgets/PuppetWidget.cpp:328
Comment 1 Jim Chen [:jchen] [:darchons] 2011-07-19 12:00:08 PDT
Looks like we should check for mWidget == NULL in TextChangeEvent and SelectionChangeEvent.

I think blassey will gladly take this bug :P
Comment 2 Josh Matthews [:jdm] (away until 9/3) 2011-07-21 14:47:42 PDT
This is an easy first bug for anyone just getting involved. A simple null-check should avoid this crash.
Comment 3 Josh Matthews [:jdm] (away until 9/3) 2011-07-21 19:30:46 PDT
Pranay said that he's interested in working on this one.
Comment 4 Pranay Choudhary 2011-07-30 03:21:28 PDT
Created attachment 549562 [details] [diff] [review]
Patch for handling null pointers in SelectionChangeEvent::RUN and a couple of more places.
Comment 5 Josh Matthews [:jdm] (away until 9/3) 2011-07-30 07:03:02 PDT
Comment on attachment 549562 [details] [diff] [review]
Patch for handling null pointers in SelectionChangeEvent::RUN and a couple of more places.

Thanks for the patch, Pranay! I'm going to hand this off to Brad, but in the meantime could you generate another patch that follows the instructions at https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f to make this easier to check in once the review is finished? Also, a style nitpick to fix at the same time:

>if (count > 0 && mWidget!=NULL) {

Just check for mWidget everywhere; you can drop the |!=NULL|.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2011-07-30 08:37:13 PDT
Comment on attachment 549562 [details] [diff] [review]
Patch for handling null pointers in SelectionChangeEvent::RUN and a couple of more places.

Review of attachment 549562 [details] [diff] [review]:
-----------------------------------------------------------------

As Josh said, you can simply test mWidget rather than mWidget != NULL, please make that change.

Also, in none of these cases is mWidget expected to be null. In addition to these null checks, can you add an NS_ABORT_IF_FALSE(mWidget, "mWidget should not be null"); just above it? It may also be helpful to add the same assertion in nsTextStateManager::Init() and the constructors of SelectionChangeEvent and TextChangeEvent. 

JChen, out of curiosity, why are we checking that count is greater than 0 in nsTextStateManager::NotifySelectionChanged()?
Comment 7 Jim Chen [:jchen] [:darchons] 2011-07-30 14:44:15 PDT
(In reply to comment #4)
> Created attachment 549562 [details] [diff] [review] [diff] [details] [review]
> Patch for handling null pointers in SelectionChangeEvent::RUN and a couple
> of more places.

Thanks for contributing the patch! Besides what Josh mentioned, another way to improve it is to check for null in NotifySelectionChanged only, and leave SelectionChangeEvent::Run unchanged.

The reason is because ::Run gets its mWidget value from NotifySelectionChanged, so if you already checked mWidget is not null in NotifySelectionChanged, it is certain that mWidget will not be null in ::Run.

(In reply to comment #6)
> 
> Also, in none of these cases is mWidget expected to be null. In addition to
> these null checks, can you add an NS_ABORT_IF_FALSE(mWidget, "mWidget should
> not be null"); just above it? It may also be helpful to add the same
> assertion in nsTextStateManager::Init() and the constructors of
> SelectionChangeEvent and TextChangeEvent. 
> 
I don't think we should be aborting. nsTextStateManager::Destroy() sets mWidget to nsnull. We also unregister listeners in ::Destroy, but if we do get a stray callback after ::Destroy() is called, mWidget will be null. I think we should just ignore cases like this.

> JChen, out of curiosity, why are we checking that count is greater than 0 in
> nsTextStateManager::NotifySelectionChanged()?

I think widget expects there to be some sort of selection in an editor. count might be 0 in the case of blurring, when selection is lost. In that case widget might get confused if we tell it selection has changed.
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2011-07-30 14:56:00 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > 
> > Also, in none of these cases is mWidget expected to be null. In addition to
> > these null checks, can you add an NS_ABORT_IF_FALSE(mWidget, "mWidget should
> > not be null"); just above it? It may also be helpful to add the same
> > assertion in nsTextStateManager::Init() and the constructors of
> > SelectionChangeEvent and TextChangeEvent. 
> > 
> I don't think we should be aborting. nsTextStateManager::Destroy() sets
> mWidget to nsnull. We also unregister listeners in ::Destroy, but if we do
> get a stray callback after ::Destroy() is called, mWidget will be null. I
> think we should just ignore cases like this.

My assertion here is that if Run() gets called after Destroy() its a bug and we should track it down. A run time abort in debug builds (essentially a debug assertion, but we killed the meaning of that with NS_ASSERTION()) will help us track that down.
Comment 9 Josh Matthews [:jdm] (away until 9/3) 2011-07-30 19:17:29 PDT
MOZ_ASSERT will give you a runtime abort in debug builds without forcing an abort in release builds.
Comment 10 Pranay Choudhary 2011-08-08 11:46:46 PDT
Created attachment 551517 [details] [diff] [review]
Updated Patch (incorporated review comments)
Comment 11 Brad Lassey [:blassey] (use needinfo?) 2011-08-08 12:29:19 PDT
Comment on attachment 551517 [details] [diff] [review]
Updated Patch (incorporated review comments)

Review of attachment 551517 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/src/nsIMEStateManager.cpp
@@ +505,4 @@
>    }
>  
>    NS_IMETHOD Run() {
>      mWidget->OnIMESelectionChange();

in your previous patch you null checked mWidget here. You should still do that.
Comment 12 Pranay Choudhary 2011-08-09 00:20:24 PDT
(In reply to Brad Lassey [:blassey][blassey@mozilla.com] from comment #11)
> in your previous patch you null checked mWidget here. You should still do
> that.
in comment 7 Jim mentioned that "check for null in NotifySelectionChanged only, and leave SelectionChangeEvent::Run unchanged"
So I've removed the null checking in SelectionChangeEvent.
Comment 13 Pranay Choudhary 2011-08-09 19:05:32 PDT
Created attachment 551973 [details] [diff] [review]
Updated Patch

incorporated review comment by Brad Lassey (Comment 11)
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-09 19:41:00 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/03dd6d99d17d
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-08-11 11:28:27 PDT
The fix is working, no crash anymore with the unminized testcase on current trunk build on my LG Optimus Black. Thanks!

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