Closed Bug 908283 Opened 6 years ago Closed 6 years ago

Don't attach onStep hooks to frames without scripts

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: fitzgen, Assigned: Sahilc.2200)

References

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js])

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 878307
Priority: -- → P3
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Assignee: nobody → Sahilc.2200
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 :)
Status: NEW → ASSIGNED
Attached patch Added check for frame script (obsolete) — Splinter Review
I think this should do it, please review
Attachment #803196 - Flags: review?(nfitzgerald)
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.
Attachment #803196 - Flags: review?(nfitzgerald)
Attached patch Updated the previous patch (obsolete) — Splinter Review
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!
Attachment #803196 - Attachment is obsolete: true
Attachment #803212 - Flags: review?(nfitzgerald)
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.
Attachment #803212 - Flags: review?(nfitzgerald) → review+
Attached patch RebasedSplinter Review
Rebased the patch
Attachment #803212 - Attachment is obsolete: true
Attachment #803262 - Flags: review?(nfitzgerald)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=9c79844af120

Assuming it comes back green, I'll land in fx-team
Attachment #803262 - Flags: review?(nfitzgerald) → review+
Fixed in fx-team!

https://hg.mozilla.org/integration/fx-team/rev/1c6591c69b5a
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1c6591c69b5a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.