908283 - Don't attach onStep hooks to frames without scripts

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

Jim Blandy :jimb from bug 878307 comment 4:

> 1) (In a follow-up) It might be possible to handle the !script cases more
> simply, just by never establishing an onStep handler for such frames at all.
> Right now, Debugger never generates D.Frame instances that don't have
> scripts, but when we have visible non-debuggee frames and frames for native
> calls, those will lack scripts. In either case, running until the frame
> changes is the best 'step' could do anyway.

Comment 1

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

Basically, if we don't have frame.script, we can't reliably step, so we shouldn't even attach the onStep handler.

Fixing this bug involves making a minor change to http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#876 so that if the frame doesn't have a script, we don't set stepFrame.onStep.

Make sure you test the changes with

  $ ./mach xpcshell-test toolkit/devtools/server

and

  $ ./mach mochitest-browser browser/devtools/debugger

Learn more about the workflow here: https://wiki.mozilla.org/DevTools/Hacking

Good luck! Don't be afraid to ask any questions in IRC or as a comment on this bug :)

Comment 2

[Sahil Chelaramani]

Attachment: Added check for frame script

I think this should do it, please review

Comment 3

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

Comment on attachment 803196 [details] [diff] [review]
Added check for frame script

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

Looks pretty good :) Fix up the patch with my comments below and attach the updated version and I will look at it again, push it to try, and assuming everything looks good land it in fx-team.

::: toolkit/devtools/server/actors/script.js
@@ +863,5 @@
>                case "step":
>                  this.dbg.onEnterFrame = onEnterFrame;
>                  // Fall through.
>                case "next":
> +                if(stepFrame.script!="undefined")

Just use `if (stepFrame.script) { ...` since if a script is attached it will always be truthy.

Small style nit: please add a space after `if` before the opening paren.

@@ +864,5 @@
>                  this.dbg.onEnterFrame = onEnterFrame;
>                  // Fall through.
>                case "next":
> +                if(stepFrame.script!="undefined")
> +                {

Small style nit: please use the braces-on-same-line style used elsewhere in this file (and the rest of our code).

@@ +866,5 @@
>                case "next":
> +                if(stepFrame.script!="undefined")
> +                {
> +                  stepFrame.onStep = onStep;
> +                  stepFrame.onPop = onPop;

We should always attach the onPop handler, otherwise we wouldn't pause when we exited the frame. Only the onStep should be conditionally attached.

Comment 4

[Sahil Chelaramani]

Attachment: Updated the previous patch

Ran tests, they all seemed to pass. Here is the patch with the changes you asked for. Let me know if anything else needs to be done here!

Comment 5

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

Comment on attachment 803212 [details] [diff] [review]
Updated the previous patch

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

Looks good to me, although it needs to be rebased on the latest fx-team. Do that and then I will push the patch to try.

Comment 6

[Sahil Chelaramani]

Attachment: Rebased

Rebased the patch

Comment 7

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9c79844af120

Assuming it comes back green, I'll land in fx-team

Comment 8

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

Fixed in fx-team!

https://hg.mozilla.org/integration/fx-team/rev/1c6591c69b5a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c6591c69b5a