Closed Bug 642717 Opened 14 years ago Closed 14 years ago

Refresh driver needs to hold strong refs to |targets| in Notify

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla5
Tracking Flags:
Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed
status1.9.2 --- unaffected
status1.9.1 --- unaffected
fennec - ---

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Whiteboard: [sg:critical?])

Attachments

(1 file)

Hold strong references to our MozBeforePaint event targets.
1.90 KB, patch
roc
: review+
dveditz
: approval2.0+
Details | Diff | Splinter Review 
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.
Whiteboard: [need review]
Priority: -- → P1
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)?
Keywords: testcase-wanted
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]
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → ?
Pushed http://hg.mozilla.org/mozilla-central/rev/ced2b9373c5f
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
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: - → ?
blocking2.0: ? → Macaw
status2.0: ? → wanted
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+
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: