1687166 - Line highlighted in debugger is displaced from line actually being executed when debugging new Function() sources

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[random_n0body]

Attachment: using_new_function.jpg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

1. Open dev console (press f12)
2. Paste this code:

new Function(`
debugger; // this line should be highlighted

if(true) {
  console.log("One step after this line was highlighted you should see this")
}

console.log("Wanna bet this is misaligned?")


`)()

3. press enter
4. press the "step into button"

Actual results:

1. The first highlighted line is: if(true), but it should be debugger;
2. The first console.log statement is skipped
3. The second highlighted line is an empty line
4. The second console.log statement is skipped
5. The third highlighted line is an empty line

Note that the code executes normally.

Expected results:

1. The first highlighted line should have been debugger;
2. The second highlighted line should have been console.log
3. The third highlighted line should have been the 2nd console.log statement

WHEN USING eval() INSTEAD OF NEW FUNCTION()() IT WORKS CORRECTLY, please just compare the two

Comment 1

[random_n0body]

Attachment: using_eval.jpg

Comment 2

[random_n0body]

Please excuse the simple demonstrating code, you might think "so what, it's apparent which line it means".

But when working with actual code with frequent function calls this is a nightmare. The debugger might show that it's entering a function which happens in case of an error and you wonder why it would do that, then you step further and realize that it actually entered a completely different function.

It makes debugging sources in new Function essentially impossible and forces me to switch to chrome every time.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report, I can easily reproduce on my machine
Honza

Comment 4

[random_n0body]

Thanks Honza.

I found a workaround in the meantime: when using inline sourcemaps (with data uri) the lines in the sourcemaps are not misaligned.

Comment 5

[random_n0body]

Nevermind. While the highlighted line doesn't seem to be misaligned anymore setting a breakpoint still sets it on the wrong line :(

Comment 6

[random_n0body]

With sourcemaps the breakpoints are displaced in chrome too, **** 😭😭😭

And safari just freezes when trying to debug new Function sources.

It's 2021 and I can make code undebuggable in all browsers with one simple trick ;)

Comment 7

[Alexandre Poirot [:ochameau]]

We started looking into this.

When evaluating a source as simple as new Function("debugger")()...
We fetch each new incoming source text content from there:
https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/devtools/server/actors/source.js#269-281

actualText() {
    [...]
    return padding + this._source.text;
}

the source text content for that source is debugger. This may be the beginning of the trouble.
We might want to ensure having function anonymous() { as prefix/first line and } as suffix/last line.

This this._source object is a Debugger.Source object that we derivate from Debugger.Script notified via DebuggerAPI.onNewScript.
We are notified about each new script from there:
https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/devtools/server/actors/thread.js#2021

But bomsy figured out... we were manipulation another Debugger.Source whose text attribute refers to function anonymous(\n) {\ndebugger\n}!
This was really puzzling as this Debuger.Source object isn't related to any script notified via DebuggerAPI.onNewScript!
After some investigation... it seems to related to this DevTools code:
https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/devtools/server/actors/source.js#454-479

  _getTopLevelBreakpointPositionScripts() {
    [...]
    // We need to find all breakpoint positions, even if scripts associated with
    // this source have been GC'ed. We detect this by looking for a script which
    // does not have a function: a source will typically have a top level
    // non-function script. If this top level script still exists, then it keeps
    // all its child scripts alive and we will find all breakpoint positions by
    // scanning the existing scripts. If the top level script has been GC'ed
    // then we won't find its breakpoint positions, and inner functions may have
    // been GC'ed as well. In this case we reparse the source and generate a new
    // and complete set of scripts to look for the breakpoint positions.
    // Note that in some cases like "new Function(stuff)" there might not be a
    // top level non-function script, but if there is a non-function script then
    // it must be at the top level and will keep all other scripts in the source
    // alive.
    if (!this._isWasm && !scripts.some(script => !script.isFunction)) {
      let newScript;
      try {
        newScript = this._source.reparse();
      } catch (e) {
        // reparse() will throw if the source is not valid JS. This can happen
        // if this source is the resurrection of a GC'ed source and there are
        // parse errors in the refetched contents.
      }
      if (newScript) {
        scripts = [newScript];
      }
    }

This code is all new to me and I'm not sure I fully understand it.
But it seems suspicious that it is triggered for such source. It isn't a GCed source?
The !scripts.some(script => !script.isFunction) logic is probably brittle.

Anyway, I'll attach a simple test which highlights that Debugger.Source.reparse() actually changes the source text content and automagically prepend and append function anonymous() {...}...

Here is a few possible call for actions:

• Should spidermonkey Debugger.Source.text attribute ensure adding function anonymous(){ ...} for "new Function" sources?
• If not, should it be done by devtools code?
• Or instead of adding these lines, are the error lines buggy and should be fixed to match current behavior of source text?
• Independently of all this, we should probably revisit this !scripts.some(script => !script.isFunction)? The whole logic around GCed sources may be all brittle? Part of it was introduced in bug 1572280.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1687166 - [devtools] Demonstrate Debugger.Source.text and Debugger.Source.reparse behavior against "new Function" sources.

Comment 9

[Alexandre Poirot [:ochameau]]

Some more discussion on phabricator where I wrote a test to assert current behavior of Debugger.Source.text:
https://phabricator.services.mozilla.com/D165294#5444543

Comment 10

[Alexandre Poirot [:ochameau]]

This issue relates to code added in bug 755821.

Debugger.Source.text attribute explicitely unwraps source test and removes function anonymous(...args){ and } over here:
https://searchfox.org/mozilla-central/rev/4c86c51a4fedf5569c948225984f8fe26713dbe4/js/src/debugger/Source.cpp#225-227

    if (ss->isFunctionBody()) {
      return ss->functionBodyString(cx_);
    }

If I remove this code, the debugger shows the right and complete source for new Function(...) sources.
You can see that via the test I wrote in https://phabricator.services.mozilla.com/D165294
Debugger.Source.text will return the same source as function.toString() and then the line offsets and pause will be correct.

I looked at the overall discussion on bugzilla and couldn't find valuable discussion about these 3 lines of code.

Let's see what try says when removing this code:
https://treeherder.mozilla.org/jobs?repo=try&revision=e86f86c1d827bc43f02231a156edaa3c90ec5397

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1687166 - [devtools] Fetch whole source text for "new Function()" sources.

But keep unwrapping sources for DOM event handlers like:
<div onclick="foo()" />
Whose source should be:
foo()
and not:
function onclick() { foo() }

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88acd0f49283
[devtools] Fetch whole source text for "new Function()" sources. r=arai,bomsy

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/88acd0f49283