783499 - Web Console should use the debugger API

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Thaddee Tyl [:espadrine]]

This is an issue of parity with Firebug, Safari, Chrome, Opera, and probably IE.

While on a breakpoint, the WebConsole should access (and autocomplete on, and modify)
the local scope.
Unfortunately, right now, it sees only the global scope.

Example: on all listed competitors, debugging the dummy page <http://jsbin.com/amanep/3>,
with a breakpoint set in the inline script, allows to modify the local `n` variable by
entering the following in the console:

> n ++

Also, if the variable name was longer, you would appreciate the fact that you can autocomplete
the variable name with all variables in scope.

One edge we maybe can have on competitors on this ground is to immediately show the update.
On all competitors which I tested, the local variables shown on a side pane do not update
as you modify them from the console, until you hit “step over”.

Comment 1

[Mihai Sucan [:msucan]]

Thanks for your report.

This sounds like the Web Console should use the threadactor when the debugger is active. Otherwise I doubt we can make it work.

Adding bug 768096 as a dependency, but that will only be a step towards a fix for this bug. I will file another bug for migrating the Web Console to use the debugger API when possible.

Comment 2

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

This is a great feature to have, but I think it will need a different approach. Currently the web console, as well as the other forthcoming remotable tools, creates a separate protocol connection to the debugger server, which is both simple and elegant. All actors however are bound to the connection pool, so each client will maintain a separate state of the page and will not be able to observe the state changes made by the others.

In order to get the web console to affect the current debugger state, we could either substantially change the protocol or provide a debugger-frontend API for other tools to use. I think I prefer the latter approach, but I'd like to hear Jim's thoughts, too.

Comment 3

[Thaddee Tyl [:espadrine]]

I prefer the debugger frontend API too.
However, you got me wondering: do we need to worry about backward compatibility in the protocol?

Comment 4

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

(In reply to Thaddee Tyl [:espadrine] from comment #3)
> I prefer the debugger frontend API too.
> However, you got me wondering: do we need to worry about backward
> compatibility in the protocol?

Yes, in general. In this case only if we pick the first option.

Another thought that occurred to me, is that if we agree with Paul's idea of never displaying the console and the debugger simultaneously, it will render the issue moot.

Also, I wonder how many people will be still using the web console for evaluating expressions, after we implement watch expressions in the debugger (bug 727429).

Comment 5

[Thaddee Tyl [:espadrine]]

(In reply to Panos Astithas [:past] from comment #4)
> Another thought that occurred to me, is that if we agree with Paul's idea of
> never displaying the console and the debugger simultaneously, it will render
> the issue moot.
> 
> Also, I wonder how many people will be still using the web console for
> evaluating expressions, after we implement watch expressions in the debugger
> (bug 727429).

I do know that a lot of developers use the webconsole while debugging, and not just to watch expressions (which is obviously a feature I would love to see, too, and that is also in Chrome, with a very nice mouseover UI). They modify the environment dynamically. They call functions.

I did not know that Paul wanted to never display the console and the debugger simultaneously, but this suggestion has severe UX drawbacks. Users noticing that the (possibly docked out) webconsole window disappears suddenly, losing all of their defined variables, will consider it a bug, not a feature.

I do realize that this bug is hard to implement, and it probably has something to do with bug 774753 too. However, we need this to be any competitive.

Comment 6

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

I feel strongly that we should support the behavior outlined by this bug.

When I am debugging a program, I don't know exactly what I want to watch or change yet (hence I am debugging and not coding) and I need an environment that lets me play around and test little things. As I play around, the things I want to modify and watch change very quickly as I rule out possibilities or have realizations. This is the use case for having the webconsole in the local scope. There is just nothing else that is going to let me iterate on ideas as fast. Watch expressions are cool, but don't quite solve the exact same problem.

That said, I could be appeased by having a scratchpad window that is always in the context of the local scope the debugger is paused in, but I suspect that most users would be more familiar with the console being in the local scope. They are used to it because every other browser's devtools support this.

Comment 7

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

I was thinking mostly of other debugging environments (like Eclipse), where watch expressions or inspection popups fulfill the same role. Of course in browser-land we have consoles and it's quite possible most users will still prefer to use them. In that case a web console that works on an invisible clone of the page is definitely something we would want to fix.

Comment 9

[Mihai Sucan [:msucan]]

Fixing this bug mainly requires work in the Web Console-related code (actors and so on).

Comment 10

[Jim Blandy :jimb]

(In reply to Panos Astithas [:past] from comment #4)
> Also, I wonder how many people will be still using the web console for
> evaluating expressions, after we implement watch expressions in the debugger
> (bug 727429).

I would bet that people will want to continue in their established habits, so I'd expect web console usage to continue for a long time. (If we somehow integrate the web console and debugger UI, that might effectively end the web console as a separate thing.)

If I understand correctly, people are discussing re-implementing the web console on top of (essentially) ThreadClient --- is that right? That would certainly make the full power of the debugger available to the web console, and it seems clean.

Whether the debugger UI and web console would share the same ThreadClient instance, or each create their own, is an interesting question.

I'm not sure how to get live updates to show up; we don't have anything like a watchpoint on Debugger.Environment instances. But perhaps something simpler would be good enough --- just a "somebody modified something" hook, that would be called whenever the debuggee ran, just enough to tell the UI to re-check the values of the variables it's displaying.

(It seems like we'll have bug 774753 fixed soon.)

Comment 11

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

(In reply to Jim Blandy :jimb from comment #10)
> (In reply to Panos Astithas [:past] from comment #4)
> > Also, I wonder how many people will be still using the web console for
> > evaluating expressions, after we implement watch expressions in the debugger
> > (bug 727429).
> 
> I would bet that people will want to continue in their established habits,
> so I'd expect web console usage to continue for a long time. (If we somehow
> integrate the web console and debugger UI, that might effectively end the
> web console as a separate thing.)

Agreed.

> If I understand correctly, people are discussing re-implementing the web
> console on top of (essentially) ThreadClient --- is that right? That would
> certainly make the full power of the debugger available to the web console,
> and it seems clean.

Right, but note the problem I mention in comment 2.

> Whether the debugger UI and web console would share the same ThreadClient
> instance, or each create their own, is an interesting question.

But since different clients/connections will get different ThreadActors we would not be able to observe the same debuggee state in this case. I know Firebug will try this, so it'll be interesting to see how it plays out.

> I'm not sure how to get live updates to show up; we don't have anything like
> a watchpoint on Debugger.Environment instances. But perhaps something
> simpler would be good enough --- just a "somebody modified something" hook,
> that would be called whenever the debuggee ran, just enough to tell the UI
> to re-check the values of the variables it's displaying.
> 
> (It seems like we'll have bug 774753 fixed soon.)

This will be a good topic for discussion in the work week.

Comment 12

[Pedro Alves]

This is the feature that will allow to bury firebug.

Don't forget to do this not only for webconsole (thus achieving parity with other tools / debugger) but also scratchpad (to rock their socks!)

Comment 13

[Thaddee Tyl [:espadrine]]

Pedro: competition is beneficial!

Panos: is there no way to share the ThreadActor through the same connection? Did Honza start experimenting with this?

Comment 14

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

(In reply to Thaddee Tyl [:espadrine] from comment #13)
> Panos: is there no way to share the ThreadActor through the same connection?
> Did Honza start experimenting with this?

That's essentially the second option I described in comment 2. It will require the console to hook into parts that currently belong to the debugger frontend (debugger-controller.js mostly). I think this is a viable approach.

Comment 15

[Jim Blandy :jimb]

For what it's worth, I think this could be addressed pretty easily, once we've switched WebConsole to use the Debugger API to do the eval instead of the sandbox.

- There's no need for the debugger and WebConsole to share a connection.

By design, it's okay to have multiple Debuggers watching the same debuggees. They don't interfere with each other: they each get their own Debugger.Object and Debugger.Frame instances, and so on. If one Debugger has an effect on the debuggee, the other Debuggers will see it.

- The performance impact can be short-lived.

A compartment is only in debug mode while it is a debuggee of some Debugger. Although adding or removing a global flushes all the JIT code for that compartment, so do many other operations, including GC. JIT code is supposed to be tossable. So the web console could keep a Debugger around (or even just create a fresh one for every eval; it's just an object), add the window as a debuggee, do the eval, and then immediately remove the window as a debuggee. The JIT code will get regenerated as needed.

- Once you're using Debugger for the eval, it's very easy to grab the youngest debuggee frame.

The code would look something like this (assuming 'global' is a window):

try {
  var globalWrapper = dbg.addDebuggee(global);
  f = dbg.getNewestFrame();
  if (f)
    return f.evalWithBindings(expr, bindings);
  else
    globalWrapper.evalInGlobalWithBindings(expr, bindings);
} finally {
  dbg.removeDebuggee(global);
}

Comment 16

[Mihai Sucan [:msucan]]

After a discussion with Rob on IRC, here's a summary of the current understanding of things. For this bug to be fixed we need to take into consideration the following:

1. to use the Debugger API to evaluate expressions we need to enable the debugger on the page - performance hit for js-heavy pages.

2. the above would be acceptable if we would only do it when you evaluate js, but...

3. the web console directly accesses content objects - we don't use the Debugger.Object wrapper.

4. the above point simply means the web console cannot access any object in the page during the paused state.

5. to access any object in the page we need to use the Debugger API - Debugger.Object and friends.

6. the above point means that we would need to enable the debugger api from the start of the web console - performance hit for js-heavy pages.

7. even if we accept the performance hit, we do have a concern: can we have object grips while the debugger is not in paused state? Can we work with Debugger.Object and other APIs while the web page is running?

We want to avoid performance regressions and having to work on two different code paths in the Web Console.

Comment 17

[Julian Viereck]

Can we only use the Debugger API to evaluate expressions IF the debugger is paused? If it is not paused, can we use the current expression evaluation strategy?

Comment 18

[Mihai Sucan [:msucan]]

(In reply to Julian Viereck from comment #17)
> Can we only use the Debugger API to evaluate expressions IF the debugger is
> paused? If it is not paused, can we use the current expression evaluation
> strategy?

That's the two code paths problem...

Comment 19

[Julian Viereck]

>
> That's the two code paths problem...

Right. Can we make this less painful by pushing the "two code paths" down the stack? E.g. have the webconsole talk over the remote debugging protocol to eval some JS and the debugger server then decides to either eval the string directly in the sandbox (if the debugger is not paused) or eval it in the current stack frame (if the debugger is paused).

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Julian Viereck from comment #19)
> >
> > That's the two code paths problem...
> 
> Right. Can we make this less painful by pushing the "two code paths" down
> the stack? E.g. have the webconsole talk over the remote debugging protocol
> to eval some JS and the debugger server then decides to either eval the
> string directly in the sandbox (if the debugger is not paused) or eval it in
> the current stack frame (if the debugger is paused).

Debugger eval gives me a Debugger.Object for the result, sandbox eval gives the actual result object. The web console actor only knows how to work with actual objects - for inspection and anything else we need. If I could access the actual content object, sure, that would work - just eval with the debugger api and for the rest, use the typical approach.

What we can do is to make the WebConsoleObjectActor and the debugger ObjectActor more compatible - provide same request and response packets, such that the Web Console client can use WCOA and OA interchangeably, without any two code paths. The Web Console actor would, then, not need to change much - apart from the eval code that does what you said: when the debugger is paused the debugger API is used to eval and an OA instance is created and sent to the client, otherwise WCOA is used. This could work really nice up to a certain point: once the debugger is not paused I am not sure if the user can continue to inspect/interact with the objects he sees in the Web Console output.

Nonetheless, this is the path I'd like to try first. Thanks for your suggestion.

Comment 21

[Jim Blandy :jimb]

It's correct that using Debugger to do the evaluation won't let you get at the objects directly, and so WCOA will need to learn to operate on Debugger.Object instances.

However, note that Debugger.Object instances can be used even when the global in whose scope they were allocated is not a debuggee. That is, you can add a debuggee, evaluate to get Debugger.Objects, remove that debuggee (thus ending the performance impact), and continue to operate on those Debugger.Objects. Debuggee-ness only affects interacts with frames and functions, not objects.

When you ask, "Can we work with Debugger.Object and other APIs while the web page is running", I'm not sure exactly what you mean. Obviously, there's only a single thread, and our tools are written as event handlers that run interleaved with content's event handlers, so whenever we might be messing with a Debugger.Object, it's a given that the web page is not running.

But what I *think* you mean is, can we use Debugger.Object without having put the debuggee in any special state --- a frozen event loop, added as a debuggee, etc. And the answer to that is, yes: a Debugger.Object is just a pointer to an object, with a nice reflection API.

I'm sorry this answer is so long; hopefully it is helpful.

Comment 22

[Mihai Sucan [:msucan]]

(In reply to Jim Blandy :jimb from comment #21)
> It's correct that using Debugger to do the evaluation won't let you get at
> the objects directly, and so WCOA will need to learn to operate on
> Debugger.Object instances.
> 
> ...
> debuggee, etc. And the answer to that is, yes: a Debugger.Object is just a
> pointer to an object, with a nice reflection API.

This seems to be what I need.

> I'm sorry this answer is so long; hopefully it is helpful.

No problem. Thank you very much for your reply - this gives me something to work with.

I'm going to take this bug. I hope to have a patch soon.

Comment 23

[Jim Blandy :jimb]

It would obviously be easier to maintain if WCOA could avoid having two paths: one for dealing with Debugger.Object instances and one for mussing with objects directly. I think this could be done.

Note that it is possible to make a Debugger.Object referring to a given object, even if no global is a debuggee. For example:

var debugger = new Debugger;

// addDebuggee returns a D.O referring to window.
var DOwindow = debugger.addDebuggee(window);

debugger.removeDebuggee(DOwindow);
// Now debugger has no debuggees. But the D.O's are still usable.

// makeDebuggeeValue returns a D.O referring to random_object.
var DOrandom = DOwindow.makeDebuggeeValue(random_object); 

// Now you can operate on DOrandom, but no debugging overhead is
// imposed on window, as it is not a debuggee.

My suggestion would be that, if the web console is going to have code to deal with Debugger.Object instances anyway, it should make that the *only* case, and produce D.Os for all the objects it operates on. Thus, we'll be exercising the same code paths in both the eval-in-global and eval-in-frame cases.

Comment 24

[Mihai Sucan [:msucan]]

Thank you for the additional clarification. That's what I have in mind - switch to only use D.O instances, so we avoid having two code paths.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: wip 1

Work-in-progress patch. This is almost ready for review - I'm only missing new tests and fixes for existing web console tests (in toolkit/).

This patch contains approximately only the changes in toolkit for the Web Console actors and for the debugger script actors. All of the debugger tests continue to pass. 

This is not meant to be tested alone - no web console frontend updates. You can find those changes in bug 808370 (going to attach a WIP patch there as well). This patch separation is, hopefully, for easier review. If you want me to fold the patches, or if you want a different split, please let me know.

There are some remaining issues:

- waiting for bug 830818 - many properties show only as getters/setters. This is breaks several tests.
- waiting for test fixes in bug 820524.
- there are imports from browser/, in toolkit/ code.
- there are some issues (maybe corner-cases?) caused by the fact the web console actor does not share the Debugger instance with the ThreadActor. See evalWithDebugger().
- I had to do some slightly unorthodox hacks to get the following jsterm helpers to work as desired: values(), keys() and $x(). See their implementation for details. I hope bug 830818 will allow me to fix the issue.
- there's at least one TODO marked as a follow up bug to report: autocomplete should use the debugger API. As it is now it continues to work, even while the debugger is paused - autocomplete code tries to never execute content code.

Please note that you need the patch from bug 820524 applied before you apply this patch.

Looking forward for your feedback. I believe we should get started with reviewing these patches before all of the tests are ready. Thank you!

Comment 26

[Mihai Sucan [:msucan]]

> This is breaks several tests.

argh typos. This is breaking several tests...

Comment 27

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

Comment on attachment 706558 [details] [diff] [review]
wip 1

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

I haven't had a chance to play with this due to the bitrotted patch in bug 808370, but it seems to be in accordance with our discussed plan.

::: toolkit/devtools/webconsole/WebConsoleClient.jsm
@@ +98,5 @@
> +   *        eval:
> +   *          _self["prop"] = value;
> +   *
> +   *        - frameActor: a FrameActor ID. The FA holds a reference to
> +   *        a Debugger.Frame. This option allow you to evaluate the string in

s/allow/allows/

::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
@@ +31,5 @@
> +// FIXME: toolkit imports from browser. Can we move vview to toolkit or...?
> +// It looks like we already import toolbox jsm's from browser/...
> +// Maybe we can recheck how strongly WCA depends on WCU. If it's not much, move
> +// this back to browser/? I wouldn't favor this kind of back and forth, but it's
> +// an option.

Good questions, but I'm afraid I don't have any good answers. I need to think more about this.

@@ +1551,5 @@
>    aOwner.sandbox.$x = function JSTH_$x(aXPath, aContext)
>    {
> +    // Work around bug 830818. If we return an object from chrome code the
> +    // debuggee value will point to the proxy that wraps the chrome object.
> +    let nodes = new aOwner.window.wrappedJSObject.Array();

This comment is not accurate, I don't think this has anything to do with bug 830818. This is a consequence of how the Debugger API is designed. Quoting the spec:

"JavaScript code in different compartments can have different views of the same object. For example, in Firefox, code in privileged compartments sees content DOM element objects without redefinitions or extensions made to that object's properties by content code. (In Firefox terminology, privileged code sees the element through an "xray wrapper".) To ensure that debugger code sees each object just as the debuggee would, each Debugger.Object instance presents its referent as it would be seen from a particular compartment. This "viewing compartment" is chosen to match the way the debugger came across the referent. As a consequence, a single Debugger instance may actually have several Debugger.Object instances: one for each compartment from which the referent is viewed."

In light of the above, I wouldn't call this a workaround, but as the proper way to implement these console helpers.

@@ +1611,5 @@
>    aOwner.sandbox.keys = function JSTH_keys(aObject)
>    {
> +    // Work around bug 830818. If we return an object from chrome code the
> +    // debuggee value will point to the proxy that wraps the chrome object.
> +    return aOwner.window.wrappedJSObject.Object.keys(WebConsoleUtils.unwrap(aObject));

Same here.

@@ +1625,5 @@
>    aOwner.sandbox.values = function JSTH_values(aObject)
>    {
> +    // Work around bug 830818. If we return an object from chrome code the
> +    // debuggee value will point to the proxy that wraps the chrome object.
> +    let arrValues = new aOwner.window.wrappedJSObject.Array();

And here.

::: toolkit/devtools/webconsole/dbg-webconsole-actors.js
@@ +72,5 @@
>  
>    this._prefs = {};
> +
> +  // TODO: maybe we can reuse the Debugger() from the ThreadActor, if it's
> +  // available. Thoughts? See complications in evalWithDebugger().

Frankly, this is what I would expect: use the ThreadActor's Debugger if the client is attached to the thread, and use a console-private Debugger otherwise.

I don't understand the problems it creates though. From my reading of evalWithDebugger it seems like it would help, no?

@@ +256,3 @@
>      // We need to unwrap the object, otherwise we cannot access the properties
>      // and methods added by the content scripts.
> +    let actor = new ObjectActor(aObject, this);

Comment does not apply any longer.

@@ +646,3 @@
>     *         The result of the evaluation.
>     */
> +  evalWithDebugger: function WCA_evalWithDebugger(aString, aOptions = {})

I thought this would be a ThreadActor method, perhaps in addition to being here, because I remember needing it in some other cases, but that can happen in a followup.

I'll admit not having followed all the corner cases carefully in this method, so I'll have to review this more thoroughly in the next update. Part of the problem for me is that I haven't yet looked into bug 808370 where this is actually being used.

@@ +702,5 @@
> +      }
> +      //try {
> +        result = this._dbgWindow.evalInGlobalWithBindings(aString, bindings);
> +      /*}
> +      catch (ex) {

Is this part commented out because it isn't yet working, or is it remnants of a previous attempt at getting it to work? I'm confused by the two large comment blocks that seem contradictory.

Comment 28

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #27)
> Comment on attachment 706558 [details] [diff] [review]
> wip 1
> 
> Review of attachment 706558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't had a chance to play with this due to the bitrotted patch in bug
> 808370, but it seems to be in accordance with our discussed plan.

Thank you for the feedback!


> ::: toolkit/devtools/webconsole/WebConsoleClient.jsm
> @@ +98,5 @@
> > +   *        eval:
> > +   *          _self["prop"] = value;
> > +   *
> > +   *        - frameActor: a FrameActor ID. The FA holds a reference to
> > +   *        a Debugger.Frame. This option allow you to evaluate the string in
> 
> s/allow/allows/
> 
> ::: toolkit/devtools/webconsole/WebConsoleUtils.jsm
> @@ +31,5 @@
> > +// FIXME: toolkit imports from browser. Can we move vview to toolkit or...?
> > +// It looks like we already import toolbox jsm's from browser/...
> > +// Maybe we can recheck how strongly WCA depends on WCU. If it's not much, move
> > +// this back to browser/? I wouldn't favor this kind of back and forth, but it's
> > +// an option.
> 
> Good questions, but I'm afraid I don't have any good answers. I need to
> think more about this.

We should discuss this on IRC or in a meeting and decide on the way forward.


> @@ +1551,5 @@
> >    aOwner.sandbox.$x = function JSTH_$x(aXPath, aContext)
> >    {
> > +    // Work around bug 830818. If we return an object from chrome code the
> > +    // debuggee value will point to the proxy that wraps the chrome object.
> > +    let nodes = new aOwner.window.wrappedJSObject.Array();
> 
> This comment is not accurate, I don't think this has anything to do with bug
> 830818. This is a consequence of how the Debugger API is designed. Quoting
> the spec:
> 
> "JavaScript code in different compartments can have different views of the
> same object. For example, in Firefox, code in privileged compartments sees
> content DOM element objects without redefinitions or extensions made to that
> object's properties by content code. (In Firefox terminology, privileged
> code sees the element through an "xray wrapper".) To ensure that debugger
> code sees each object just as the debuggee would, each Debugger.Object
> instance presents its referent as it would be seen from a particular
> compartment. This "viewing compartment" is chosen to match the way the
> debugger came across the referent. As a consequence, a single Debugger
> instance may actually have several Debugger.Object instances: one for each
> compartment from which the referent is viewed."
> 
> In light of the above, I wouldn't call this a workaround, but as the proper
> way to implement these console helpers.

Oh, I missed that part of the spec. Good point, thank you!

> ::: toolkit/devtools/webconsole/dbg-webconsole-actors.js
> @@ +72,5 @@
> >  
> >    this._prefs = {};
> > +
> > +  // TODO: maybe we can reuse the Debugger() from the ThreadActor, if it's
> > +  // available. Thoughts? See complications in evalWithDebugger().
> 
> Frankly, this is what I would expect: use the ThreadActor's Debugger if the
> client is attached to the thread, and use a console-private Debugger
> otherwise.
> 
> I don't understand the problems it creates though. From my reading of
> evalWithDebugger it seems like it would help, no?

That is correct, but there's a problem here: what do you do when the web console is started first? You end up with two debugger instances and if you do evals, you get Debugger.Objects from two different Debuggers. That's when using vview and eval-in-frame with vview becomes funky.

JSTerm helpers need to become D.Os of the Debugger instance you are going to eval with. Same goes for the _self (the bound object that comes from vview). If you have a D.O from before the debugger was open, and if you try to update it in vview, you bump into the error: D.O comes from a different Debugger instance - frame.evalWithBindings() doesn't allow bindings from different debuggers (rightly so!).

That's why evalWithDebugger() was in a weird state when I submitted the wip patch.

> @@ +256,3 @@
> >      // We need to unwrap the object, otherwise we cannot access the properties
> >      // and methods added by the content scripts.
> > +    let actor = new ObjectActor(aObject, this);
> 
> Comment does not apply any longer.

Hah, good catch!


> @@ +646,3 @@
> >     *         The result of the evaluation.
> >     */
> > +  evalWithDebugger: function WCA_evalWithDebugger(aString, aOptions = {})
> 
> I thought this would be a ThreadActor method, perhaps in addition to being
> here, because I remember needing it in some other cases, but that can happen
> in a followup.
> 
> I'll admit not having followed all the corner cases carefully in this
> method, so I'll have to review this more thoroughly in the next update. Part
> of the problem for me is that I haven't yet looked into bug 808370 where
> this is actually being used.
> 
> @@ +702,5 @@
> > +      }
> > +      //try {
> > +        result = this._dbgWindow.evalInGlobalWithBindings(aString, bindings);
> > +      /*}
> > +      catch (ex) {
> 
> Is this part commented out because it isn't yet working, or is it remnants
> of a previous attempt at getting it to work? I'm confused by the two large
> comment blocks that seem contradictory.

This is something that didn't work properly and I had no way to fix it.

Luckily with bug 837723 I can now move JS objects from one debugger to another, so you can eval in the web console, and update objects, in any case: before the debugger is open, while it's paused, and after resume.

Problem solved. Now evalWithDebugger() is cleaner and simpler. Thanks to Jim and Honza who requested the API for slightly different reasons.

Will submit updated patch.

Comment 29

[Mihai Sucan [:msucan]]

Attachment: near-proposed patch

This is the "very close to done" patch, which I believe is ready for review.

Remaining issues:

- browser/ imports from toolkit/.
- some tests that check object class continue to fail (gracefully/no bad breakage/no timeouts). Object Proxy instead of whatever is expected.
- I need to add some tests for the new stuff.

For fixing the tests, in the light of the Debugger's spec: how about I call makeDebuggeeValue() on JS objects after I unwrap them? This might work for us. Please let me know if this is fine with you.

For the browser/ imports: it looks to me that they are pretty well contained, and the problem predates this patch. gDevTools and TargetFactory are used only in the $0 jsterm helper which breaks client-server boundaries, anyway. The new VariablesView is used in pprint() only. They are all lazy getters. Maybe we can leave these in with a warning at the top?

Looking forward to your review. Thanks!

(please note the new dependency: bug 837723 )

Comment 30

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch.

Comment 31

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

Comment on attachment 713393 [details] [diff] [review]
rebased patch

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

Works beautifully! My only qualm is the duplication of the objectGrip, etc. methods that I mention below. I'd really like us to avoid the extra memory overhead if possible.

(In reply to Mihai Sucan [:msucan] from comment #29)
> For fixing the tests, in the light of the Debugger's spec: how about I call
> makeDebuggeeValue() on JS objects after I unwrap them? This might work for
> us. Please let me know if this is fine with you.

This sounds good to me.

> For the browser/ imports: it looks to me that they are pretty well
> contained, and the problem predates this patch. gDevTools and TargetFactory
> are used only in the $0 jsterm helper which breaks client-server boundaries,
> anyway. The new VariablesView is used in pprint() only. They are all lazy
> getters. Maybe we can leave these in with a warning at the top?

Since this is not a new problem created by this patch, I say let's punt that for now.

::: toolkit/devtools/webconsole/dbg-webconsole-actors.js
@@ +265,4 @@
>     * @param object
>     *        The object grip.
>     */
> +  objectGrip: function WCA_objectGrip(aObject, aPool)

Why do you need to add your own version of objectGrip, longStringGrip and _createStringGrip and not rely on the implementations in ThreadActor? Does it have to do with you wanting to keep a separate actor pool? If so, why is that?

@@ +514,5 @@
> +      else if ("yield" in evalResult) {
> +        result = evalResult.yield;
> +      }
> +
> +      if ("throw" in evalResult) {

Nit: else if

@@ +656,5 @@
> +    // To allow the variables view to update properties from the web console we
> +    // provide the "bindObjectActor" mechanism: the Web Console tells the
> +    // ObjectActor ID for which it desires to evaluate an expression. The
> +    // Debugger.Object pointed at by the actor ID is bound such that it is
> +    // available during expression evaluation (evalInGlobalWithBindings()).

All of these very useful comments should be added to the method comment, since they pertain to the method as a whole, not to the _createGlobal call in particular.

@@ +672,5 @@
> +    // actor.
> +    //
> +    // The Debugger.Frame comes from the jsdebugger's Debugger instance, which
> +    // is different from the Web Console's Debugger instance. This means that
> +    // for evaluation to work, we need to reinstance the jsterm helpers - they

typo: reinstantiate

@@ +699,5 @@
> +    if (aOptions.frameActor) {
> +      let frameActor = this.conn.getActor(aOptions.frameActor);
> +      if (!frameActor) {
> +        Cu.reportError("Web Console Actor: the frame actor was not found: " +
> +                       aOptions.frameActor);

We should return here, otherwise we'll get an error in the next line.

::: toolkit/devtools/webconsole/test/test_bug819670_getter_throws.html
@@ +42,5 @@
>  
>    onInspect = onInspect.bind(null, aState);
> +  let client = new GripClient(aState.dbgClient, aResponse.result);
> +  // FIXME: this causes a firefox crash
> +  //client.getPrototypeAndProperties(onInspect);

Besides looking for a workaround, please file a bug, too.

Comment 32

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #31)
> Comment on attachment 713393 [details] [diff] [review]
> rebased patch
> 
> Review of attachment 713393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works beautifully! My only qualm is the duplication of the objectGrip, etc.
> methods that I mention below. I'd really like us to avoid the extra memory
> overhead if possible.

Thanks for your review!


> (In reply to Mihai Sucan [:msucan] from comment #29)
> > For fixing the tests, in the light of the Debugger's spec: how about I call
> > makeDebuggeeValue() on JS objects after I unwrap them? This might work for
> > us. Please let me know if this is fine with you.
> 
> This sounds good to me.

Will try. I hope it works.


> > For the browser/ imports: it looks to me that they are pretty well
> > contained, and the problem predates this patch. gDevTools and TargetFactory
> > are used only in the $0 jsterm helper which breaks client-server boundaries,
> > anyway. The new VariablesView is used in pprint() only. They are all lazy
> > getters. Maybe we can leave these in with a warning at the top?
> 
> Since this is not a new problem created by this patch, I say let's punt that
> for now.

Agreed.


> ::: toolkit/devtools/webconsole/dbg-webconsole-actors.js
> @@ +265,4 @@
> >     * @param object
> >     *        The object grip.
> >     */
> > +  objectGrip: function WCA_objectGrip(aObject, aPool)
> 
> Why do you need to add your own version of objectGrip, longStringGrip and
> _createStringGrip and not rely on the implementations in ThreadActor? Does
> it have to do with you wanting to keep a separate actor pool? If so, why is
> that?

|objectGrip|, |longStringGrip| and the other methods rely on the fact that the debugger is paused. They also rely on very specific lifetimes for objects, and they use specific actor pools, based on that. The WebConsoleActor cannot make such assumptions - I had to do my own methods.

Do you have ideas how we can improve this code? I didn't feel strongly - I was quite glad to see a lot of code simplification with the removal of the WebConsoleObjectActor.

> @@ +514,5 @@
> > +      else if ("yield" in evalResult) {
> > +        result = evalResult.yield;
> > +      }
> > +
> > +      if ("throw" in evalResult) {
> 
> Nit: else if

Good catch.


> @@ +656,5 @@
> > +    // To allow the variables view to update properties from the web console we
> > +    // provide the "bindObjectActor" mechanism: the Web Console tells the
> > +    // ObjectActor ID for which it desires to evaluate an expression. The
> > +    // Debugger.Object pointed at by the actor ID is bound such that it is
> > +    // available during expression evaluation (evalInGlobalWithBindings()).
> 
> All of these very useful comments should be added to the method comment,
> since they pertain to the method as a whole, not to the _createGlobal call
> in particular.

Good point.

> @@ +672,5 @@
> > +    // actor.
> > +    //
> > +    // The Debugger.Frame comes from the jsdebugger's Debugger instance, which
> > +    // is different from the Web Console's Debugger instance. This means that
> > +    // for evaluation to work, we need to reinstance the jsterm helpers - they
> 
> typo: reinstantiate

Good catch. I found more issues so I made more changes in these comments.


> @@ +699,5 @@
> > +    if (aOptions.frameActor) {
> > +      let frameActor = this.conn.getActor(aOptions.frameActor);
> > +      if (!frameActor) {
> > +        Cu.reportError("Web Console Actor: the frame actor was not found: " +
> > +                       aOptions.frameActor);
> 
> We should return here, otherwise we'll get an error in the next line.

Ah, good catch. This code morphed a lot through its lifetime. My intent is to allow eval to happen in global if frameActor is not found. Fixed.


> ::: toolkit/devtools/webconsole/test/test_bug819670_getter_throws.html
> @@ +42,5 @@
> >  
> >    onInspect = onInspect.bind(null, aState);
> > +  let client = new GripClient(aState.dbgClient, aResponse.result);
> > +  // FIXME: this causes a firefox crash
> > +  //client.getPrototypeAndProperties(onInspect);
> 
> Besides looking for a workaround, please file a bug, too.

Will do, thank you!

Comment 33

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

> > ::: toolkit/devtools/webconsole/dbg-webconsole-actors.js
> > @@ +265,4 @@
> > >     * @param object
> > >     *        The object grip.
> > >     */
> > > +  objectGrip: function WCA_objectGrip(aObject, aPool)
> > 
> > Why do you need to add your own version of objectGrip, longStringGrip and
> > _createStringGrip and not rely on the implementations in ThreadActor? Does
> > it have to do with you wanting to keep a separate actor pool? If so, why is
> > that?
> 
> |objectGrip|, |longStringGrip| and the other methods rely on the fact that
> the debugger is paused. They also rely on very specific lifetimes for
> objects, and they use specific actor pools, based on that. The
> WebConsoleActor cannot make such assumptions - I had to do my own methods.

I am of the opinion that we should eventually refactor these so that they don't rely on those things and instead the thread actor handles lifetime and debugger state for them. I will defer to others the decision of whether this is the right patch to implement these changes.

Comment 34

[Mihai Sucan [:msucan]]

Attachment: address review comments

Updated the patch to address review comments from Panos. I also opened follow-up bugs for TODO items.

Two blockers:

- the XUL document.__proto__ inspection crasher - see bug 842726. I couldn't work around it. What should we do here? Disable the test or is this something Jim can fix soon?

- class: Proxy. I tried to do obj.wrappedJSObject for addDebuggee() and for makeDebuggeeValue(). No dice. Jim, what can we do here to avoid getting the wrapped object when we inspect an object?

Here's a patch I did to experiment with unwrapping the objects before I call makeDebuggeeValue():

https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/bug-783499-experiment

Thank you!

Comment 35

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #33)
> > |objectGrip|, |longStringGrip| and the other methods rely on the fact that
> > the debugger is paused. They also rely on very specific lifetimes for
> > objects, and they use specific actor pools, based on that. The
> > WebConsoleActor cannot make such assumptions - I had to do my own methods.
> 
> I am of the opinion that we should eventually refactor these so that they
> don't rely on those things and instead the thread actor handles lifetime and
> debugger state for them. I will defer to others the decision of whether this
> is the right patch to implement these changes.

To avoid covering too much scope within this bug I would suggest we open a follow-up bug. If you and Panos agree, I can file it.

Comment 36

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

(In reply to Mihai Sucan [:msucan] from comment #35)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #33)
> > > |objectGrip|, |longStringGrip| and the other methods rely on the fact that
> > > the debugger is paused. They also rely on very specific lifetimes for
> > > objects, and they use specific actor pools, based on that. The
> > > WebConsoleActor cannot make such assumptions - I had to do my own methods.
> > 
> > I am of the opinion that we should eventually refactor these so that they
> > don't rely on those things and instead the thread actor handles lifetime and
> > debugger state for them. I will defer to others the decision of whether this
> > is the right patch to implement these changes.
> 
> To avoid covering too much scope within this bug I would suggest we open a
> follow-up bug. If you and Panos agree, I can file it.

I don't really care whether we fix this here or in a followup. What I am wondering about is how come thread-scope is not sufficient for the web console, and if it's not sufficient, what is the right scope (tab-scope?) that we should implement in the debugger, so that other consumers can use it as well.

Comment 37

[Mihai Sucan [:msucan]]

Nick, can you please file a follow-up bug report for the refactoring you suggested here? Thanks!

Comment 38

[Mihai Sucan [:msucan]]

Boris: I have a problem with the patch I attached here. While it works with page content debugging - all objects can be correctly inspected, I have problems with chrome content inspection, in the toolkit webconsole tests. All objects I inspect from tests show up as Proxy. Why is that? I tried to unwrap the objects before makeDebuggeeValue() but no luck - see comment 34.

Now I've been looking into why we might still get wrappers and I found one additional possible cause: mochitest-chrome tests are loaded in an iframe. The top level window is the tests driver. The web console attaches to the top level window. Is the test's own iframe in a different compartment? How can I get the tests to work as expected?

The failing tests are:

toolkit/devtools/webconsole/test/test_object_actor.html
toolkit/devtools/webconsole/test/test_consoleapi.html

Thank you!

Comment 39

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

Long shot, but could you try the patch in bug 831367 to see if it makes any difference?

Comment 40

[Boris Zbarsky [:bzbarsky]]

> All objects I inspect from tests show up as Proxy. Why is that?

Are you touching objects from a different compartment?  .wrappedJSObject will give you a different kind of cross-compartment wrapper, but still a wrapper.

> Is the test's own iframe in a different compartment?

Every single DOM Window is in a different compartment ever since compartment-per-global landed.  So yes, if the web console is in the compartment of the top level window bu the tests are in an iframe then you'll have a cross-compartment access.

The simplest approach to dealing with cross-compartment wrappers seems to me to just unwrap them in the C++ methods where you want to work with underlying objects, right?

Comment 41

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #39)
> Long shot, but could you try the patch in bug 831367 to see if it makes any
> difference?

Thanks, tried it and it made no difference.


(In reply to Boris Zbarsky (:bz) from comment #40)
> > All objects I inspect from tests show up as Proxy. Why is that?
> 
> Are you touching objects from a different compartment?  .wrappedJSObject
> will give you a different kind of cross-compartment wrapper, but still a
> wrapper.

Good point. I learned this through experimentation.


> > Is the test's own iframe in a different compartment?
> 
> Every single DOM Window is in a different compartment ever since
> compartment-per-global landed.  So yes, if the web console is in the
> compartment of the top level window bu the tests are in an iframe then
> you'll have a cross-compartment access.
> 
> The simplest approach to dealing with cross-compartment wrappers seems to me
> to just unwrap them in the C++ methods where you want to work with
> underlying objects, right?

Sounds good, but here we work with the objects in JSM's.

Anyhow, I found the way to solve test failures. Having learned about ObjectWrapper.jsm I was able to get all the tests passing.

Thank you both for the suggestions.

Comment 42

[Mihai Sucan [:msucan]]

Attachment: patch 5: test fixes

All tests pass.

Comment 43

[Mihai Sucan [:msucan]]

Attachment: patch 6: minor fixes

Minor fixes, based on more testing.

Asking for a quick re-review so you don't miss any of the changes that happened since your last review. Thanks!

Comment 44

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

Comment on attachment 721423 [details] [diff] [review]
patch 6: minor fixes

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

Looks good.

For future reference, I would appreciate it if you briefly described the changes or the changed files, so that I can avoid going over everything again. It would save me a lot of time and allow me to get to the review sooner.

Comment 45

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #44)
> Comment on attachment 721423 [details] [diff] [review]
> patch 6: minor fixes
> 
> Review of attachment 721423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> For future reference, I would appreciate it if you briefly described the
> changes or the changed files, so that I can avoid going over everything
> again. It would save me a lot of time and allow me to get to the review
> sooner.

Sorry, I was in a hurry and I forgot to write down the changes:


- removed locationChange from dbg-client.jsm.
- also did:
-        Cu.reportError(ex.message + "\n" + ex.stack);
+        Cu.reportError(ex + "\n" + ex.stack);

in dbg-client.jsm, found that exceptions didn't include full message.

- a fix in onEvaluateJS() from WebConsoleActor, related to displaying eval exceptions.

- fixes for comments.


Thanks for the review!

Comment 46

[Mihai Sucan [:msucan]]

Attachment: patch 7: rebase

Rebased the patch. This is ready to land - all tests pass, no oranges on try. https://tbpl.mozilla.org/?tree=Try&rev=f05d8fb72edb

Comment 47

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/f5d6c95a9de9

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5d6c95a9de9

Comment 50

[Alexandre Poirot [:ochameau]]

Attachment: b2g18 - Web Console should use the debugger API

Comment 51

[Alexandre Poirot [:ochameau]]

Comment on attachment 771331 [details] [diff] [review]
b2g18 - Web Console should use the debugger API

Patch rebased against b2g18.