Closed Bug 1262110 Opened 6 years ago Closed 6 years ago

Release dock menu on shutdown


(Core :: Widget: Cocoa, defect)

Not set



Tracking Status
firefox48 --- fixed


(Reporter: peterv, Assigned: peterv)




(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
My patch in bug 1251361 triggers a leak in bc tests on OS X, but it's probably an existing leak. I haven't been able to reproduce it locally, so I don't know what makes it show up. CC logs from Tinderbox show this is because the cocoa menu code (in particular the dock menu code) holds on to elements. I looked into hooking it up to the cycle collector, but it's lot of changes (touching non-cocoa widget code too).

Just making nsMacDockSupport (which is a service!) release the dock menu on shutdown fixes the leak too, and is a much smaller change. Let me know if you think I should make all the cocoa widget code CC'ed, or if I should file a followup for that. In theory widgets should all go away on shutdown, but I don't know how likely they are to be involved in cycles.
Attachment #8738104 - Flags: review?(bugs)
Comment on attachment 8738104 [details] [diff] [review]

Ok, nsMacDockSupport (";1") is a service and is kept alive until shutdown. So making it CCable wouldn't really help more than this.
I'd say the leak is FF UI leak, given that it doesn't ever clear dockMenu it sets.

We initialize it in
Could we set it to null in
try {
  let dockSupport = Cc[";1"]
  dockSupport.dockMenu = null;
} catch (e) {}

mDockMenu = do_QueryInterface(aDockMenu, &rv); in nsMacDockSupport::SetDockMenu is silly
since it always sets mDockMenu, yet may cause an exception to be thrown.
Comment 0 sounds related to bug 994371. You get an extra shutdown CC due to some Cocoa stuff holding onto elements. You could try that patch and see if it helps. I didn't bother landing it because I didn't feel like trying to fix all of the extra shutdown CCs.
Hmm, it sounds like I didn't get that patch fully working. But still, it is something to consider.
Comment on attachment 8738104 [details] [diff] [review]

So I can live with this approach too, but I'd prefer fixing this in browser.js if possible.
Attachment #8738104 - Flags: review?(bugs) → review+
Yeah, I pushed a fix in browser.js to try, but for some reason the tests didn't run yet.
Attachment #8738104 - Attachment is obsolete: true
Attachment #8739040 - Flags: review?(bugs) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.