825039 - Make Scratchpad execute in context of current debugger

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Julian Viereck]

Evaluating some script in scratchpad should evaluate the script in the context of the current debugger stack frame, IFF the debugger is active.

Related bugs:
- https://bugzilla.mozilla.org/show_bug.cgi?id=774753
- https://bugzilla.mozilla.org/show_bug.cgi?id=783499

Comment 1

[Julian Viereck]

:victorporof created this gist: https://gist.github.com/4391557

The idea is execute a string either in the debugger's current frame stack if the debugger is paused or use the current existing evalInSandbox on the scratchpad.

There are some issues with this:
- this doesn't work for remote tabs. As Scratchpad doesn't handle remote tabs/debugging at all ATM, I don't think this is something to be concerned about ATM
- victor also mentioned on IRC "and using the debugger frontend is a bit too tangled.."

My plan was to continue the lines of victor's gist. Given the issues above, I won't continue until the issues are resolved.

Comment 2

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

This seems like a slight twist on bug 723556, but if we make the changes planned in that bug (make the scratchpad always use the remote protocol) we wouldn't need this.

Comment 3

[Brandon Benvie [:benvie]]

I think this bug will do to cover my current work on making the scratchpad execute using the debugger protocol.

Comment 4

[Brandon Benvie [:benvie]]

Attachment: WIP super basic but partially working

This patch contains the groundwork for fully converting the Scratchpad to execute over the debugger protocol. It does actually work, but with a lot of caveats:

* The Scratchpad must be opened via the button on the devtools toolbox, not the menu. Otherwise the DebuggerServer/Client are not initialized
* The text rendering is a very minimal placeholder that mostly relies on `response.errorMessage` `response.result.displayString`
* The inspect dialog is completely unimplemented, aside from inspecting the above string result.
* Executing in chrome is unsupported
* Probably many other broken things

Comment 5

[Victor Porof [:vporof][:vp]]

There's a similar effort going on at https://github.com/mozilla/r2d2b2g/pull/415

Comment 6

[Brandon Benvie [:benvie]]

The ScratchpadActor used there is an excellent foundation. With the goal of adding more features to the Scratchpad down the road, it probably would be wise to add a dedicated actor at some point anyway. I was avoiding it for now for the sake of simplicity, but I think adding the actor is more simple. Also, it may be required unless there's some other existing way to eval in chrome, since this is an existing feature of the scratchpad that needs to be supported.

Comment 7

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

We're moving away from evalInSandbox to the debugger API in general (bug 783499 for the web console), due to the unintuitive behavior of the former (new global variables not visible from the content page, etc.). Mihai adds an evaluateJS method to the web console client there that should fit the bill for scratchpad as well.

Furthermore, using the debugger API will be necessary for more advanced features, like evaluating the scratchpad in a paused page or setting breakpoints in scratchpad code.

Comment 8

[Brandon Benvie [:benvie]]

Yeah, in the initial patch I posted I'm using evaluateJS. A combination of what I've done so far along with Luca Greco's done there for b2g is probably ideal.

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've put together my prototype because being able to use
scratchpad on the b2g-desktop connected to the simulator
speed up my tests, it was not intended to be merged
immediately into the simulator,
and I tried to get a mostly complete remote scratchpad
without change any scratchpad file (and leaving the standard
scratchpad to work as usually) adapting current scratchpad code
to the new scenario and position:
- move into a new debugger actor
- expose current displayed firefoxos app and
  current displayed tab on the firefoxos browser

build a solution that will work in the different scenarios
(e.g. desktop, android and firefoxos simulator/devices)
is much more complicated and I'm pretty sure it should reuse
webconsole actors for the same reason exposed by Panos Astithas
(e.g. evaluateJS returns a "value grip" which is inspectable
and it is able to evaluate in a frameActor)

Moreover the scratchpad contexts list should be build from the
"listTabs" reply, unfortunately listTabs doesn't currently
contains FirefoxOS Apps or FirefoxOS Browser Tabs (at least on
b2g-desktop), but that's a different issue.

I'll do some experimentations on the Brandon Benvie prototype asap
to better understand if there's anything useful we can get from my
prototype or if I've encountered and know anything useful on any of
the remaining issues.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: WIP2

Rebases the patch to the tentative bug808369 patch.

Comment 11

[Brandon Benvie [:benvie]]

Attachment: WIP2.1

Remove a stray "+".

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP2.1

Make it a patch...

Comment 13

[Brandon Benvie [:benvie]]

Attachment: WIP2.2

rebase to latest fx-team

Comment 14

[Brandon Benvie [:benvie]]

Attachment: WIP3

Uses the updated VariablesViewController.

Comment 15

[Brandon Benvie [:benvie]]

Attachment: WIP4

Rebase to latest bug 828680.

Comment 16

[Brandon Benvie [:benvie]]

Attachment: WIP5

rebase

Comment 17

[Brandon Benvie [:benvie]]

Attachment: WIP6

Rebase to bug 869984.

Comment 18

[Brandon Benvie [:benvie]]

Attachment: WIP7

This patch adds support for execution in both the existing sandbox, as well as in the current debugger context. If the devtools toolbox is open when a given Scratchpad window is created, then the Scratchpad will execute via the DebuggerClient in that context. If not, then it executes locally in its own sandbox context.

This doesn't handle the case where a Scratchpad is using a DebuggerClient and then the toolbox is closed (errors). I'm not sure what's the best to do from a UI standpoint here: silently change to a local sandbox, close the Scratchpad when the toolbox closes (including a save prompt), something else?

Comment 19

[Victor Porof [:vporof][:vp]]

I think there are 4 cases that should be handled (maybe some overlap?):

1. Scratchpad is opened with no toolbox available
2. Scratchpad is opened and toolbox is available
3. Scratchpad is open, and then toolbox is opened
4. Scratchpad and toolbox are both open, and then toolbox is closed

We should also notify users whether "Scratchpad is now executing on the toolbox's target" and "Scratchpad is now executing locally", or at least provide a way to opt in/out. I think 90% of the cases you'll want to execute things remotely, so opt out is what I would choose.

For notifications, I bet it's a good idea to use the current notification system (the yellow bar at the top, shown, for example, when a chrome sandbox is picked). That can include the ok/cancel buttons handling opt in/out.

Comment 20

[Victor Porof [:vporof][:vp]]

Note that things would greatly simplify if we're ever going to dock scratchpad (bug 708213). Maybe it's a good idea to revisit that bug?

Comment 21

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

Since we know that evalInSandbox and evalInGlobal have observably different behavior, why not always use the debugger server for scratchpad? It seems to me that starting a debugger server and connecting directly if no toolbox is available would avoid all these issues with the sandbox fallback.

Comment 22

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #21)

I also thought about this, but how would you chose a target? Scratchpad doesn't currently have a way of connecting to things, so using a toolbox's target seemed like a good idea.

(I'm thinking about this under the assumption that one might want to use Scratchpad to run code on top of a remote target, which is not necessarily localhost and the same browser instance)

But Panos has a good point with things getting quite tricky when relying on the toolbox... what if there are multiple toolboxes? :)

Comment 23

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

Connecting to a remote target will always go through the remote connection screen, so, unless we plan on adding a scratchpad-specific connection screen/dialog, remote targets will always have a toolbox open. Your point about multiple toolboxes is spot on, too: there can already be multiple toolboxes open, in different tabs or windows.

I think the main issue you are alluding to though is what happens if the scratchpad is opened before a remote connection is made. This is a valid question and I think we can answer that by extrapolating from current behavior.

Scratchpad's current behavior is to target the active tab in the current window. This can be confusing to some (I think there have been bugs filed because people assumed that scratchpad's context wouldn't change after launch), but it is what it is. If we intend to preserve that (and I think it makes sense for the original scratchpad use case), then we can infer that when the current window is a detached devtools window targetting a remote instance, scratchpad should target that same remote instance.

Even though that is a reasonable way for things to work, I do believe that it's long past time we added a target indicator to the scratchpad window, just like we have in the detached toolbox window. We are currently using the titlebar in scratchpad to indicate the file name, but we could extend it with the target URL, or add a statusbar, or use a notification similar to the one for browser context, or do something else. I don't believe we are doing srcatchpad users a favor by not indicating clearly the execution context when it can be modified by a simple tab switch.

Comment 24

[Brandon Benvie [:benvie]]

(In reply to Panos Astithas [:past] from comment #21)
> Since we know that evalInSandbox and evalInGlobal have observably different
> behavior, why not always use the debugger server for scratchpad? It seems to
> me that starting a debugger server and connecting directly if no toolbox is
> available would avoid all these issues with the sandbox fallback.

This was originally my intention, but the current hybrid solution was something robcee and I arrived on to solve the issue where a Scratchpad is opened when no toolbox is open. Now that I've implemented that solution, and seeing the other issues that come up with it, I'm inclined to agree with your assessment.

Based on your comments, I think what needs to happen for this bug is that having a Scratchpad open needs to tie it to whatever the current window is. In order to implement that (I think?), the Scratchpad needs to be responsible for spinning up DebuggerServer/DebuggerClients for each tab/whenever the current tab changes.

Is this correct? Do you have any suggestions on the best way to implement this?

Comment 25

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #20)
> Note that things would greatly simplify if we're ever going to dock
> scratchpad (bug 708213). Maybe it's a good idea to revisit that bug?

It would, except there's still value in having the separate Scratchpad, as it provides the unique feature of being able to execute in the currently selected tab without having to open up separate toolboxes.

Comment 26

[Mihai Sucan [:msucan]]

Why not do what the browser console does? Open your own client-server connection and ignore the toolbox.

To handle the tab switching add a new root Scratchpad Actor on the server (as opposed to a tab actor). Only attach once to the new actor, then send all the eval requests to that.

The scratchpad actor can handle tab switching. It can create its own console actor instances for which you do not start any listeners (thus "lightweight" console actor instances), for every tab. Only initialize console actors per tab lazily - eg. when the user evals something.

You don't need to implement evaluation or handling of objects - just wrap and manage the console actors as needed.

Comment 27

[Brandon Benvie [:benvie]]

(In reply to Mihai Sucan [:msucan] from comment #26)
> Why not do what the browser console does? Open your own client-server
> connection and ignore the toolbox.
> 
> To handle the tab switching add a new root Scratchpad Actor on the server
> (as opposed to a tab actor). Only attach once to the new actor, then send
> all the eval requests to that.
> 
> The scratchpad actor can handle tab switching. It can create its own console
> actor instances for which you do not start any listeners (thus "lightweight"
> console actor instances), for every tab. Only initialize console actors per
> tab lazily - eg. when the user evals something.
> 
> You don't need to implement evaluation or handling of objects - just wrap
> and manage the console actors as needed.

Thanks Mihai! This is how I'm going to implement it.

Comment 28

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

(In reply to Brandon Benvie [:bbenvie] from comment #24)
> Based on your comments, I think what needs to happen for this bug is that
> having a Scratchpad open needs to tie it to whatever the current window is.
> In order to implement that (I think?), the Scratchpad needs to be
> responsible for spinning up DebuggerServer/DebuggerClients for each
> tab/whenever the current tab changes.
> 
> Is this correct? Do you have any suggestions on the best way to implement
> this?

You only need to start one DebuggerServer per process and one DebuggerClient per tool (or toolbox). In order to keep track of tab or window appearance or removal, you need to listen for a tabListChanged event that the root actor will send. This is a fairly recent addition and we don't use it yet in devtools. See the docs for more:

https://wiki.mozilla.org/Remote_Debugging_Protocol#The_Request.2FReply.2FNotify_Pattern

From a quick look at the patch it seems you already do most of the necessary work, but the Browser Debugger and Browser Console are also good examples of how to work outside the toolbox framework.

(In reply to Mihai Sucan [:msucan] from comment #26)
> Why not do what the browser console does? Open your own client-server
> connection and ignore the toolbox.
> 
> To handle the tab switching add a new root Scratchpad Actor on the server
> (as opposed to a tab actor). Only attach once to the new actor, then send
> all the eval requests to that.
> 
> The scratchpad actor can handle tab switching. It can create its own console
> actor instances for which you do not start any listeners (thus "lightweight"
> console actor instances), for every tab. Only initialize console actors per
> tab lazily - eg. when the user evals something.
> 
> You don't need to implement evaluation or handling of objects - just wrap
> and manage the console actors as needed.

I don't believe a separate scratchpad actor is necessary. When the current scratchpad context is the browser, you just need to use the global console actor to evaluate expressions. When the context is a tab, you use that tab's console actor. In the latter case you listen for tabListChanged events and fetch the tab list again before connecting to the console actor.

Actually, now that I think of it, if you get the tab list every time the user evaluates something, you don't even need to listen for tab list events. Caching the console actor you are connected to will be better for performance (and probably for other stateful functions, like displaying the current target in the titlebar), but since scratchpad has a fairly stateless interaction with its context, you could just delay getting the appropriate console actor until the very last moment.

Comment 29

[Mihai Sucan [:msucan]]

That's a good idea Panos. I didn't know of the new tabListChanged event.

Comment 30

[Brandon Benvie [:benvie]]

Attachment: WIP8

This patch has it always executing in browser. Next step is to make it evaluate in the current tab when content is selected.

Comment 31

[Brandon Benvie [:benvie]]

Attachment: WIP9

This patch now will execute in the currently selected tab, but doesn't currently handle executing in browser. Also began ripping out the old unneeded guts.

Comment 32

[Brandon Benvie [:benvie]]

Attachment: WIP10

This adds chrome support, but only for the initial chrome window. I'm not sure how to go about handling other chrome windows.

Comment 33

[Brandon Benvie [:benvie]]

Attachment: WIP11

I have a feeling I'm doing something *very* terrible here, but this actually works and evaluates in the currently active browser when context is set to "browser".

Comment 34

[Brandon Benvie [:benvie]]

Attachment: WIP12

This patch cleans up the last one, removing the importing of DebuggerServer and removing unnecessary state from ScratchpadTab and ScratchpadWindow.

Comment 35

[Brandon Benvie [:benvie]]

Attachment: WIP13

More cleanup and comments.

Comment 36

[Brandon Benvie [:benvie]]

Attachment: WIP14

This patch adds printing of errors. It's kind of ugly, but I think this is how it needs to be done. An error's "message", "stack", "lineNumber", and "fileName" properties are lazily created. As in:

> try {
>   throw new Error("test");
> } catch (e) {
>   Object.getOwnPropertyNames(e); // []
>   e.message;
>   Object.getOwnPropertyNames(e); // ["message"]
> }

So `grip.getPrototypeAndProperties` for the error won't provide the needed information. This patch gets around that by using `grip.getProperty` for each of the four properties, generating a promise for each and using `Promise.all` to wait for the collected results.

Another ugly part is that code evaluated using `webConsoleClient.evaluateJS` bears the fileName of "debugger eval code" and I didn't see how to change this. Right now I'm just using a string replace so it matches how the Scratchpad used to print errors.

Comment 37

[Brandon Benvie [:benvie]]

Attachment: WIP15

Apparently "displayString" was removed. This patch adds support for `ObjectActor#getDisplayString` (and the corresponding `GripClient#displayString`) that attempts to locate an object's `toString` method and then calls it on the object. This is used for the Scratchpad's "display" functionality. `Scratchpad.display` now resolves the promise it returns after everything is displayed, since we now have to go over the protocol in order to display anything that's not primitive.

Comment 38

[Brandon Benvie [:benvie]]

Attachment: WIP16

Includes changes from bug 807924 (localization of stringConversionFailed message as well as a test for it).

Comment 39

[Brandon Benvie [:benvie]]

Attachment: WIP16

Make it a patch. <:|

Comment 40

[Brandon Benvie [:benvie]]

Attachment: WIP17

* Make all the methods (run, inspect, display) make sure they wait for ultimate resolution before resolving the promises they return.
* Adds telemetry for DISPLAYSTRING (I had added this before the discussion on Friday)
* Fixes writeAsErrorComment
* Comments

Comment 41

[Brandon Benvie [:benvie]]

I'm at something a crossroads here. The problem is this:

Scratchpad's display functionality is the only thing in the devtools that can execute arbitrary user code (AKA debuggee code). That is because it essentially does `value + ''` on the completion value of the executed code, resulting in a call to toString. For example, Scratchpad display `({ toString: () => 'hi' })` displays "hi".

To support this functionality over the debugger protocol, I had to add a new method to ObjectActor/GripClient that does exactly that. Panos had a very valid objection that this would only ever be used by the Scratchpad, and even having it there puts it at odds with everything else in the devtools where we never implicitly execute debuggee code.

Assuming we were ok just continuing with the existing behavior in the Scratchpad, the alternative method to `toString`ing the result of the execution is doing something like `webConsoleClient.evaluateJS('eval("'+escapeJS(code)+'") + ""')` since the only way to capture the completion value of arbitrary JS, including statements, is via eval. This introduces other issues, like the code is now executing an extra frame deep, is executing in an eval scope, etc. It's also really ugly.

Another option is simply to remove the run and display functionality from the Scratchpad and just have all execution use inspect. This was something that had been suggested earlier, just as a way to simplify the UI of the Scratchpad and to match how it's mostly used anyway.

A third option is to create a new Scratchpad actor that has the dirty "runDebuggeeCodeToGetStringficationOfObject" method tacked onto it where we can stick this capability.

Comment 42

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

(In reply to Brandon Benvie [:bbenvie] from comment #41)
> Another option is simply to remove the run and display functionality from
> the Scratchpad and just have all execution use inspect. This was something
> that had been suggested earlier, just as a way to simplify the UI of the
> Scratchpad and to match how it's mostly used anyway.

+1

Comment 43

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

(In reply to Brandon Benvie [:bbenvie] from comment #41)
> Another option is simply to remove the run and display functionality from
> the Scratchpad and just have all execution use inspect. This was something
> that had been suggested earlier, just as a way to simplify the UI of the
> Scratchpad and to match how it's mostly used anyway.

Just to clarify, we would only need to remove the Display functionality, not remove Run as well, right? I find Run to be very useful when I'm interested in the side effects of execution (console.log, page manipulation), instead of the value of the last expression.

I've been a loyal user and fan of the display functionality in the past, but lately I've found the streamlined workflow of Cmd-I and ESC to be more useful, since I can identify objects faster in many cases by observing their shape in the inspector.

If we do decide that running debuggee code is OK, then a separate request to the object actor is better than a separate actor just for that purpose, provided we plaster scary warnings in API comments and in the Scratchpad introductory comment.

We should definitely get Rob's opinion here though. Scratchpad is his baby and he probably has parental advice to offer :-)

Comment 44

[Rob Campbell [:rc] (:robcee)]

I don't think we should remove either of them. They both have their uses. They exist for a reason.

Comment 45

[Brandon Benvie [:benvie]]

I think I have a good compromise for the "getDisplayString" functionality which I've currently implemented. Right now it simply calls the object's toString (executing debuggee code). I'll leave it as is for this bug so I can finally finish this off. I'll follow up with implementing a cleaned up version of it that doesn't execute debuggee code, and also results in cleaner output (like '[1, 2, 3]' instead of '1,2,3'). Something similar to original displayString property that the webconsole had and used for displaying the output.

Comment 46

[Victor Porof [:vporof][:vp]]

(In reply to Brandon Benvie [:bbenvie] from comment #45)

You rely on getString from the VariablesView (which the webconsole also uses). That way, in the future, if we make that smarter, everyone benefits!

Comment 47

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #44)
> I don't think we should remove either of them. They both have their uses.
> They exist for a reason.

I get eval's reason to exist (what Panos described), but what is the reason for display when we have (a good) inspect (now). Display is just a shittier representation of the result of the evaluation that is probably missing information and in a crappier UI.

What am I missing?

Comment 48

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #46)
> (In reply to Brandon Benvie [:bbenvie] from comment #45)
> 
> You rely on getString from the VariablesView (which the webconsole also
> uses). That way, in the future, if we make that smarter, everyone benefits!

This doesn't match the existing Scratchpad behavior. Display `[1, {}, 'a']` results in '1,[object Object],a'. I don't think it's desired for that to display '[object Array]'. Regardless, I feel strongly that this patch should not change the behavior of the Scratchpad, but rather that should end up in a future patch. To maintain the existing behavior of the Scratchpad I have to call the results `toString` method somehow.

I think the ultimate goal would be to have that display `[1, {}, 'a']` and that is better created on the server side of it and sent as a string, rather than built over multiple transports client <-> server. This would preclude it from being a VariablesView helper.

Comment 49

[Victor Porof [:vporof][:vp]]

(In reply to Brandon Benvie [:bbenvie] from comment #48)
> 
> This doesn't match the existing Scratchpad behavior. Display `[1, {}, 'a']`
> results in '1,[object Object],a'. I don't think it's desired for that to
> display '[object Array]'. Regardless, I feel strongly that this patch should
> not change the behavior of the Scratchpad, but rather that should end up in
> a future patch. To maintain the existing behavior of the Scratchpad I have
> to call the results `toString` method somehow.
> 

Ok, makes sense.

> I think the ultimate goal would be to have that display `[1, {}, 'a']` and
> that is better created on the server side of it and sent as a string, rather
> than built over multiple transports client <-> server. This would preclude
> it from being a VariablesView helper.

Yes, that should happen for all our tools. I strongly agree with the fact that this needs to be retrieved in one go, not over multiple transports. See bug 753332 for my proposal.

Comment 50

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #49)
> > I think the ultimate goal would be to have that display `[1, {}, 'a']` and
> > that is better created on the server side of it and sent as a string, rather
> > than built over multiple transports client <-> server. This would preclude
> > it from being a VariablesView helper.
> 
> Yes, that should happen for all our tools. I strongly agree with the fact
> that this needs to be retrieved in one go, not over multiple transports. See
> bug 753332 for my proposal.

Ah yes, that's the bug I was thinking of. I propose to basically use what I have here in my latest patch, and then to fix bug 753332 which would replace the getDisplayString here.

Comment 51

[Brandon Benvie [:benvie]]

Attachment: WIP18

Fixed most of the tests, 7 failures remaining. Make `Scratchpad.writeAsErrorComment` support DOM errors and objects without messages correctly... this function is turning into a monster. It has to do all the things that it does in order to replicate existing functionality though.

Comment 52

[Brandon Benvie [:benvie]]

Attachment: WIP19

Down to 3 test fails. These remaining failures are because the webconsole -> Scratchpad links. The web console tests the source URL of a file to detect if it originated from the Scratchpad, and if so it changes the link so that it will focus the Scratchpad when clicked. The problem is that since I'm using webConsoleClient.evaluateJS to execute the code, its origin is simply "debugger eval code" (this comes from Debugger.cpp `DebuggerGenericEval` and is not changeable). I glossed over this problem in Scratchpad.writeAsErrorComment by doing a string replace, but that's not going to fly here.

The question is: how can I hook up this location link to work correctly? I can't control the filename and I don't know another way to propagate the state of "this thing that was executed is for the Scratchpad".

Comment 53

[Brandon Benvie [:benvie]]

After putting some time into trying to find a workaround for this, the only solution I've been able to come up with is to make the various Debugger eval functions accept an optional filename parameter. This makes sense as a feature anyway since `Cu.evalInSandbox` allows specifying a filename and it would be useful for evaluation via the Debugger to have feature parity.

Comment 54

[Brandon Benvie [:benvie]]

Attachment: WIP20

Updated to use the patch from bug 889627, which allows for specifying the url when using Debugger eval. All tests pass.

Comment 55

[Brandon Benvie [:benvie]]

Attachment: WIP21

Rebase and change to use "promise" instead of "Promise".

Comment 56

[Brandon Benvie [:benvie]]

Attachment: WIP22

Clean up the imports a bit and switch some to use require. Also use "require" instead of "devtools.require".

Comment 57

[Brandon Benvie [:benvie]]

Attachment: WIP23

Adds comments and fixes inspecting null/undefined.

Comment 58

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

Panos, can we get a review from you for the dbg-client.jsm, script.js?

Comment 59

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

Panos, can we get a review from you for the dbg-client.jsm, script.js?

Mihai, can you take a look at the WebConsoleClient.jsm and webconsole.js changes?

Comment 60

[Mihai Sucan [:msucan]]

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

Patch looks good. The webconsole-related changes are minimal. r+ from me (do note I did not review the rest of the code).

I looked a bit into the scratchpad.js changes and I have only one important concern: garbage collection. I wasn't sure if you do this or not. Can you please check? ObjectActors need to be explicitly released, otherwise scratchpad will leak all objects it sees after every kind of eval, until the debugger connection is closed.

Unlike the js debugger, which has well-defined lifetimes for objects (pause and thread lifetimes), the console (and scratchpad) need to manage the release of objects on their own.

::: browser/devtools/webconsole/webconsole.js
@@ +3157,5 @@
>  
>      let evalOptions = {
>        bindObjectActor: aOptions.bindObjectActor,
>        frameActor: frameActor,
> +      url: aOptions.url,

Is this needed here? If there's no user of this option the web console code, we do not need to add it here, for now.

::: toolkit/devtools/server/actors/webconsole.js
@@ +579,5 @@
>  
>      let evalOptions = {
>        bindObjectActor: aRequest.bindObjectActor,
>        frameActor: aRequest.frameActor,
> +      url: aRequest.url,

Please make sure you also update the protocol description in the Web Console wiki page [1], after this patch lands.


[1] https://developer.mozilla.org/en-US/docs/Tools/Web_Console/remoting

@@ +896,5 @@
>      helpers.evalInput = aString;
>  
> +    let evalOptions;
> +    if (typeof aOptions.url == "string") {
> +      evalOptions = { url: aOptions.url };

Please also update the |aOptions| argument description for this function.

::: toolkit/devtools/webconsole/WebConsoleClient.jsm
@@ +109,5 @@
>        type: "evaluateJS",
>        text: aString,
>        bindObjectActor: aOptions.bindObjectActor,
>        frameActor: aOptions.frameActor,
> +      url: aOptions.url,

Please update the comment for this function to include new property for |aOptions|.

Comment 61

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

this looks really good. Couple of minor nits below. r+ for the scratchpad.js and scratchpad.xul bits.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1575,5 @@
> +   *         The promise for the result of connecting to this tab or window.
> +   */
> +  connect: function ST_connect()
> +  {
> +    if (this._connecter) {

I don't wanna be that guy, but I think you mean "_connector". With an 'o'. maybe.

::: toolkit/devtools/server/actors/script.js
@@ +1797,5 @@
> +    } catch (e) {}
> +
> +    let result = null;
> +    if (toString && toString.callable) {
> +      // If a toString method was found then call it on the object.

do we need more commenting on this? This is a potential failure-point, right if someone does something with their toString() method?

Comment 62

[Brandon Benvie [:benvie]]

Thanks for the review!

(In reply to Rob Campbell [:rc] (:robcee) from comment #61)
> Comment on attachment 775876 [details] [diff] [review]
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +1575,5 @@
> > +   *         The promise for the result of connecting to this tab or window.
> > +   */
> > +  connect: function ST_connect()
> > +  {
> > +    if (this._connecter) {
> 
> I don't wanna be that guy, but I think you mean "_connector". With an 'o'.
> maybe.

Oops. <:|

> ::: toolkit/devtools/server/actors/script.js
> @@ +1797,5 @@
> > +    } catch (e) {}
> > +
> > +    let result = null;
> > +    if (toString && toString.callable) {
> > +      // If a toString method was found then call it on the object.
> 
> do we need more commenting on this? This is a potential failure-point, right
> if someone does something with their toString() method?

Probably needs more commenting, but the functionality is sound. It will only call the function if it is callable, and it will only use the result if it got a string return (a throw or a non-string return will be ignored). I'll note this.

Comment 63

[Brandon Benvie [:benvie]]

(In reply to Mihai Sucan [:msucan] from comment #60)
> I looked a bit into the scratchpad.js changes and I have only one important
> concern: garbage collection. I wasn't sure if you do this or not. Can you
> please check? ObjectActors need to be explicitly released, otherwise
> scratchpad will leak all objects it sees after every kind of eval, until the
> debugger connection is closed.

Hmm I see. I guess basically what I'll have to do is every time something is inspected, any existing stuff will need to be released. This is simple enough to do with VariableViewController#releaseActors. I'll also need to so that when the Scratchpad is closed.

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +3157,5 @@
> >  
> >      let evalOptions = {
> >        bindObjectActor: aOptions.bindObjectActor,
> >        frameActor: frameActor,
> > +      url: aOptions.url,
> 
> Is this needed here? If there's no user of this option the web console code,
> we do not need to add it here, for now.

Ah yes, not sure why I added that there. Will remove it.


I'll make sure to update the docs and the comments you pointed out. Thanks for reviewing this for me!

Comment 64

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

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

I only have a couple of nits about the debugger bits, but the forRemoteTab call seems wrong. I haven't looked thoroughly at scratchpad.js though.

::: browser/devtools/scratchpad/scratchpad.js
@@ +1619,5 @@
> +   *         The promise for the TabTarget for this tab.
> +   */
> +  _attach: function ST__attach()
> +  {
> +    return TargetFactory.forRemoteTab(this._tab);

This doesn't seem right. forRemoteTab takes a tab form as an argument, not a XUL tab. What you should probably have done instead is:

Target.forTab(this._tab).makeRemote()

::: toolkit/devtools/server/actors/script.js
@@ +1781,5 @@
> +   *
> +   * @param aRequest object
> +   *        The protocol request object.
> +   */
> +  onDisplayString: function OA_onDisplayString(aRequest) {

How about the scary API comment about executing debuggee code that I mentioned in comment 43?

@@ +1793,5 @@
> +          toString = desc.value;
> +          break;
> +        }
> +      } while (obj = obj.proto)
> +    } catch (e) {}

Swallowing errors scares me, let's at least dumpn() the error to the terminal.

Comment 65

[Victor Porof [:vporof][:vp]]

Comment on attachment 775876 [details] [diff] [review]
WIP23

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +1778,5 @@
>     */
>    _update: function SS__update(aObject)
>    {
> +    let view = this.variablesView;
> +    view.createHierarchy();

If you're never doing a commitHierarchy, then this is probably not needed.

Comment 66

[Brandon Benvie [:benvie]]

Thanks for the review!

(In reply to Panos Astithas [:past] from comment #64)
> Comment on attachment 775876 [details] [diff] [review]
> WIP23
> 
> Review of attachment 775876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only have a couple of nits about the debugger bits, but the forRemoteTab
> call seems wrong. I haven't looked thoroughly at scratchpad.js though.
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +1619,5 @@
> > +   *         The promise for the TabTarget for this tab.
> > +   */
> > +  _attach: function ST__attach()
> > +  {
> > +    return TargetFactory.forRemoteTab(this._tab);
> 
> This doesn't seem right. forRemoteTab takes a tab form as an argument, not a
> XUL tab. What you should probably have done instead is:
> 
> Target.forTab(this._tab).makeRemote()

I was originally doing this, but I changed it to use forRemoteTab in anticipation of this being used for remote targets (bug 895180). Also forRemoteTab did exactly what I needed (returns a promise that returns the target). Technically forRemoteTab passes the options argument directly to `new TabTarget(options)` so that's why it still actually works. I'll change it back for this bug since there's no remote context yet.

> ::: toolkit/devtools/server/actors/script.js
> @@ +1781,5 @@
> > +   *
> > +   * @param aRequest object
> > +   *        The protocol request object.
> > +   */
> > +  onDisplayString: function OA_onDisplayString(aRequest) {
> 
> How about the scary API comment about executing debuggee code that I
> mentioned in comment 43?

So my compromise for this was basically that for right now this will execute debuggee code, but I will follow up with changing it to something that doesn't execute any debuggee code and also returns a more useful string (or a more complex description that can be turned into a string, see https://bugzilla.mozilla.org/show_bug.cgi?id=753332#c22).


> @@ +1793,5 @@
> > +          toString = desc.value;
> > +          break;
> > +        }
> > +      } while (obj = obj.proto)
> > +    } catch (e) {}
> 
> Swallowing errors scares me, let's at least dumpn() the error to the
> terminal.

This would be DebuggeeWouldRun errors if the debuggee is a Proxy, although that's not actually implemented (DebuggerObject#getOwnPropertyDescriptor on a proxy will silently run debuggee code currently). I'll add a dumpn though should it change in the future.

Comment 67

[Brandon Benvie [:benvie]]

Attachment: WIP24

* release actors when they are no longer needed
* add localization for the timeout message
* don't silently eat errors in onDisplayString
* more comments in onDisplayString
* add comments for new url option
* use TargetFactory.forTab instead of forRemoteTab
* don't createHierachy when loading an object into the VariablesView

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

Comment 68

[Brandon Benvie [:benvie]]

bc failures are unrelated (blackboxing test failures).

Comment 69

[Brandon Benvie [:benvie]]

If at first your try doesn't succeed, try try again.

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

Comment 70

[Brandon Benvie [:benvie]]

Arg apparently the permaorange blackboxing patch hadn't been backed out by the time I submitted the try.

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

Comment 71

[Brandon Benvie [:benvie]]

Try was broken, let's try again.

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

Comment 72

[Brandon Benvie [:benvie]]

Attachment: WIP25

Fixed the test fail that snuck in.

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

Comment 73

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

And there was much rejoicing:
https://hg.mozilla.org/integration/fx-team/rev/25efc66ef8d8

Comment 74

[Brandon Benvie [:benvie]]

Woo!

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25efc66ef8d8

Comment 76

[Philipp Kewisch [:Fallen][📱📆]]

This caused a warning in the line:

> } while (obj = obj.proto);

I don't have the exact message, but something like: test for equality (==) mistyped as assignment (=)?

I suggest extra parenthesis to avoid the warning. New bug or do you want to fix it here?

Comment 77

[Girish Sharma [:Optimizer]]

(In reply to Philipp Kewisch [:Fallen] from comment #76)
> This caused a warning in the line:
> 
> > } while (obj = obj.proto);
> 
> I don't have the exact message, but something like: test for equality (==)
> mistyped as assignment (=)?
> 
> I suggest extra parenthesis to avoid the warning. New bug or do you want to
> fix it here?

new bug. this one's already resolved fixed.

Comment 78

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

(In reply to Philipp Kewisch [:Fallen] from comment #76)
> This caused a warning in the line:
> 
> > } while (obj = obj.proto);
> 
> I don't have the exact message, but something like: test for equality (==)
> mistyped as assignment (=)?
> 
> I suggest extra parenthesis to avoid the warning. New bug or do you want to
> fix it here?

I already have a fix for this in the patch for bug 810966.