Closed Bug 642717 Opened 9 years ago Closed 9 years ago
Refresh driver needs to hold strong refs to |targets| in Notify
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.
Attachment #520121 - Flags: review?(roc) → review+
Comment on attachment 520121 [details] [diff] [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...
Attachment #520121 - Flags: approval2.0?
Whiteboard: [need review] → [need approval]
blocking2.0: ? → -
tracking-fennec: --- → ?
we can pick this up whenever it lands for desktop. not holding mobile4.0 for this.
tracking-fennec: ? → 2.0-
It sounds bad in theory, but do we know if this happens in practice (for example, is there a testcase that demonstrates the problem)?
Whiteboard: [need approval] → [sg:critical?][need approval]
I haven't tried writing one, but I could do that if you want...
Daniel, see comment 5.
Whiteboard: [sg:critical?][need approval] → [sg:critical?][need landing]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][need landing] → [sg:critical?]
Target Milestone: --- → mozilla2.2
I really think we need to take this for any security-update releases we do for 2.0. Renominating accordingly.
blocking2.0: - → ?
Comment on attachment 520121 [details] [diff] [review] Hold strong references to our MozBeforePaint event targets. Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #520121 - Flags: approval2.0? → approval2.0+
You need to log in before you can comment on or make changes to this bug.