793433 - crash in nsAppShellService::RegisterTopLevelWindow

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses specialpowers)

This bug was filed from the Socorro interface and is 
report bp-f7e12a47-474d-4609-9063-f7d722120922 .
============================================================= 
0 	xul.dll 	nsAppShellService::RegisterTopLevelWindow 	xpfe/appshell/src/nsAppShellService.cpp:457
1 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:70
2 	xul.dll 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:2405
3 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1469
4 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367
5 	mozjs.dll 	js::Invoke 	js/src/jsinterp.h:109
6 	mozjs.dll 	js_fun_apply 	js/src/jsfun.cpp:951
7 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367
8 	mozjs.dll 	js::Invoke 	js/src/jsinterp.h:109
9 	mozjs.dll 	js_fun_call 	js/src/jsfun.cpp:839
10 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367

The code that's causing the crash was touched in bug 774633.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like domWindow is coming back null?

Comment 2

[Bobby Holley (:bholley)]

I mean, the testcase is:

  appService = SpecialPowers.wrap(Components).classes['@mozilla.org/appshell/appShellService;1'].
       getService(Components.interfaces.nsIAppShellService). registerTopLevelWindow({});

Where the argument here is supposed to be an nsIXULWindow. So we're purposely passing garbage to a privileged function.

Martijn, is this something that people do? I'm happy to just stick a null check in there, but on face my reaction to this bug is "well, don't do that".

Comment 3

[Josh Matthews [:jdm]]

Non-critical null-deref? Sounds like easy fodder for a new contributor to me. http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsAppShellService.cpp is the file to look at.

Comment 4

[bhavana bajaj [:bajaj]]

Assinging it to :Bholley, feel free to reassign if needed.

Comment 5

[Bobby Holley (:bholley)]

It's not clear to me why we're tracking this. I don't think it's a really big deal, and think it would be a good bug for a new contributor.

Comment 6

[foudfou]

I'll be happy to work on it.

Comment 7

[foudfou]

Attachment: patch v0

Bobby told me on IRC that the test should probably be put into xpconnect/ instead of appshell/ because:
- build system maintainers have a manually-written list of test directories for the android test boards
- the appshell code probably won't be touched much in the future

Comment 8

[Bobby Holley (:bholley)]

Note - XPConnect chrome tests should go in tests/chrome, not tests/mochitest.

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 669160 [details] [diff] [review]
patch v0

Review of attachment 669160 [details] [diff] [review]:
-----------------------------------------------------------------

I have no issues with this patch, but this isn't code that I own. Over to Johnny.

Comment 10

[foudfou]

Attachment: patch v1 -- test in tests/chrome

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 669166 [details] [diff] [review]
patch v1 -- test in tests/chrome

Stealing this review from Johnny.

>diff --git a/js/xpconnect/tests/chrome/Makefile.in b/js/xpconnect/tests/chrome/Makefile.in
>--- a/js/xpconnect/tests/chrome/Makefile.in
>+++ b/js/xpconnect/tests/chrome/Makefile.in
>@@ -60,6 +60,7 @@
> 		test_weakref.xul \
> 		test_wrappers.xul \
> 		test_bug720619.xul \
>+		test_bug793433.xul \

Please place this in appropriate sorted order. Bonus points for fixing the one above it while you're at it.



>diff --git a/js/xpconnect/tests/chrome/test_bug793433.xul b/js/xpconnect/tests/chrome/test_bug793433.xul

>+SimpleTest.waitForExplicitFinish();

There's nothing asynchronous here, so I think you should be fine without the SimpleTest.waitForExplicitFinish() and the SimpleTest.finish(). 

>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+
>+function shouldThrowException(fun, args, exception) {
>+  try {
>+    fun.apply(this, args);
>+    return false;
>+  } catch (e) {
>+    $("display").innerHTML += "<br/>OK thrown: "+e.message;
>+    return (e instanceof Components.Exception &&
>+            e.result === exception)
>+  }
>+}

Kudos for the precise testing here. However, I'm not sure it's really worth raising a failure if someone changes the code to return NS_ERROR_FAILURE instead of of NS_ERROR_INVALID_POINTER. We really just want to make sure that we throw something and don't crash.

Also, rather than mucking around with $("display") and such, I think it would be better to just embed the ok() calls directly in this function.  So something like:

function shouldThrowException(fun, args) {
  try {
    fun.apply(this, args);
    ok(false, "Didn't throw!");
  } catch (e) {
    ok(true, "Correctly threw: " + e);
  }
}

Looks great in general! r=bholley with those changes.

Comment 12

[foudfou]

Attachment: patch v2 -- addressing reviewer's comments

Thank you for the careful review!

Comment 13

[foudfou]

Attachment: patch v2 -- addressing reviewer's comments [correct file]

Oops, correct patch attached.

Comment 14

[Matthew N. [:MattN]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9895d8cf22b2
I'll push to inbound later if it's green.

Comment 15

[Matthew N. [:MattN]]

Thanks.
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/25a66981bdb0

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/25a66981bdb0

Comment 17

[Robert Kaiser]

This is marked trracking-firefox18+, are you planning to get this into Aurora as well?

Comment 18

[Josh Matthews [:jdm]]

I don't think that's necessary. This is really only present in misused test-only code.

Comment 19

[Robert Kaiser]

(In reply to Josh Matthews [:jdm] from comment #18)
> I don't think that's necessary. This is really only present in misused
> test-only code.

In that case, let's take it off the tracking radar.