393294 - nsDOMPopupBlockedEvent leaks an nsWeakReference

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Robert Sayre]

Attachment: comptr leak

Pretty much any site that tries to open a popup seems to trigger this leak.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix.

This is untested, but should fix this leak.

Comment 2

[Johnny Stenback (:jst)]

And the attached patch should also make us not leak the nsString members in nsPopupBlockedEvent too.

Comment 3

[Johnny Stenback (:jst)]

While I'm for some reason unable to get gdb to break when we hit this code, I do know for a fact that this code does run (yay for good ol' printf()). Given that, this should fix this leak.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix 2 other possible leaks

nsBeforePageUnloadEvent has an nsString member and nsCommandEvent has an nsCOMPtr<nsIAtom> member.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 277881 [details] [diff] [review]
fix 2 other possible leaks

 class nsDOMCommandEvent : public nsDOMEvent,
                           public nsIDOMCommandEvent
 {
 public:
   nsDOMCommandEvent(nsPresContext* aPresContext,
                     nsCommandEvent* aEvent);
+  ~nsDOMCommandEvent();

Might as well explicitly state here that this is virtual while you're at it.

r+sr+a=jst

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Checked in the patch 2 (added |virtual|)

Comment 7

[jag (Peter Annema)]

 nsDOMBeforeUnloadEvent::~nsDOMBeforeUnloadEvent() 
 {
+  if (mEventIsInternal &&
+      mEvent->eventStructType == NS_BEFORE_PAGE_UNLOAD_EVENT) {
+      delete static_cast<nsBeforePageUnloadEvent*>(mEvent);
+      mEvent = nsnull;
+  }
 }

Nit: incorrectly indented body

Comment 8

[Johnny Stenback (:jst)]

Fix checked in. (and fixed the indentation issue pointed out by jag as well.)

Comment 9

[jag (Peter Annema)]

Heh, you unindented the wrapped part of the |if| condition. I re-indented that for you.

Comment 10

[Johnny Stenback (:jst)]

Duh. Thanks Jag!