Closed Bug 949762 Opened 6 years ago Closed 5 years ago

Tracer should respect black boxing

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: fitzgen, Assigned: jjurovych)

References

Details

Attachments

(3 files, 4 obsolete files)

We should hide or grey out frame enters/exit logs in the traces view when the frame's source is black boxed. This will make it much easier for users to visually parse the logs and find the things they are interested in.

I'm leaning towards "grey out", but could be convinced either way.
Assignee: nobody → nfitzgerald
assigned, p3.
Status: NEW → ASSIGNED
Priority: -- → P3
Assignee: nfitzgerald → nobody
Assignee: nobody → jjurovych
Attached patch bug-949762.patch (obsolete) — Splinter Review
This patch keeps track of what traces come from blackboxed sources and then greys them out.

So far:
* successive traces from blackboxed sources do not get collapsed
* (un)blackboxing a source doesn't affect existing traces

I will be working on these next. Do we want to land this now or wait till everything's ready?
Attachment #8458958 - Flags: review?(nfitzgerald)
Attached image v1-button-example.png
This is what it looks like right now after clicking button, then blackboxing source and then clicking button again.
Attached image v1-todomvc.png
Backbone.js TodoMVC with jquery.js blackboxed.
Comment on attachment 8458958 [details] [diff] [review]
bug-949762.patch

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

Looks pretty good, but see comment below about reusing SourceClient and SourceActor rather than reimplementing the management of black boxed sources.

> Do we want to land this now or wait till everything's ready?

This is definitely an improvement over what we have now, so I think we should ship it (once the patch is fixed up a bit) and do follow ups to make it even better :)

::: browser/devtools/debugger/debugger-controller.js
@@ +1077,5 @@
>    this._onSourcesAdded = this._onSourcesAdded.bind(this);
>    this._onBlackBoxChange = this._onBlackBoxChange.bind(this);
>    this._onPrettyPrintChange = this._onPrettyPrintChange.bind(this);
> +
> +  this.blackBoxedSources = new Set(); // Stores URLs of blackBoxed sources

Rather than creating another place we store and maintain sources' black boxed state, we should re-use the source's SourceClient which already maintains this data.

To do this, the TraceActor should send the function's SourceActor's form in its "enterFrame" packets. It can get this from the ThreadSources object.

::: browser/devtools/debugger/test/browser_dbg_tracing-07.js
@@ +17,5 @@
> +      gPanel = aPanel;
> +      gDebugger = gPanel.panelWin;
> +
> +      waitForSourceShown(aPanel, "code_tracing-01.js")
> +        .then(() => startTracing(aPanel))

For new tests, we like to use Task.jsm. Should be pretty straightforward to make this test use it.
Attachment #8458958 - Flags: review?(nfitzgerald)
Attached patch bug-949762-2-wip.patch (obsolete) — Splinter Review
@fitzgen I have refactored the code. Rather than storing a copy of blackboxed sources, now I ask the ThreadActor. This means that I'm calling a sibling's method, which we wanted to avoid. I think this is still better than extending a parent (TabActor/RootActor) from a children (ThreadActor), though.

ThreadSources now live in actors/utils/Sources.js instead of actors/script.js, but unless we actually use them somewhere else besides ThreadActor, this change should probably be reverted.

Do you have something particular in mind how to go around accessing a sibling?
Attachment #8458958 - Attachment is obsolete: true
Comment on attachment 8461279 [details] [diff] [review]
bug-949762-2-wip.patch

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

(In reply to Jakub Jurovych from comment #6)
> Created attachment 8461279 [details] [diff] [review]
> bug-949762-2-wip.patch
> 
> @fitzgen I have refactored the code. Rather than storing a copy of
> blackboxed sources, now I ask the ThreadActor. This means that I'm calling a
> sibling's method, which we wanted to avoid. I think this is still better
> than extending a parent (TabActor/RootActor) from a children (ThreadActor),
> though.
> 
> ThreadSources now live in actors/utils/Sources.js instead of
> actors/script.js, but unless we actually use them somewhere else besides
> ThreadActor, this change should probably be reverted.
> 
> Do you have something particular in mind how to go around accessing a
> sibling?

I meant to have the parent actor own the sources, not for any kind of extending or inheritence, which I agree would be a bad idea.

It's fine though, I think you're right that leaving the sources on the thread is ok. Let's not split it out from script.js.

This should have an xpcshell test that creates two sources (say, A and B) which calls A->B->A. Then test black boxing one of the sources and verifying the traced logs.

::: toolkit/devtools/server/Makefile.in
@@ +7,5 @@
>  libs::
>  	$(INSTALL) $(IFLAGS1) $(srcdir)/*.jsm $(FINAL_TARGET)/modules/devtools
>  	$(INSTALL) $(IFLAGS1) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/server
>  	$(INSTALL) $(IFLAGS1) $(srcdir)/actors/*.js $(FINAL_TARGET)/modules/devtools/server/actors
> +	$(INSTALL) $(IFLAGS1) $(srcdir)/actors/utils/*.js $(FINAL_TARGET)/modules/devtools/server/actors/utils
\ No newline at end of file

FYI, bug 1039952 landed in fx-team, so pretty soon you won't need this because that bug includes it.

::: toolkit/devtools/server/actors/script.js
@@ +516,5 @@
>      return this._threadLifetimePool;
>    },
>  
> +  /**
> +   * Sources are stored in a parent to make them accessible to other actors.

Nit: "stored in the ThreadActor's parent"

... but it isn't anyways here.
Attachment #8461279 - Flags: feedback+
Attached patch bug-949762-3.patch (obsolete) — Splinter Review
Attachment #8461279 - Attachment is obsolete: true
Attachment #8468928 - Flags: review?(nfitzgerald)
Attached patch bug-949762-4.patch (obsolete) — Splinter Review
Fixing test title.
Attachment #8468928 - Attachment is obsolete: true
Attachment #8468928 - Flags: review?(nfitzgerald)
Attachment #8468973 - Flags: review?(nfitzgerald)
Comment on attachment 8468973 [details] [diff] [review]
bug-949762-4.patch

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

Looks good! r+ with comments below addressed.

::: browser/devtools/debugger/test/browser_dbg_tracing-07.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test that we get the expected frame enter/exit logs in the tracer view.

I think this test is more about trace logs + black boxing. Left over from copy+paste?

@@ +65,5 @@
> +  SpecialPowers.pushPrefEnv({'set': [["devtools.debugger.tracer", true]]}, deferred.resolve);
> +  return deferred.promise;
> +}
> +
> +function popPref() {

These seem like pretty useful things to share in head.js, if we didn't hard code the prefs we were pushing :)

::: toolkit/devtools/server/tests/unit/test_trace_actor-10.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Create 2 sources, A and B, B is black boxed. When calling functions A->B->A, verify that only

Nit: wrap at 80 chars, please

@@ +46,5 @@
> +
> +  yield startTrace();
> +
> +  yield executeOnNextTickAndWaitForPause(evalSetup, gClient);
> +  yield resume(gThreadClient);

I don't think you need to jump through the executeOnNextTickAndWaitForPause hoops if you're just going to resume again. Instead you can just eval the text on this tick of the event loop and not have a debugger statement at the end of the text. The reason we do that in other cases has to do with the interaction between promises/task.jsm, debugger pauses, and event loops. I don't think it's necessary when we don't need a pause.
Attachment #8468973 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/93f366d6cb3c
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/93f366d6cb3c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.