DebuggerScript_getUrl crashes if script->filename is null

RESOLVED FIXED in mozilla15



7 years ago
7 years ago


(Reporter: jorendorff, Assigned: jorendorff)


Other Branch

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [chrome-debug])


(1 attachment)



7 years ago
This definitely happens in the browser. We need a shell function that creates functions with null filenames in order to test it.


7 years ago
Whiteboard: [chrome-debug]

Comment 1

7 years ago
So I can't tell if we're actually supposed to support NULL filenames or not. The JSAPI documentation doesn't really say, but we don't JS_ASSERT(filename), and if we did I think we'd break Gecko.

So we probably should support NULL filenames, but there are no tests and in fact if I make it testable then I immediately get weird behavior:

js> evaluate("throw new Error()", {fileName: null});

The error is never thrown. In fact creating the error object mysteriously fails, because of this code in jsexn.cpp:Exception():

669	            filename = FilenameToString(cx, iter.script()->filename);
670	            if (!filename)
671	                return false;

So basically this is a mess.
Assignee: general → nobody
Component: JavaScript Engine → js-ctypes
QA Contact: general → js-ctypes
Target Milestone: --- → mozilla14
Version: Other Branch → 15 Branch

Comment 2

7 years ago
oh for the love of Mike
Assignee: nobody → general
Component: js-ctypes → JavaScript Engine
QA Contact: js-ctypes → general
Target Milestone: mozilla14 → ---
Version: 15 Branch → Other Branch

Comment 3

7 years ago
OK, we definitely have null-filename scripts running around, because the patch in bug 738480 causes us to segfault when browser/devtools/commandline/test/browser_gcli_break.js tries to examine them.

Better to support this everywhere than to ban it... I guess? Any other JSAPI people care?
Blocks: 738480


7 years ago
Assignee: general → jorendorff

Comment 4

7 years ago
Created attachment 626690 [details] [diff] [review]

This is gross. I'm convinced we should do away with null filenames, but let's stop the crashes first and do the API change in a different bug.
Attachment #626690 - Flags: review?(jimb)

Comment 5

7 years ago
Comment on attachment 626690 [details] [diff] [review]

Review of attachment 626690 [details] [diff] [review]:

Looks great. One semi-nit; only change if you agree it's clearer.

::: js/src/jsexn.cpp
@@ +563,5 @@
>              return false;
>          args[1].setString(filename);
>      } else {
> +        const char *cfilename;
> +        if (!iter.done() && (cfilename = iter.script()->filename)) {

nit: Assignment operators in 'if' conditions aren't very legible. Could we perhaps:

filename = cx->runtime->emptyString;
if (!iter.done()) {
  const char *cfilename = iter.script()->filename;
  if (cfilename) {
    filename = FilenameToString(cx, cfilename);
    if (!filename)
      return false;
Attachment #626690 - Flags: review?(jimb) → review+
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.