Closed Bug 877682 Opened 7 years ago Closed 7 years ago

blackbox sources in the debugger server via the rdp

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

This is the first step to blackboxing. We need protocol support, and the debugger server needs to be able to step through libraries, filter out their stack frames, etc.
Depends on: 877686
Blocks: 877686
No longer depends on: 877686
Assignee: nobody → nfitzgerald
Attached patch wip 1 (obsolete) — Splinter Review
Basic functionality is working, and the test for that is passing. I know that edge cases are probably buggy right now, and I plan to write more tests and fix up the code some more. Specifically:

* Blackboxing the source that you are paused in

* Making sure that breakpoints inside blackboxed sources don't pause execution, and that they still exist after unblackboxing

* Making sure that debugger statements inside blackboxed sources don't pause execution
Attached patch wip 2 (obsolete) — Splinter Review
WIP 2

Added the tests I mentioned before, just need to go through and add comments/documentation and clean up little things here and there.
Attachment #759434 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
Woooooooooooo!

Bring on the review cycle >:)
Attachment #759537 - Attachment is obsolete: true
Attachment #759928 - Flags: review?(past)
Comment on attachment 759928 [details] [diff] [review]
v1

dcamp volunteered to review on irc :)

Race to see who R+'s me first! Go!
Attachment #759928 - Flags: review?(dcamp)
Comment on attachment 759928 [details] [diff] [review]
v1

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

This is surprisingly simple!

::: toolkit/devtools/client/dbg-client.jsm
@@ +1581,5 @@
> +    telemetry: "BLACKBOX",
> +    after: function (aResponse) {
> +      if (!aResponse.error) {
> +        this._isBlackBoxed = true;
> +      }

This blackBox/UnblackBox stuff is going to get confused if the user doesn't synchronize very carefully:

client.blackBox(() => { console.log("blackboxed!"); });
client.unblackBox(() => { console.log("unblackboxed!") });

Is going to print:

unblackboxed!
blackboxed!

And leave the source black boxed.  That's probably not what is expected.

::: toolkit/devtools/server/actors/script.js
@@ +489,5 @@
> +    for (; frame && (!count || frames.length < count); i++, frame=frame.older) {
> +      if (this.sources.isBlackBoxed(frame.script.url)) {
> +        continue;
> +      }
> +

Don't you think we should at least include a single frame for blackboxed sources?

I would expect this stack:

#1 callback
#2 whenWillItEnd
#3 moreGoop
#4 doMoreLibraryStuff
#5 doLibraryStuff: 
#6 userCode: function { doLibraryStuff(callback) }

To collapse to:
#1 callback
#2 <library.js internals>
#3 userCode: function { doLibraryStuff(callback) }

Not:
#1 callback
#2 userCode: function { doLibraryStuff(callback) }

But maybe that's not what everybody expects?  Want to bring this up on the list?
Attachment #759928 - Flags: review?(dcamp)
(In reply to Dave Camp (:dcamp) from comment #5)
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +1581,5 @@
> > +    telemetry: "BLACKBOX",
> > +    after: function (aResponse) {
> > +      if (!aResponse.error) {
> > +        this._isBlackBoxed = true;
> > +      }
> 
> This blackBox/UnblackBox stuff is going to get confused if the user doesn't
> synchronize very carefully:
> 
> client.blackBox(() => { console.log("blackboxed!"); });
> client.unblackBox(() => { console.log("unblackboxed!") });
> 
> Is going to print:
> 
> unblackboxed!
> blackboxed!
> 
> And leave the source black boxed.  That's probably not what is expected.

The behavior will also depend on whether the source is already blackboxed or not, due to the check in the blackBox/unblackBox methods that calls the callback function synchronously. This is quite common in dbg-client.jsm, but I've started writing new methods in an always-async fashion and I think we should also do a mass conversion of the existing ones at some point.

> ::: toolkit/devtools/server/actors/script.js
> @@ +489,5 @@
> > +    for (; frame && (!count || frames.length < count); i++, frame=frame.older) {
> > +      if (this.sources.isBlackBoxed(frame.script.url)) {
> > +        continue;
> > +      }
> > +
> 
> Don't you think we should at least include a single frame for blackboxed
> sources?
> 
> I would expect this stack:
> 
> #1 callback
> #2 whenWillItEnd
> #3 moreGoop
> #4 doMoreLibraryStuff
> #5 doLibraryStuff: 
> #6 userCode: function { doLibraryStuff(callback) }
> 
> To collapse to:
> #1 callback
> #2 <library.js internals>
> #3 userCode: function { doLibraryStuff(callback) }
> 
> Not:
> #1 callback
> #2 userCode: function { doLibraryStuff(callback) }
> 
> But maybe that's not what everybody expects?  Want to bring this up on the
> list?

That's also what I would expect. Blackboxing shouldn't mean removing completely.
Comment on attachment 759928 [details] [diff] [review]
v1

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

::: toolkit/devtools/server/actors/script.js
@@ +53,5 @@
>    get state() { return this._state; },
>    get attached() this.state == "attached" ||
>                   this.state == "running" ||
>                   this.state == "paused",
> +  get youngestFrame() this._youngestFrame,

This proliferation of getters is honestly starting to worry me, especially ever since I observed that they are present (although certainly not significant yet) in profiles.

I am not saying that we should ban this practice from the codebase, or that it doesn't sadden me that Ion Monkey can't inline those calls yet (perhaps profiling or debugging break such optimizations?), but I would be far happier if we could restrict adding such getters to cases where there is an obvious benefit. In this case, for example, I'd rather make the private property public than add a getter.

Again, I don't intend to make a big deal out of this, especially for _youngestFrame, which IIRC we have a bug on file to drop in favor of Debugger.prototype.getNewestFrame(). I guess you could tell that reading debugger profiles these past few days has left me with scars. I probably need therapy.

@@ +1486,5 @@
> +   */
> +  onBlackBox: function SA_onBlackBox(aRequest) {
> +    this.threadActor.sources.blackBox(this.url);
> +    return {
> +      type: "blackboxed"

We usually indicate success in a response packet to a side-effects only request without any property besides "from".

@@ +1504,5 @@
>  
>  SourceActor.prototype.requestTypes = {
> +  "source": SourceActor.prototype.onSource,
> +  "blackbox": SourceActor.prototype.onBlackBox,
> +  "unblackbox": SourceActor.prototype.onUnblackBox

I wonder if one of cloak/uncloak, fold/unfold or hide/reveal would sound better.

@@ +2702,5 @@
> +      this._thread.conn.send(this._thread.onResume({
> +        type: "resume",
> +        resumeLimit: {
> +          type: "step"
> +        }

One problem with this is that our protocol design tries to be stateless and let the client control state transitions. When the user for example has indicated a preference to pause on exceptions, DOM events or XHR, we are (or soon will be) sending this information along every resumption packet, so that the server can properly control the debuggee.

In this case resuming without any such flags set will indicate for example that exceptions thrown while stepping will not pause execution, even if the option is set in the client. See also bug 870128 for a similar issue.

I think it would be better to not try to be smart in this case and indicate to the user somehow (if at all) that blackboxing will take place in the next pause, since the current state is not amenable to manipulation.
Attachment #759928 - Flags: review?(past)
Ok what I am going to do is send all frames, but mark whether they are black boxed or not. This way, whatever UI we decide upon, we will have enough data to implement it in the frontend.

New patch incoming.
Attached patch v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e8df2847b351

* youngestFrame is now a public, no more getter

* blackbox/unblackbox success response packets are just { from: actor } now

* Don't automatically step out of the source when we are paused in it and it gets black boxed, instead just let the client know that we are paused in it and let them decide what to do

* SourceClient.prototype.(un)blackBox is no longer sometimes sync/async. Just always sends the packet to the server.

* Send all frames to the client, whether black boxed or not, but mark the black boxed frames as such. This way the client can choose how to use that data and there is enough info to do whatever it should choose.
Attachment #759928 - Attachment is obsolete: true
Attachment #760648 - Flags: review?(past)
Attachment #760648 - Flags: review?(dcamp)
Comment on attachment 760648 [details] [diff] [review]
v2

Looks good to me, but make sure past r+'s too.

I agree with past's uncertainty about the naming.  Wonder if skip/skipped/unskip is better?
Attachment #760648 - Flags: review?(dcamp) → review+
Comment on attachment 760648 [details] [diff] [review]
v2

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

Looks good to me. You might want to ping Jim about the blackbox/unblackbox names, he is good in that sort of thing. Up to you.

::: toolkit/devtools/server/actors/script.js
@@ +340,5 @@
>        }
>  
>        // Define the JS hook functions for stepping.
>  
> +      let onEnterFrame = (aFrame) => {

You could omit the parens here, too.

@@ +2054,5 @@
>        form.where = { url: this.frame.script.url,
>                       line: this.frame.script.getOffsetLine(this.frame.offset) };
> +      if (this.threadActor.sources.isBlackBoxed(this.frame.script.url)) {
> +        form.isBlackBoxed = true;
> +      }

You could use the one-liner from SA_form here and elsewhere, if you care about that sort of thing.
Attachment #760648 - Flags: review?(past) → review+
Attached patch v2.1Splinter Review
Fixed nits that Panos raised.
Attachment #760648 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/0be523ac1539
Status: NEW → ASSIGNED
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0be523ac1539
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.