16832 - xpcom/proxy addref/release problem. was:window.confirm() crashes.

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Doug Turner (:dougt)]

after clicking OK (maybe cancel too) to a window.confirm() invoked either by a
javascript: url, or through the nsIDOMWindow crashes with the following stack
trace:

nsEventQueueImpl::Unlink(nsEventQueueImpl * const 0x02b4f464) line 283 + 12
bytes
nsEventQueueImpl::~nsEventQueueImpl() line 49
nsEventQueueImpl::`scalar deleting destructor'(unsigned int 0x00000001) + 15
bytes
nsEventQueueImpl::Release(nsEventQueueImpl * const 0x02b4f460) line 72 + 129
bytes
EventQueueEntry::RemoveQueue(nsIEventQueue * 0x02b4f460) line 155 + 12 bytes
nsEventQueueServiceImpl::PopThreadEventQueue(nsEventQueueServiceImpl * const
0x00c600f0, nsIEventQueue * 0x02b4f460) line 352
GlobalWindowImpl::OpenInternal(JSContext * 0x01abd780, long * 0x00e3adb0,
unsigned int 0x00000004, int 0x00000001, nsIDOMWindow * * 0x0012c450) line 2163
GlobalWindowImpl::OpenDialog(GlobalWindowImpl * const 0x01abd948, JSContext *
0x01abd780, long * 0x00e3adb0, unsigned int 0x00000004, nsIDOMWindow * *
0x0012c450) line 2029
nsCommonDialogs::DoDialog(nsCommonDialogs * const 0x02b4b910, nsIDOMWindow *
0x01abd948, nsIDialogParamBlock * 0x02b4b7b0, const char * 0x00f359c0) line 296
+ 29 bytes
nsCommonDialogs::Confirm(nsCommonDialogs * const 0x02b4b910, nsIDOMWindow *
0x01abd948, const unsigned short * 0x00000000, const unsigned short *
0x0012c540, int * 0x0012c60c) line 134 + 27 bytes
nsWebShellWindow::Confirm(nsWebShellWindow * const 0x019cccd8, const unsigned
short * 0x0012c540, int * 0x0012c60c) line 2991 + 31 bytes
GlobalWindowImpl::Confirm(GlobalWindowImpl * const 0x02774318, JSContext *
0x027713b0, long * 0x00e7ee2c, unsigned int 0x00000001, int * 0x0012c60c) line
1140 + 35 bytes
WindowConfirm(JSContext * 0x027713b0, JSObject * 0x024232f0, unsigned int
0x00000001, long * 0x00e7ee2c, long * 0x0012c6c8) line 1166 + 31 bytes
js_Invoke(JSContext * 0x027713b0, unsigned int 0x00000001, unsigned int
0x00000000) line 672 + 26 bytes
js_Interpret(JSContext * 0x027713b0, long * 0x0012cf90) line 2248 + 15 bytes
js_Execute(JSContext * 0x027713b0, JSObject * 0x024232f0, JSScript *
0x02b4ba50, JSFunction * 0x00000000, JSStackFrame * 0x00000000, int 0x00000000,
long * 0x0012cf90) line 845 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x027713b0, JSObject * 0x024232f0,
JSPrincipals * 0x010f9aa8, const unsigned short * 0x02b4bb90, unsigned int
0x00000018, const char * 0x00000000, unsigned int 0x00000000, long *
0x0012cf90) line 2638 + 27 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x027742c0, const nsString &
{...}, void * 0x024232f0, nsIPrincipal * 0x010f9aa0, const char * 0x00000000,
unsigned int 0x00000000, nsString & {...}, int * 0x0012d270) line 186 + 53
bytes
nsEvaluateStringProxy::EvaluateString(nsEvaluateStringProxy * const 0x02b4cd30,
nsIScriptContext * 0x027742c0, const char * 0x02b4bd90, void * 0x00000000,
nsIPrincipal * 0x010f9aa0, const char * 0x00000000, int 0x00000000, char * *
0x0012d26c, int * 0x0012d270) line 92 + 50 bytes
XPTC_InvokeByIndex(nsISupports * 0x02b4cd30, unsigned int 0x00000003, unsigned
int 0x00000008, nsXPTCVariant * 0x02b4bc90) line 135
EventHandler(PLEvent * 0x02b4bd40) line 529 + 41 bytes
nsProxyObject::Post(unsigned int 0x00000003, nsXPTMethodInfo * 0x01905560,
nsXPTCMiniVariant * 0x0012d114, nsIInterfaceInfo * 0x02b4cce0) line 243 + 9
bytes
nsProxyEventObject::CallMethod(nsProxyEventObject * const 0x02b4bed0, unsigned
short 0x0003, const nsXPTMethodInfo * 0x01905560, nsXPTCMiniVariant *
0x0012d114) line 279
PrepareAndDispatch(nsXPTCStubBase * 0x02b4bed0, unsigned int 0x00000003,
unsigned int * 0x0012d1c8, unsigned int * 0x0012d1b4) line 96 + 31 bytes
SharedStub() line 121

I am assigning to danm since he is the last to touch nsEventQueue.


To reproduce just type:

javascript:window.confirm('doomed')

into the location bar.

Comment 1

[Scott MacGregor]

I believe this is the current stopper that is really keeping the tree closed.
My linux build works okay except when I dismiss dialogs and i crash with
a stack trace identitcal to this.

I'm trying to back out the event queue changes now to see what we get.

Comment 2

[Scott MacGregor]

Turns out this was a bug in the proxy code that danm tracked down. I checked in
a temporary fix so we can get the tree open.

Doug, nsProxyEvent has a member variable for the queue: mEventQueue which you
changed today to no longer hold an addref'ed reference to. However, the dtor
of this method was still releasing the event queue and this was leading to
crashes later on in the code because we had an extra release here.

I just checked in some code that Dan came up with to take the extra release out
of the dtor.

I'm going to leave this bug open and assigned to you so you can make sure you
want to handle it this way instead of putting the addref's back in for
mEventQueue.

Comment 3

[Doug Turner (:dougt)]

Dan, thanks.  the fix looks good.

Yesterday I move these ownership calls much higher than the nsProxyObject.  I
fixed the constructors but never fixed the destructors.  (shame on me).  I also
need to remove the NS_RELEASE of the of the real object since it only is
addref'ed once and the caller should deal with that one.

Comment 4

[Doug Turner (:dougt)]

also removing an extra release on the real object.  fixes checked in.  thanks
dan.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6cee78f8c5d
[components] Revert "For https://github.com/mozilla-mobile/fenix/issues/16832: Separate facts for addon install and addon enabled" (https://github.com/mozilla-mobile/android-components/pull/9499)