Last Comment Bug 738468 - DebuggerScript_getUrl crashes if script->filename is null
: DebuggerScript_getUrl crashes if script->filename is null
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks: 738480
  Show dependency treegraph
 
Reported: 2012-03-22 14:53 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-05-31 06:30 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.96 KB, patch)
2012-05-23 21:16 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2012-03-22 14:53:42 PDT
This definitely happens in the browser. We need a shell function that creates functions with null filenames in order to test it.
Comment 1 Jason Orendorff [:jorendorff] 2012-05-15 13:02:01 PDT
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});
js>

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.
Comment 2 Jason Orendorff [:jorendorff] 2012-05-15 13:03:06 PDT
oh for the love of Mike
Comment 3 Jason Orendorff [:jorendorff] 2012-05-23 15:42:00 PDT
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?
Comment 4 Jason Orendorff [:jorendorff] 2012-05-23 21:16:24 PDT
Created attachment 626690 [details] [diff] [review]
v1

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.
Comment 5 Jim Blandy :jimb 2012-05-25 14:29:45 PDT
Comment on attachment 626690 [details] [diff] [review]
v1

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;
  }
}
Comment 6 Jason Orendorff [:jorendorff] 2012-05-30 13:21:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a18a93721d
Comment 7 Ed Morley [:emorley] 2012-05-31 06:30:15 PDT
https://hg.mozilla.org/mozilla-central/rev/29a18a93721d

Note You need to log in before you can comment on or make changes to this bug.