[jsdbg2] |Debugger.Source.prototype.displayURL| is not available within the |Debugger.onNewScript| hook for |new Function| scripts

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Honza, Assigned: Simon Lindholm)

Tracking

unspecified
mozilla35
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Debugger.script.source.displayURL isn't available for new Function scripts with
//# sourceURL directive when accessing it within Debugger.onNewScript hook.

It looks like it's being set asynchronously since it's properly set when testing the property again after timeout.

Note that it's properly set (available within the onNewScript hook) for eval scripts.

Also discussed here:
https://groups.google.com/forum/?hl=en#!topic/mozilla.dev.platform/nvdJeI_HV7g

Honza
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Summary: displayURL is not available within Debugger.onNewScript hook for new Function scripts → [jsdbg2] |Debugger.Source.prototype.displayURL| is not available within the |Debugger.onNewScript| hook for |new Function| scripts

Comment 1

3 years ago
That's terrible.

Comment 2

3 years ago
var g = newGlobal();
var dbg = new Debugger;
var gDO = dbg.addDebuggee(g);

dbg.onNewScript = function (script) {
  print("in onNewScript: " + script.source.displayURL);
};
g.evaluate('//# sourceURL=munge.js\n' +
           'print("about to call Function constructor");\n' +
           'f = Function("return \'Hi!\';");',
           { fileName: "syringe.js" });
print(uneval(gDO.getOwnPropertyDescriptor('f').value.script.source.displayURL));

Comment 3

3 years ago
I don't think comment 0's description of the behavior is correct. It doesn't seem that displayURL is ever set on the function's source, even after the onNewScript handler has fired.

    $ cat ~/moz/displayURL2.js
    var g = newGlobal();
    var dbg = new Debugger;
    var gDO = dbg.addDebuggee(g);

    g.evaluate('//# sourceURL=munge.js\n' +
               'f = Function("null();");',
               { fileName: "syringe.js" });
    var fDO = gDO.getOwnPropertyDescriptor('f').value;
    print("f's URL: " + fDO.script.source.url);
    print("f's displayURL: " + fDO.script.source.displayURL);
    $ obj~/js/src/js -f ~/moz/displayURL2.js 
    f's URL: syringe.js line 2 > Function
    f's displayURL: null
    $

Comment 4

3 years ago
That is, I think the behavior is that Debugger.Source instances for calls to Function do not inherit the surrounding script's displayURL, under any circumstances.

I think this is the correct behavior --- one should use the source's introductionScript to generate an appropriately descriptive name:

$ cat ~/moz/displayURL2.js
var g = newGlobal();
var dbg = new Debugger;
var gDO = dbg.addDebuggee(g);

g.evaluate('//# sourceURL=munge.js\n' +
           'f = Function("null();");',
           { fileName: "syringe.js" });
var fDO = gDO.getOwnPropertyDescriptor('f').value;
print("f's URL: " + fDO.script.source.url);
print("f's displayURL: " + fDO.script.source.displayURL);
print("f's introduction script's displayURL: " + fDO.script.source.introductionScript.source.displayURL);
$ obj~/js/src/js -f ~/moz/displayURL2.js 
f's URL: syringe.js line 2 > Function
f's displayURL: null
f's introduction script's displayURL: munge.js
$ 

Having eval'ed or Function'ed sources inherit the calling code's URL is just wrong, and should never happen. It's legacy behavior.

Comment 5

3 years ago
Note that eval and Function behave consistently:

    $ cat ~/moz/displayURL2.js
    var g = newGlobal();
    var dbg = new Debugger;
    var gDO = dbg.addDebuggee(g);

    g.evaluate('//# sourceURL=munge.js\n' +
               'f = eval("(function () { null(); })");',
               { fileName: "syringe.js" });
    var fDO = gDO.getOwnPropertyDescriptor('f').value;
    print("f's URL: " + fDO.script.source.url);
    print("f's displayURL: " + fDO.script.source.displayURL);
    print("f's introduction script's displayURL: " + fDO.script.source.introductionScript.source.displayURL);
    $ obj~/js/src/js -f ~/moz/displayURL2.js 
    f's URL: syringe.js line 2 > eval
    f's displayURL: null
    f's introduction script's displayURL: munge.js
    $
If the source of the new function created here: 

  new Function("//# sourceURL=foo.js")

doesn't have displayURL set to "foo.js" we have a serious problem.

What Jim has detailed is expected behavior to me.

Comment 7

3 years ago
I'm sorry, I'm going around in circles --- the bug is exactly as Honza described at the outset:

$ cat ~/moz/displayURL3.js
var g = newGlobal();
var dbg = new Debugger;
var gDO = dbg.addDebuggee(g);

var capturedScript;
var capturedDisplayURL;
dbg.onNewScript = function (script) {
  capturedScript = script;
  capturedDisplayURL = script.source.displayURL;
  dbg.onNewScript = undefined;
};
var fDO = gDO.makeDebuggeeValue(g.Function('//# sourceURL=munge.js\n'));
assertEq(capturedScript, fDO.script);
assertEq(capturedDisplayURL, fDO.script.source.displayURL);
$ js/src/obj~/js/src/js -f ~/moz/displayURL3.js
/home/jimb/moz/displayURL3.js:14:0 Error: Assertion failed: got null, expected "munge.js"
$ 

Since this is js-shell-only, at least we know there's no weird asynchronous stuff going on.

Comment 8

3 years ago
I'm going to leave this unassigned, because I have some urgent stuff to take care of at the moment, but given the test case above, it should be straightforward to at least identify the cause of this bug, if not fix it.
(Assignee)

Comment 9

3 years ago
Created attachment 8494467 [details] [diff] [review]
981987.patch
Attachment #8494467 - Flags: review?(jimb)
(Assignee)

Comment 10

3 years ago
Something like that, hopefully. It fixes the shell test case, although I don't know enough about the surrounding code to know if there's something I'm missing. Try run would be appreciated - I don't have access.

Comment 11

3 years ago
Comment on attachment 8494467 [details] [diff] [review]
981987.patch

Review of attachment 8494467 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great --- thanks!
Attachment #8494467 - Flags: review?(jimb) → review+

Comment 12

3 years ago
Comment on attachment 8494467 [details] [diff] [review]
981987.patch

Review of attachment 8494467 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, wait. This isn't ready to land. Please incorporate the test case I posted in comment 7, and re-request review; I'll r+ that; we can mark it checkin-needed; and the sheriffs will land it.
Attachment #8494467 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8494762 [details] [diff] [review]
981987.patch

Not sure about test organization, but there you go. Did you want to do a try run?
Attachment #8494467 - Attachment is obsolete: true
Attachment #8494762 - Flags: review?(jimb)

Comment 14

3 years ago
(In reply to Simon Lindholm from comment #13)
> Created attachment 8494762 [details] [diff] [review]
> 981987.patch
> 
> Not sure about test organization, but there you go. Did you want to do a try
> run?

Yes, definitely. Can you do that?

Comment 15

3 years ago
Comment on attachment 8494762 [details] [diff] [review]
981987.patch

Review of attachment 8494762 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Did you verify that it fails without the patch?
Attachment #8494762 - Flags: review?(jimb) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Jim Blandy :jimb from comment #14)
> Yes, definitely. Can you do that?

I would, but I don't have level 1 commit access. Think I should file a bug for it? Otherwise it might be simpler if you do it for now (I also don't have hg set up, etc.).

> Looks good. Did you verify that it fails without the patch?

Yes, it does.

Comment 17

3 years ago
Try push:
https://tbpl.mozilla.org/?tree=Try&rev=0eeb0845895e

I added checks for sourceMapURL as well.

Comment 18

3 years ago
Created attachment 8495340 [details] [diff] [review]
Set displayURL before invoking Debugger.onNewScript hook.

Try push looks good.
Attachment #8494762 - Attachment is obsolete: true
Attachment #8495340 - Flags: review+

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/03b5cb83435d
Assignee: nobody → simon.lindholm10
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/03b5cb83435d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.