978019 - Breakpoint doesn't get hit on self-executing function

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

Debugger doesn't pause at a breakpoint.

STR:
1. Load https://getfirebug.com/tests/manual/issues/7217/test.html
2. Open the debugger 
4. Create a breakpoint at line 2 in test.html
5. Reload the page, breakpoint doesn't hit -> BUG

Originally reported in Firebug issue list:
http://code.google.com/p/fbug/issues/detail?id=7217

(it's works in JSD1)

Honza

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Line 2 of test.js, right? I can reproduce. :(

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hmmm, the script is added via onNewScript -> _addScript, but the breakpoint doesn't get set because endLine is reported as 1, and our breakpoint is on line 2. I think we might just have an off by one error on this line, where the decrement is unnecessary:

  let endLine = aScript.startLine + aScript.lineCount - 1;

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-978019-iife-breakpoint.patch

https://tbpl.mozilla.org/?tree=Try&rev=3abeb0e1fdb2

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8383858 [details] [diff] [review]
bug-978019-iife-breakpoint.patch

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

Nice catch! I started playing a little more with the test case and I found that slightly modifying the script indicates there are more problems. Try changing the one-liner to 3 lines like this:

var b = 9;
console.log("x", b);
return b;

In this case only a breakpoint in the first line will pause execution. Breakpoints in the other two lines are ignored. Can you investigate why?

::: browser/devtools/debugger/test/head.js
@@ +15,5 @@
>  let { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
>  let { Promise: promise } = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {});
>  let { gDevTools } = Cu.import("resource:///modules/devtools/gDevTools.jsm", {});
>  let { devtools } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +let { require } = devtools;

This seems unused, is it a leftover from debugging?

Comment 5

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> Line 2 of test.js, right?
Sorry, yes, it should have been test.js, but you already figured that out :-)

Honza

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-978019-iife-breakpoint.patch

Hmmmm...

In my debugging, when we call |findScripts| from |_restoreBreakpoints|, we only get the global level Debugger.Script (aka its staticLevel is 0) and *not* the Debugger.Script for the Immediately Invoked Function Expression (IIFE). Furthermore, |onNewScript| is never called for the IIFE's Debugger.Script. Because neither of those two things happen, the breakpoints are never reset on the Debugger.Script and will never be hit.

Very troubling that the IIFE's Debugger.Script doesn't show up via either |findScripts| or |onNewScript|...

Would think that maybe the fact that |findScripts| is GC sensitive or that we lazily compile functions would account for the IIFE not showing up in |findScripts|, but I have no idea why it wouldn't show up via |onNewScript|.

needinfo?ing Jim for help.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Wait, I kind of remember the debugger API only calling |onNewScript| for new global level scripts (aka static level is 0). This combined with the lazy function compilation could possibly account for this bug. If this is the case (I'm about to dig), I'd think that we would want to have onNewScript called for every new Debugger.Script instance.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-978019-iife-breakpoint.patch

https://tbpl.mozilla.org/?tree=Try&rev=ed07b6abde04

So yeah, child scripts weren't getting found by our |findScripts| call nor by |onNewScript| so we have to find it ourselves by calling |getChildScripts| on every script found by |findScripts| in our |_restoreBreakpoints| method.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-978019-iife-breakpoint.patch

https://tbpl.mozilla.org/?tree=Try&rev=a78f86bfb9e3

Actually we just need to call |getChildScripts| on the scripts we get from |onNewScript| because that only gives us top level scripts.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See also, the reason this bug was kind of tricky to track down: https://bugzilla.mozilla.org/show_bug.cgi?id=979094

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8385025 [details] [diff] [review]
bug-978019-iife-breakpoint.patch

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

It looks like the new test is broken, so I'm clearing the review until it's fixed.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #11)
> Comment on attachment 8385025 [details] [diff] [review]
> bug-978019-iife-breakpoint.patch
> 
> Review of attachment 8385025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks like the new test is broken, so I'm clearing the review until it's
> fixed.

Huh. I was getting this error locally, but then I rebased and it was fixed. Maybe I need to update my hg repo too...

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=ed6382c10d5c

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8385025 [details] [diff] [review]
bug-978019-iife-breakpoint.patch

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

Very nice patch for a very sad bug.
*sigh*

::: browser/devtools/debugger/test/browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js
@@ +23,5 @@
> +    Task.spawn(function* () {
> +      yield waitForSourceShown(gPanel, CODE_URL);
> +
> +      // Pause and set our breakpoints.
> +      yield doInterrupt();

This shouldn't be necessary right? No need to take it out if doing that would make the test more flaky though.

::: toolkit/devtools/server/actors/script.js
@@ +2195,5 @@
>    onNewScript: function (aScript, aGlobal) {
>      this._addScript(aScript);
> +    for (let s of aScript.getChildScripts()) {
> +      this._addScript(s);
> +    }

Could you add a brief comment about why we have to do this?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/63fb1f3f6b0b

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/63fb1f3f6b0b

Comment 17

[Sebastian Zartner [:sebo]]

Justed tested with Firebug and it's working fine for me now. Thanks for the quick fix!

Sebastian

Comment 18

[Petruta Horea [:phorea], Desktop Test Engineering]

Reproduced on Developer Tools / Debugger on Nightly 2014-02-28.
Verified as fixed on latest Aurora 30.0a2 2014-04-23.