196241 - Assertion failure: cx->requestDepth > 0, at /mnt/ibm/mozhack/mozilla/js/src/jsapi.c:783

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[timeless]

This only crashes in debug builds. I've reproduced it
on boffo (linux-qt-tinderbox-tip)
and raistlin (windows-vc6-xpchacker-preDarin)

run xpcshell.
enter this:

Components.classes["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPrefService).savePrefFile({})

#4  0x400ba9b1 in JS_Assert (s=0x400bf834 "cx->requestDepth > 0",
    file=0x400bce00 "/mnt/ibm/mozhack/mozilla/js/src/jsapi.c", ln=783)
    at /mnt/ibm/mozhack/mozilla/js/src/jsutil.c:173
#5  0x40028dc0 in JS_EndRequest (cx=0x8090fc8) at
/mnt/ibm/mozhack/mozilla/js/src/jsapi.c:783
#6  0x4053d763 in XPCCallContext::~XPCCallContext (this=0xbfffd1a4, __in_chrg=2)
    at /mnt/ibm/mozhack/mozilla/js/src/xpconnect/src/xpccallcontext.cpp:325
#7  0x4057176b in nsXPCWrappedJSClass::CallMethod (this=0x8146c30,
wrapper=0x80d30f8, methodIndex=9,
    info=0x8152690, nativeParams=0xbfffd35c)
    at /mnt/ibm/mozhack/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1583
#8  0x405694e1 in nsXPCWrappedJS::CallMethod (this=0x80d30f8, methodIndex=9,
info=0x8152690, params=0xbfffd35c)
    at /mnt/ibm/mozhack/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:428
#9  0x402178f3 in PrepareAndDispatch (methodIndex=9, self=0x80d30f8,
args=0xbfffd3f4)
    at
/mnt/ibm/mozhack/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:100
#10 0x40656b2b in nsSafeSaveFile::nsSafeSaveFile (this=0xbfffd544,
aTargetFile=0x80d30f8, aNumBackupCopies=1)
    at /mnt/ibm/mozhack/mozilla/modules/libpref/src/nsSafeSaveFile.cpp:58
#11 0x4064ff19 in nsPrefService::WritePrefFile (this=0x819baf8, aFile=0x80d30f8)
    at /mnt/ibm/mozhack/mozilla/modules/libpref/src/nsPrefService.cpp:371
#12 0x4064f655 in nsPrefService::SavePrefFile (this=0x819baf8, aFile=0x80d30f8)
    at /mnt/ibm/mozhack/mozilla/modules/libpref/src/nsPrefService.cpp:232
#13 0x40644a6b in nsPref::SavePrefFile (this=0x819baa8, aFile=0x80d30f8)
    at /mnt/ibm/mozhack/mozilla/modules/libpref/src/nsPref.cpp:80

Comment 1

[timeless]

oh, right this is probably important:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "exists"'
when calling method: [nsIFile::exists]"  nsresult: "0x80570030
(NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: typein ::
<TOP_LEVEL> :: line 1"  data: no]
************************************************************

Comment 2

[timeless]

brendan suggests i give this to dbradley :) 
requestCount logging at each ++ and -- will show the prob

Comment 3

[David Bradley]

Attachment: AutoScriptEvaluate

An errant error path. This should do the trick and hopefully guard against
potential future problems. While the JS_SetErrorReporter wasn't an issue, this
matched up with the script eval code as well, so I moved it as well.

Comment 4

[timeless]

Comment on attachment 116518 [details] [diff] [review]
AutoScriptEvaluate

don't make these changes:
> +++ xpcprivate.h	7 Mar 2003 04:57:48 -0000
> @@ -129,7 +129,7 @@
> @@ -1643,7 +1643,7 @@
you somehow managed to get your files out of sync with cvs...

>@@ -3071,7 +3071,86 @@
imo this form is silly:
>+            nsISupports *supports =
>+                (JS_GetOptions(mJSContext) & JSOPTION_PRIVATE_IS_NSISUPPORTS)
>+                ? NS_STATIC_CAST(nsISupports*, JS_GetContextPrivate(mJSContext))
>+                : nsnull;
>+            if(supports)
>+            {
>+                nsCOMPtr<nsIXPCScriptNotify> scriptNotify = 
>+                    do_QueryInterface(supports);
>+                if(scriptNotify)
>+                    scriptNotify->ScriptExecuted();
>+            }
you could just as easily do this:
if (JS_GetOptions(mJSContext) & JSOPTION_PRIVATE_IS_NSISUPPORTS)
{
  nsCOMPtr<nsIXPCScriptNotify> scriptNotify = do_QueryInterface(
    NS_STATIC_CAST(nsISupports*, JS_GetContextPrivate(mJSContext)));
  if (scriptNotify)
    scriptNotify->ScriptExecuted();
}
but that's just a personal thing

otherwise, the patch fixes my problem and makes things cleaner :)

Comment 5

[David Bradley]

Attachment: AutoScriptEvaluate2

Ok, proper diff now. I'm going to leave the code as is. There's a potential
perf hit by changing it. Since the code as is, doesn't go through and build an
nsCOMPtr and do all the QI stuff.

I also added a tracking bool to get a slight perf boost during destruction. It
now just checks JS_GetContextThread once instead of twice. It should not
change, else we'll have balancing issues again.

Comment 6

[David Bradley]

Comment on attachment 116556 [details] [diff] [review]
AutoScriptEvaluate2

There are a couple of JS issues to think about. Mainly there are slight changes
in the ordering, I'm pretty sure this shouldn't change anything, but a
second/third opinion would be good. Specifically the JS_SetErrorReporter and
JS_EndRequest calling points

Comment 7

[David Bradley]

Comment on attachment 116556 [details] [diff] [review]
AutoScriptEvaluate2

New patch coming up

Comment 8

[David Bradley]

Attachment: AutoScriptEvaluated v3

Addresses the issue of Evaluated being called more than once. Moves the inline
functions out they're really too big and too complex. Timeless checked and the
flag and JS_GetContextPrivate go hand in hand so it's not a performance issue.
And some minor cleanup.

Comment 9

[David Bradley]

Comment on attachment 116577 [details] [diff] [review]
AutoScriptEvaluated v3

Third times a charm? See previous comment for details on changes.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 116577 [details] [diff] [review]
AutoScriptEvaluated v3

>+    /**
>+     * Does the pre script evaluation and sets the error reporter if given
>+     * This function should only be called once, and will assert if called
>+     * more than once
>+     * @param errorReporter the error reporter callback function to set
>+     */
>+    void Evaluate(JSErrorReporter errorReporter = nsnull);

Nit: seems like a misnomer because there's nothing being Evaluated here -- how
about PreEvaluate, or PrepareToEvaluate, or StartEvaluating?  I like
StartEvaluating best, for now.

Nit: how about a blank line between major-doc-commented members?

>+    /**
>+     * Does the post script evaluation and resets the error reporter
>+     */
>+    ~AutoScriptEvaluated();
>+private:
>+    JSContext* mJSContext;
>+    JSExceptionState* mState;
>+    JSErrorReporter mOldErrorReporter;
>+    PRBool mEvaluated;
>+    PRBool mContextHasThread;

Nit: this is set from JS_GetContextThread, which returns an intN (for some
bogus reason, should be a jsword!  Oh well).  When setting, how about changing
from:

>+    mContextHasThread = JS_GetContextThread(mJSContext);

to

>+    mContextHasThread = JS_GetContextThread(mJSContext) != 0;

Or else changing mContextHasThread to be jsword mContextThread?  I like the
latter better.	I'll fix the JS API type botch, if possible (binary
compatibility break when using jsapi.h from C++ -- horrors!).

/be

Comment 11

[David Bradley]

Attachment: V4

I also changed the class name slightly. Dropped the 'd' in addition to
Brendan's comments. Also take a little closer look, as cleaned my tree and then
went to reapply the patch. The patch utility crashed and I had to manually
apply the changes to xpcwrappedjsclass.cpp file. I compared the two patches and
I'm reasonably sure I got everything, but always good to have a double check.

Comment 12

[timeless]

Comment on attachment 116766 [details] [diff] [review]
V4

yeah, this class name is better.

     // XXX we should install an error reporter that will sent reports to

sent=>send

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 116766 [details] [diff] [review]
V4

sr=[e-mail].

/be

Comment 14

[David Bradley]

Patch checked in

Comment 15

[Phil Schwartau]

To benchmark the fix I tried two different debug builds from dates
BEFORE David's checkin. This is what I get from the WinNT xpcshell
when I try timeless' testcase above:

MOZILLA DEBUG BUILD 2003-02-17
MOZILLA DEBUG BUILD 2003-03-11
js>Components.classes["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPrefService).savePrefFile({})
WARNING: Trying to set p, file d:/mozilla/modules/libpref/src/prefapi.cpp, line 1017

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "exists"'
when calling method: [nsIFile::exists]"  nsresult
: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS
frame :: typein :: <TOP_LEVEL> :: line 1"  data: n
o]
************************************************************
uncaught exception: [Exception... "Component returned failure code: 0x80004002
(NS_NOINTERFACE) [nsIPrefService.savePrefFile]"
  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: typein ::
<TOP_LEVEL> :: line 1"  data: no]


I will pull a debug build tonight and try again tomorrow to see
the effect of the fix. Am I missing something, or is this right?

The reason I ask is, I do NOT crash on the testcase in either of
the builds listed above. I know that timeless was crashing on it,
and I'm wondering if I'm missing something here -

Comment 16

[David Bradley]

When I run, I get the complaint about "exists". But then I get an assert, about
accessing an nsCOMPtr that is null. You should see the nsCOMPtr assertion and
then crash. And I think repulled sometime today (3/12). Let me know if you see
some other behavior. The crash is expected, though.

Comment 17

[David Bradley]

The crash is sporadic, since mTargetFileExists isn't initialized. And the code
returns early if it happens to be zero.

Comment 18

[Phil Schwartau]

David: thanks. It's good to know the crash is sporadic, since I
was never able to reproduce it...

I pulled and build the browser overnight, after the fix was checked in,
and ran timeless' test again:

MOZILLA DEBUG BUILD 2003-03-12
js>
Components.classes["@mozilla.org/preferences;1"].getService(Components.interfaces.nsIPrefService).savePrefFile({})
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "exists"'
when calling method: [nsIFile::exists]"  nsresul
: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS
frame :: typein :: <TOP_LEVEL> :: line 1"  data:
o]
************************************************************
uncaught exception: [Exception... "Component returned failure code: 0x80004002
(NS_NOINTERFACE) [nsIPrefService.savePrefFile]
  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: typein ::
<TOP_LEVEL> :: line 1"  data: no]



The only thing that's different from my previous results (Comment #15)
is that I no longer get the warning

WARNING: Trying to set p, file d:/mozilla/modules/libpref/src/prefapi.cpp,
                          line 1017


I'm going to provisionally mark this bug Verified because I'm not
crashing. But I would love to hear from timeless that the problem
seems fixed to him, since I never crashed on this to begin with -

Comment 19

[timeless]

Well, i've since committed the fix for the preference half of the bug, so the
crashes should all be gone.  I suppose we could make a regression test component
which is intentionally broken, but I'm not up to it, sorry.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

>is that I no longer get the warning

Yup, that warning is supposed to be gone now, because the prefs.js entry that
caused it has since been removed.