494060 - JavaScript Error: "console.targetWindow is null" in venkman-debugger.js, line 311

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Karsten Düsterloh]

Seen with current SeaMonkey trunk:
- Activate exception tracing inside Venkman.
- Restart SeaMonkey with -browser -venkman.
During startup, the JS module file XPCOMUtils.jsm is loaded, but - of course - has no target window, causing:

vnk: execution hook: function XPCOMUtils_QueryInterface(iid=XPComponent:{7}) in <file:/home/kd/projekte/mozilla/mozilla.org/obj/sr/mozilla/dist/bin/modules/XPCOMUtils.jsm> line 265
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "console.targetWindow is null" {file: "chrome://venkman/content/venkman-debugger.js" line: 311}]' when calling method: [jsdIExecutionHook::onExecute]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///home/kd/projekte/mozilla/mozilla.org/obj/sr/mozilla/dist/bin/modules/XPCOMUtils.jsm :: XPCOMUtils_QueryInterface :: line 265"  data: yes]
************************************************************

Comment 1

[Karsten Düsterloh]

Attachment: accept that JS modules don't need a window

This patch takes care of the fact that JS modules usually neither belong to windows nor do they need a wrapper...

Comment 2

[:Gijs (he/him)]

Comment on attachment 378727 [details] [diff] [review]
accept that JS modules don't need a window

>diff --git a/resources/content/venkman-debugger.js b/resources/content/venkman-debugger.js
>--- a/resources/content/venkman-debugger.js
>+++ b/resources/content/venkman-debugger.js
>@@ -301,38 +301,39 @@ function jsdExecutionHook (frame, type, 
>     console.baseWindow.enabled = true;
>     
>     if (!ASSERT(cx, "no cx in execution hook"))
>         return hookReturn;
>     
>     var glob = cx.globalObject;
>     if (!ASSERT(glob, "no glob in execution hook"))
>         return hookReturn;
>-    
>+
>     console.targetWindow = getBaseWindowFromWindow(glob.getWrappedValue());
>-    targetWasEnabled = console.targetWindow.enabled;
>-    if (console.targetWindow != console.baseWindow)
>+    if (console.targetWindow) // JS modules usually don't have a target window
>     {
>-        cx.scriptsEnabled = false;
>-        console.targetWindow.enabled = false;
>+        targetWasEnabled = console.targetWindow.enabled;
>+        if (console.targetWindow != console.baseWindow)
>+        {
>+            cx.scriptsEnabled = false;
>+            console.targetWindow.enabled = false;
>+        }
Are you sure leaving cx.scriptsEnabled alone if there is no targetWindow is the Right Thing to do? I am not sure if it is...


>diff --git a/resources/content/venkman-records.js b/resources/content/venkman-records.js
>--- a/resources/content/venkman-records.js
>+++ b/resources/content/venkman-records.js
>@@ -161,17 +161,19 @@ function FrameRecord (jsdFrame)
> 
>     this.functionName = jsdFrame.functionName;
>     if (!jsdFrame.isNative)
>     {
>         this.scriptWrapper = getScriptWrapper(jsdFrame.script);
>         this.location = getMsg(MSN_FMT_FRAME_LOCATION,
>                                [getFileFromPath(jsdFrame.script.fileName),
>                                 jsdFrame.line, jsdFrame.pc]);
>-        this.functionName = this.scriptWrapper.functionName;
>+        // JS modules usually don't have a scriptWrapper
>+        if (this.scriptWrapper)
>+            this.functionName = this.scriptWrapper.functionName;

We should probably set a different name if there is no scriptWrapper. We already use a custom name for eval scripts, see http://hg.mozilla.org/venkman/file/f27ea2ff7bf3/resources/content/venkman-debugger.js#l477 . Iff this.scriptWrapper is only null for jsmodules here (something I wouldn't like to assume, but there we go...) then we should reflect that this is a jsmodule context in this special name. Also, as far as I know it's a bug that JS modules don't have a scriptwrapper, though I might be wrong there...

r=me with those fixed / explained to me :-).

Comment 3

[Karsten Düsterloh]

Attachment: accept windowless scripts, v2

(In reply to comment #2)
> Are you sure leaving cx.scriptsEnabled alone if there is no targetWindow
> is the Right Thing to do?

Actually, no.
I also took the liberty to comment on what is happening in jsdExecutionHook and did some minor whitespace cleanup in this function. 

> >+++ b/resources/content/venkman-records.js
> >@@ -161,17 +161,19 @@ function FrameRecord (jsdFrame)

Btw: I needed to patch the function, because otherwise I'll die here when setting exception handling to "stop".

> Also, as far as I know it's a bug that JS modules don't have a
> scriptwrapper, though I might be wrong there...

The basic scriptwrapper problem doesn't seem to be the module as such (there are in fact .jsm with a scriptwrapper!), but the special code construct in XPCOMUtils.jsm: 

function makeQI(interfaceNames) {
  return function XPCOMUtils_QueryInterface(iid) {
    if (iid.equals(Ci.nsISupports))
      return this;
    for each(let interfaceName in interfaceNames) {
      if (Ci[interfaceName].equals(iid))
        return this;
    }
    throw Cr.NS_ERROR_NO_INTERFACE;
  };
}

I don't get a call to jsdIScriptHook.onScriptCreate for the stack-created function XPCOMUtils_QueryInterface, thus we don't create a scriptwrapper.
(Since you seem to know this bug, which one is it? I couldn't find it.)

As a workaround, we maybe could create a scriptwrapper for yet-unknown scripts inside getScriptWrapper, but this doesn't feel quite right.

So I just decided to wallpaper the error here; the old comment was wrong, of course.

Comment 4

[:Gijs (he/him)]

(In reply to comment #3)
> > Also, as far as I know it's a bug that JS modules don't have a
> > scriptwrapper, though I might be wrong there...
> 
> The basic scriptwrapper problem doesn't seem to be the module as such (there
> are in fact .jsm with a scriptwrapper!), but the special code construct in
> XPCOMUtils.jsm: 
> 
> function makeQI(interfaceNames) {
>   return function XPCOMUtils_QueryInterface(iid) {
>     if (iid.equals(Ci.nsISupports))
>       return this;
>     for each(let interfaceName in interfaceNames) {
>       if (Ci[interfaceName].equals(iid))
>         return this;
>     }
>     throw Cr.NS_ERROR_NO_INTERFACE;
>   };
> }
> 
> I don't get a call to jsdIScriptHook.onScriptCreate for the stack-created
> function XPCOMUtils_QueryInterface, thus we don't create a scriptwrapper.
> (Since you seem to know this bug, which one is it? I couldn't find it.)

There is bug 459996, but it doesn't really have that much detail. I put two and two together and presumed from the fix and that bug that (part of) the problem was a lack of scriptwrappers for JSMs.

I will have to have a look at the patch at a later time, sorry (will try to make it soon though! :-) ).

Comment 5

[:Gijs (he/him)]

Comment on attachment 381766 [details] [diff] [review]
accept windowless scripts, v2

This looks good, also like the comments. Thanks! r=me

Comment 6

[Karsten Düsterloh]

Landed as <http://hg.mozilla.org/venkman/rev/06ea5135b7f3>.

Comment 8

[Lo]

Bug 418426 is still reproducible with this patch. The call to cx.scriptsEnabled = false throws an exception:

[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERF
ACE) [jsdIContext.scriptsEnabled]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  loc
ation: "JS frame :: chrome://venkman/content/venkman-debugger.js :: jsdExecution
Hook :: line 323"  data: no]

Comment 9

[alta88]

is there a released version with this fix?  it's not on AMO nor can i find a dev build anywhere in the (seemingly unmaintained) venkman pages.