Last Comment Bug 642717 - Refresh driver needs to hold strong refs to |targets| in Notify
: Refresh driver needs to hold strong refs to |targets| in Notify
Status: RESOLVED FIXED
: [sg:critical?]
: testcase-wanted
Product: Core
Classification: Components
Component: Layout
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla5
Assigned To: Boris Zbarsky (:bz)
: layout
:
:
:
  Show dependency treegraph
 
Reported: 2011-03-17 20:36 PDT by Boris Zbarsky (:bz)
Modified: 2011-05-24 23:35 PDT (History)
8 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  -
  Macaw+
  .1-fixed
  ---
  unaffected
  ---
  unaffected


Attachments
Hold strong references to our MozBeforePaint event targets. (1.90 KB, patch)
2011-03-17 20:38 PDT, Boris Zbarsky (:bz)
roc: review+
dveditz: approval2.0+
Details | Diff | Splinter Review

Summon comment box

Description Boris Zbarsky (:bz) 2011-03-17 20:36:58 PDT
I came across this while working on bug 607529 (and the patch for that bug includes a fix for this one, but that bug is not landing in fx4).

I believe that the current code in Notify is unsafe.  |targets| holds weak references, and since they're on the stack they can't be removed when the documents go away.  So if one of the events triggers script that ends up collecting some of the other documents, then we could end up doing event dispatch to a dead event target, which seems bad.

I'll attach a minimal fix here.  I don't think this is worth respinning for, but I do think we should take this ASAP once we start taking .x fixes.
Comment 1 Boris Zbarsky (:bz) 2011-03-17 20:38:39 PDT
Created attachment 520121 [details] [review]
Hold strong references to our MozBeforePaint event targets.
Comment 2 Boris Zbarsky (:bz) 2011-03-17 21:15:56 PDT
Comment on attachment 520121 [details] [review]
Hold strong references to our MozBeforePaint event targets.

Requesting approval, in case we want to fix this for mobile even without fixing it for desktop...
Comment 3 Doug Turner (:dougt) 2011-03-18 09:46:17 PDT
we can pick this up whenever it lands for desktop.  not holding mobile4.0 for this.
Comment 4 Daniel Veditz 2011-03-24 13:26:49 PDT
It sounds bad in theory, but do we know if this happens in practice (for example, is there a testcase that demonstrates the problem)?
Comment 5 Boris Zbarsky (:bz) 2011-03-24 13:30:30 PDT
I haven't tried writing one, but I could do that if you want...
Comment 6 Boris Zbarsky (:bz) 2011-03-24 13:30:46 PDT
Daniel, see comment 5.
Comment 7 Boris Zbarsky (:bz) 2011-03-28 11:51:41 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/ced2b9373c5f
Comment 8 Boris Zbarsky (:bz) 2011-03-28 22:14:36 PDT
I really think we need to take this for any security-update releases we do for 2.0.  Renominating accordingly.
Comment 9 Daniel Veditz 2011-04-01 11:04:22 PDT
Comment on attachment 520121 [details] [review]
Hold strong references to our MozBeforePaint event targets.

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 10 Boris Zbarsky (:bz) 2011-04-01 13:01:35 PDT
http://hg.mozilla.org/releases/mozilla-2.0/rev/7bf6e15028ed

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