Last Comment Bug 688208 - nsWindow for Android doesn't check whether the instance is destroying or not after dispatching an event
: nsWindow for Android doesn't check whether the instance is destroying or not ...
Status: RESOLVED FIXED
[sg:critical?][qa-]
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
: -- critical (vote)
: mozilla11
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-21 10:31 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-04-10 21:24 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
-
-
fixed
11+
fixed
unaffected
+


Attachments
OnDestroy patch rev. 1 (648 bytes, patch)
2011-10-06 15:19 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
kungFu patch rev. 1 (10.39 KB, patch)
2011-10-06 15:21 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Review
OnDestroy patch rev. 2 (875 bytes, patch)
2011-11-22 12:34 PST, Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
bugmail.mozilla: checkin+
Details | Diff | Review
kungFu patch rev. 2 (11.36 KB, patch)
2011-11-22 12:34 PST, Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
bugmail.mozilla: checkin+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 10:31:51 PDT
Android's nsWindow doesn't hold itself and doesn't check whether it's destroying or not after dispatching an event. I'm not sure that actually causes crash bugs. We should research it.

nsWindow might be destroyed by DOM event handler. Mac's widget has fixed this already but I'm not sure the detail and bug#. Steven probably knows the bug#.

I think nsWindow for Android should do:

+    nsRefPtr<nsWindow> kungFuDeathGrip(this);
     DispatchEvent(event);
+    if (Destroyed()) {
+        return;
+    }

And:

nsWindow should set nsBaseWidget::mOnDestroyCalled to TRUE when it's destroying.
# It seems that nsBaseWidget::OnDestroy() should be called too...
Comment 1 Daniel Veditz [:dveditz] 2011-09-21 16:40:49 PDT
Don't know enough about android to confirm this, but plausibly sg:critical based on analogy with other platforms.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-05 11:55:05 PDT
Bug 402505 is the one where this issue was happening on Mac. I tried the test cases from that bug on android and they didn't trigger any crashes. I'll keep investigating and try other ways to trigger the crash before concluding one way or another.
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-06 15:19:29 PDT
So I wasn't able to trigger the crash mostly because it seems impossible to close the window on Android. After discussing with blassey, it makes sense to fix this anyway since in the future it might be possible to close windows and this bug might show up. Will attach two patches (one for the missing nsBaseWidget::OnDestroy() call, and one for the kungFuDeathGrip stuff.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-06 15:19:51 PDT
Created attachment 565374 [details] [diff] [review]
OnDestroy patch rev. 1
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-06 15:21:07 PDT
Created attachment 565375 [details] [diff] [review]
kungFu patch rev. 1

Sorry, forgot to check the patch box on the previous one.
Comment 6 Doug Turner (:dougt) 2011-10-09 19:27:13 PDT
kats, can you build a test case that opens two xul windows and programatically close them?
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 19:17:02 PST
Comment on attachment 565375 [details] [diff] [review]
kungFu patch rev. 1

Each platform seems to handle these "check for destroyed after a DOM event" differently, but it looks like you have things covered here. Certainly better covered than before.

It's also true that we don't have really great ways to test these cases yet either.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-22 12:34:05 PST
Created attachment 576226 [details] [diff] [review]
OnDestroy patch rev. 2

Rebase to m-c tip and update patch so that it is an hg export with commit message rather than a raw diff.
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-22 12:34:39 PST
Created attachment 576227 [details] [diff] [review]
kungFu patch rev. 2

Rebase to m-c tip and update patch so that it is an hg export with commit message rather than a raw diff.
Comment 12 Daniel Veditz [:dveditz] 2012-02-23 16:31:26 PST
Since we're shipping XUL Mobile "11" based on ESR so we will need to fix this one on the ESR in order to get this mobile-only security fix out to users.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-02-23 16:46:29 PST
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-24 09:47:30 PST
Landed on the mozilla-esr10 repository:

https://hg.mozilla.org/releases/mozilla-esr10/rev/9d8331f4e43b
https://hg.mozilla.org/releases/mozilla-esr10/rev/4663a567d884
Comment 15 [On PTO until 6/29] 2012-02-28 12:44:23 PST
Did we create a way to test this fix? I see comments about doing so but that's it.
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-02-28 13:16:45 PST
No, IIRC we couldn't find a way to test this behaviour properly.
Comment 17 [On PTO until 6/29] 2012-03-01 17:34:13 PST
Not a 1.9.2 issue. Marking as unaffected.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 10:38:41 PST
Untracking from QA Firefox Desktop verifications

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