Closed Bug 793433 Opened 7 years ago Closed 7 years ago

crash in nsAppShellService::RegisterTopLevelWindow


(Core :: XPConnect, defect, critical)

Windows NT
Not set





(Reporter: martijn.martijn, Assigned: foudil.newbie+bmo)



(Keywords: crash, regression, testcase, Whiteboard: [mentor=jdm][lang=c++])

Crash Data


(2 files, 3 obsolete files)

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.
Looks like domWindow is coming back null?
Keywords: regression
I mean, the testcase is:

  appService = SpecialPowers.wrap(Components).classes[';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".
Non-critical null-deref? Sounds like easy fodder for a new contributor to me. is the file to look at.
Whiteboard: [mentor=jdm][lang=c++]
Assinging it to :Bholley, feel free to reassign if needed.
Assignee: nobody → bobbyholley+bmo
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.
Assignee: bobbyholley+bmo → nobody
I'll be happy to work on it.
Assignee: nobody →
Attached patch patch v0 (obsolete) — Splinter Review
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
Attachment #669160 - Flags: review?(josh)
Note - XPConnect chrome tests should go in tests/chrome, not tests/mochitest.
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.
Attachment #669160 - Flags: review?(jst)
Attachment #669160 - Flags: review?(josh)
Attachment #669160 - Flags: feedback+
Attached patch patch v1 -- test in tests/chrome (obsolete) — Splinter Review
Attachment #669160 - Attachment is obsolete: true
Attachment #669160 - Flags: review?(jst)
Attachment #669166 - Flags: review?(jst)
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/ b/js/xpconnect/tests/chrome/
>--- a/js/xpconnect/tests/chrome/
>+++ b/js/xpconnect/tests/chrome/
>@@ -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


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.
Attachment #669166 - Flags: review?(jst) → review+
Thank you for the careful review!
Attachment #669166 - Attachment is obsolete: true
Oops, correct patch attached.
Attachment #669457 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed to try:
I'll push to inbound later if it's green.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This is marked trracking-firefox18+, are you planning to get this into Aurora as well?
I don't think that's necessary. This is really only present in misused test-only code.
(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.
You need to log in before you can comment on or make changes to this bug.