Last Comment Bug 726502 - nsDeviceMotion::DeviceMotionChanged may index out of bounds mWindowListeners array
: nsDeviceMotion::DeviceMotionChanged may index out of bounds mWindowListeners ...
Status: RESOLVED FIXED
[sg:critical][qa-]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-12 19:26 PST by Olli Pettay [:smaug]
Modified: 2015-10-16 11:38 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
wontfix
+
fixed
+
fixed
12+
fixed


Attachments
patch v.1 (2.08 KB, patch)
2012-03-09 02:20 PST, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch .2 (4.08 KB, patch)
2012-03-09 08:16 PST, Doug Turner (:dougt)
bugs: review-
Details | Diff | Splinter Review
patch v.3 (2.90 KB, patch)
2012-03-09 08:30 PST, Doug Turner (:dougt)
bugs: review+
Details | Diff | Splinter Review
patch of what landed on m-c (2.94 KB, patch)
2012-03-10 20:13 PST, Doug Turner (:dougt)
doug.turner: review+
akeybl: approval‑mozilla‑aurora-
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-02-12 19:26:04 PST
The code dispatches DOM events synchronously, and DOM event listeners can do anything, 
like close windows. As far as I see nothing guarantees that
mWindowListeners is still long enough when next iteration starts (after event dispatch).
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-22 17:19:10 PST
We should probably protect mListeners here as well.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 13:34:28 PST
Doug, any chance you could look into this? If you're too pressured with mobile stuff right now, do you know someone else who could look into this?
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-03-08 12:58:57 PST
Presumably a testcase would be something like:
Have a page with an iframe. The iframe adds an event listener for "devicemotion" the notifies the parent, at which time the parent adds an event listener for "devicemotion". In the parent's "devicemotion" handler function, close the window. This should result in nsDeviceMotion touching freed memory. (Presumably we could validate this with valgrind.)

I guess this might be easier if the parent page was itself an iframe, so it could just remove itself from the parent document or something instead of relying on being able to close the window.
Comment 4 Doug Turner (:dougt) 2012-03-09 02:20:46 PST
Created attachment 604352 [details] [diff] [review]
patch v.1

I couldn't reproduce any failure with the code as it is.  My test case was to create 100 iframe each using device motion.  In the device motion callback, the iframe would ask the parent document (top) to randomly remove one of the iframes.  The test would complete when all of the iframes were removed.  I ran this test a few hundred times without any failure. 

Ted, is this similar to what you were thinking?


I do not see any harm in making a copy of the listeners before calling out through the callback.  This is what the attach patch does.  We do not need any locking because all of this code is main thread only.
Comment 5 Doug Turner (:dougt) 2012-03-09 02:26:53 PST
i see a few crashes in crash stats that are probably this bug.
Comment 6 Olli Pettay [:smaug] 2012-03-09 03:56:22 PST
Comment on attachment 604352 [details] [diff] [review]
patch v.1


>+  nsCOMArray<nsIDeviceMotionListener> listeners = mListeners;
>+  for (PRUint32 i = listeners.Count(); i > 0 ; ) {
>     --i;
>     nsRefPtr<nsDeviceMotionData> a = new nsDeviceMotionData(type, x, y, z);
>-    mListeners[i]->OnMotionChange(a);
>+    listeners[i]->OnMotionChange(a);
>   }
> 
>-  for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+  nsTArray<nsIDOMWindow*> windowListeners = mWindowListeners;
Nothing keeps the objects in windowListeners alive.
You need to make an nsCOMArray<nsIDOMWindow> or nsTArray<nsCOMPtr<nsIDOMWindow> >
Comment 7 Doug Turner (:dougt) 2012-03-09 06:49:38 PST
okay.  But I am not seeing ~() being called when RemoveWindowListener is called.  Something is keeping the window alive.  Maybe we just don't have the right test case.
Comment 8 Olli Pettay [:smaug] 2012-03-09 07:49:29 PST
I'm sure Jesse or mw22 could create you a testcase where window object is deleted ;)
But since the bug is rather obvious here, I wouldn't spend time for the testcase.
Comment 9 Doug Turner (:dougt) 2012-03-09 08:16:40 PST
Created attachment 604416 [details] [diff] [review]
patch .2

I don't remember why we weren't using nsCOMArray<nsIDOMWindow>.  I think it was a shutdown leak.  I pushed this patch to try.

https://hg.mozilla.org/try/rev/e48f12163d10


Alternatively we can just use a nsCOMArray in nsDeviceMotion::DeviceMotionChanged.
Comment 10 Olli Pettay [:smaug] 2012-03-09 08:19:24 PST
Comment on attachment 604416 [details] [diff] [review]
patch .2

This leads most certainly to leaks
(which lead to huge cycle collection times).

Could you just copy the values in
nsTArray<nsIDOMWindow*> mWindowListeners;
to nsTArray<nsCOMPtr<nsIDOMWindow> > and then iterate that.
Comment 11 Doug Turner (:dougt) 2012-03-09 08:30:01 PST
Created attachment 604419 [details] [diff] [review]
patch v.3
Comment 12 Olli Pettay [:smaug] 2012-03-09 08:34:52 PST
Comment on attachment 604419 [details] [diff] [review]
patch v.3

>-  for (PRUint32 i = mWindowListeners.Length(); i > 0 ; ) {
>+  nsCOMArray<nsIDOMWindow> windowListeners;
>+  for (PRUint32 i = windowListeners.Count(); i > 0 ; ) {
>+      windowListeners.AppendObject(mWindowListeners[i]);
>+  }
2 space indentation, please.
Comment 14 Doug Turner (:dougt) 2012-03-10 11:15:12 PST
https://hg.mozilla.org/mozilla-central/rev/6737b6762eb8
Comment 15 Doug Turner (:dougt) 2012-03-10 20:13:40 PST
Created attachment 604713 [details] [diff] [review]
patch of what landed on m-c
Comment 16 Olli Pettay [:smaug] 2012-03-11 08:01:48 PDT
Argh, I didn't review the commit message. I guess I should have.
We don't want commit messages to say "hey, here is a possible sg:crit" bug. 

Could we take the patch to FF11, if we're going to fix that Pwn2Own bug too.
Comment 17 Doug Turner (:dougt) 2012-03-11 10:53:57 PDT
i think it is pretty obvious to look at csets that have bugs that are marked as Security-Sensitive.  Check-in comments should not lie.
Comment 18 Alex Keybl [:akeybl] 2012-03-12 15:42:55 PDT
Can we get a risk evaluation of this bug before uplifting to Aurora 12, and therefore Beta 12 in a couple of days?
Comment 19 Alex Keybl [:akeybl] 2012-03-14 14:34:42 PDT
Comment on attachment 604713 [details] [diff] [review]
patch of what landed on m-c

[Triage Comment]
Approving this sg:crit fix for Beta 12 since Aurora is now 13.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2012-03-20 10:16:41 PDT
Comment on attachment 604713 [details] [diff] [review]
patch of what landed on m-c

[Triage Comment]
Approving for landing on ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 15:41:57 PDT
qa- until testcase-wanted is satisfied
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-06 23:25:49 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/a288423859c5

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