Make Scratchpad execute in context of current debugger

RESOLVED FIXED in Firefox 25

Status

defect
P3
normal
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: julian.viereck, Assigned: bbenvie)

Tracking

(Blocks 3 bugs)

Trunk
Firefox 25
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [woo])

Attachments

(1 attachment, 28 obsolete attachments)

49.34 KB, patch
Details | Diff | Splinter Review
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
: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.
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.
Priority: -- → P3
I think this bug will do to cover my current work on making the scratchpad execute using the debugger protocol.
Assignee: nobody → bbenvie
Depends on: 851381
Status: NEW → ASSIGNED
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
There's a similar effort going on at https://github.com/mozilla/r2d2b2g/pull/415
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.
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.
Depends on: 783499
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.
Depends on: 808369
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.
Blocks: 690529
Posted patch WIP2 (obsolete) — Splinter Review
Rebases the patch to the tentative bug808369 patch.
Attachment #731372 - Attachment is obsolete: true
Posted file WIP2.1 (obsolete) —
Remove a stray "+".
Attachment #738619 - Attachment is obsolete: true
Posted patch WIP2.1 (obsolete) — Splinter Review
Make it a patch...
Attachment #738812 - Attachment is obsolete: true
Posted patch WIP2.2 (obsolete) — Splinter Review
rebase to latest fx-team
Attachment #738813 - Attachment is obsolete: true
Depends on: 867458
Depends on: 828680
Posted patch WIP3 (obsolete) — Splinter Review
Uses the updated VariablesViewController.
Attachment #743919 - Attachment is obsolete: true
Posted patch WIP4 (obsolete) — Splinter Review
Rebase to latest bug 828680.
Attachment #755542 - Attachment is obsolete: true
Posted patch WIP5 (obsolete) — Splinter Review
rebase
Attachment #758307 - Attachment is obsolete: true
Depends on: 869984
Posted patch WIP6 (obsolete) — Splinter Review
Rebase to bug 869984.
Attachment #758699 - Attachment is obsolete: true
Posted patch WIP7 (obsolete) — Splinter Review
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?
Attachment #764322 - Attachment is obsolete: true
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.
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?
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.
(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? :)
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.
(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?
(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.
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.
(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.
(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.
That's a good idea Panos. I didn't know of the new tabListChanged event.
Posted patch WIP8 (obsolete) — Splinter Review
This patch has it always executing in browser. Next step is to make it evaluate in the current tab when content is selected.
Attachment #764433 - Attachment is obsolete: true
Posted patch WIP9 (obsolete) — Splinter Review
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.
Attachment #766147 - Attachment is obsolete: true
Posted patch WIP10 (obsolete) — Splinter Review
This adds chrome support, but only for the initial chrome window. I'm not sure how to go about handling other chrome windows.
Attachment #766875 - Attachment is obsolete: true
Posted patch WIP11 (obsolete) — Splinter Review
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".
Attachment #766903 - Attachment is obsolete: true
Posted patch WIP12 (obsolete) — Splinter Review
This patch cleans up the last one, removing the importing of DebuggerServer and removing unnecessary state from ScratchpadTab and ScratchpadWindow.
Attachment #766987 - Attachment is obsolete: true
Posted patch WIP13 (obsolete) — Splinter Review
More cleanup and comments.
Attachment #767328 - Attachment is obsolete: true
Posted patch WIP14 (obsolete) — Splinter Review
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.
Attachment #767438 - Attachment is obsolete: true
Posted patch WIP15 (obsolete) — Splinter Review
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.
Attachment #767904 - Attachment is obsolete: true
Blocks: 807924
Posted file WIP16 (obsolete) —
Includes changes from bug 807924 (localization of stringConversionFailed message as well as a test for it).
Attachment #768630 - Attachment is obsolete: true
Posted patch WIP16 (obsolete) — Splinter Review
Make it a patch. <:|
Attachment #768636 - Attachment is obsolete: true
Posted patch WIP17 (obsolete) — Splinter Review
* 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
Attachment #768637 - Attachment is obsolete: true
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.
Flags: needinfo?
(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
Flags: needinfo?
Flags: needinfo?
(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 :-)
Flags: needinfo? → needinfo?(rcampbell)
I don't think we should remove either of them. They both have their uses. They exist for a reason.
Flags: needinfo?(rcampbell)
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.
(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!
(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?
(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.
(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.
(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.
Posted patch WIP18 (obsolete) — Splinter Review
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.
Attachment #769743 - Attachment is obsolete: true
Posted patch WIP19 (obsolete) — Splinter Review
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".
Attachment #770368 - Attachment is obsolete: true
Flags: needinfo?
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.
Depends on: 889627
Flags: needinfo?
Posted patch WIP20 (obsolete) — Splinter Review
Updated to use the patch from bug 889627, which allows for specifying the url when using Debugger eval. All tests pass.
Attachment #770449 - Attachment is obsolete: true
Attachment #772917 - Attachment is patch: true
Attachment #772917 - Attachment mime type: text/x-patch → text/plain
Posted patch WIP21 (obsolete) — Splinter Review
Rebase and change to use "promise" instead of "Promise".
Attachment #772917 - Attachment is obsolete: true
Posted patch WIP22 (obsolete) — Splinter Review
Clean up the imports a bit and switch some to use require. Also use "require" instead of "devtools.require".
Attachment #774223 - Attachment is obsolete: true
Posted patch WIP23 (obsolete) — Splinter Review
Adds comments and fixes inspecting null/undefined.
Attachment #774232 - Attachment is obsolete: true
Blocks: 894997
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?
Attachment #775876 - Flags: review?(past)
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?
Attachment #775876 - Flags: review?(mihai.sucan)
Blocks: 895180
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|.
Attachment #775876 - Flags: review?(mihai.sucan) → review+
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?
Attachment #775876 - Flags: review?
Attachment #775876 - Flags: review+
Attachment #775876 - Flags: review? → review+
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.
(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 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.
Attachment #775876 - Flags: review?(past) → review+
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.
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.
Posted patch WIP24 (obsolete) — Splinter Review
* 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
Attachment #775876 - Attachment is obsolete: true
bc failures are unrelated (blackboxing test failures).
If at first your try doesn't succeed, try try again.

https://tbpl.mozilla.org/?tree=Try&rev=f609adada5db
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
Try was broken, let's try again.

https://tbpl.mozilla.org/?tree=Try&rev=d43a30c1c662
Posted patch WIP25Splinter Review
Fixed the test fail that snuck in.

https://tbpl.mozilla.org/?tree=Try&rev=97bba509188a
Attachment #778605 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
Blocks: 678971
And there was much rejoicing:
https://hg.mozilla.org/integration/fx-team/rev/25efc66ef8d8
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Woo!
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][woo]
https://hg.mozilla.org/mozilla-central/rev/25efc66ef8d8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][woo] → [woo]
Target Milestone: --- → Firefox 25
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?
(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.
(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.
No longer blocks: 853359
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.