1287889 - WebConsole should adopt the _lastConsoleInputEvaluation when the debugger changes

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

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

If in a webconsole the debugger used to evaluate javascript code in the current target context is changed (e.g. switching from the current paused frame to the initial target window context, once the threadClient has been resumed) between two console evaluation, then  an "Debugger.Object belongs to a different Debugger" exception is raised, caused from the last console input evaluation result (that is cached in the webconsole instance as this._lastConsoleInputEvaluation), even if the execute expression doesn't use the "$_" console helper directly.

The goal of this issue is to fix the above issue by using the new adoptDebuggeeValue helper introduced in the Debugger API from Bug 1252958, to adopt the Debugger.Object when the above scenario happens, prevent the exception from being raised and get the "$_" console helper to work as expected.

Comment 1

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

Attachment: Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

Review commit: https://reviewboard.mozilla.org/r/65312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65312/

Comment 2

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

Comment on attachment 8772519 [details]
Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

Attached a patch (and a related test, actually it is an existent test related to the debugger and the split console extended to test the splitConsole and the issue described above) for an initial feedback on the proposed approach.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8772519 [details]
Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

:bgrins has more context on the console, so I think he'd be a better choice here.

Comment 4

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/65312/#review63224

Thanks for tracking this down

::: devtools/client/debugger/test/mochitest/browser_dbg_split-console-paused-reload.js:40
(Diff revision 1)
>      frames.selectedItem.target,
>      dbgWin);
>    info("The breadcrumb received focus.");
>  
>    // This is the meat of the test.
> -  let result = toolbox.once("webconsole-ready", () => {
> +  let waitForSplitConsole = toolbox.once("webconsole-ready", () => {

There's a getSplitConsole helper function available here that takes care of all this, including the pref cleanup which could then be removed: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/head.js#1228

::: devtools/server/actors/webconsole.js:1297
(Diff revision 1)
>      let evalOptions;
>      if (typeof aOptions.url == "string") {
>        evalOptions = { url: aOptions.url };
>      }
>  
> +    // If the debugger object is changed from the last evaluation,

No need to keep track of _lastConsoleInputEvaluationDebugger, this should do the same:

    if (this._lastConsoleInputEvaluation &&
        this._lastConsoleInputEvaluation.global !== dbgWindow) {
        this._lastConsoleInputEvaluation = dbg.adoptDebuggeeValue(
          this._lastConsoleInputEvaluation
        );
    }

::: devtools/server/actors/webconsole.js:1398
(Diff revision 1)
> +    // Store the debugger that has generated the result, because we need to adopt the
> +    // _lastConsoleInputEvaluation value if the Debugger is changed, or we are going
> +    // to raise "Debugger.Object belongs to a different Debugger" exceptions on the
> +    // first evalWithDebugger call that use a different debugger (e.g. switching from
> +    // a paused thread frame and the main debugging window).
> +    this._lastConsoleInputEvaluationDebugger = dbg;

See above, this can be removed

Comment 5

[Brian Grinstead [:bgrins]]

I notice in the docs that it *can* throw: https://hg.mozilla.org/integration/mozilla-inbound/rev/6001a9e0ec92#l1.13.  Although I believe in this case _lastConsoleInputEvaluation will always be a Debugger.Object

Comment 6

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

Comment on attachment 8772519 [details]
Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65312/diff/1-2/

Comment 7

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

https://reviewboard.mozilla.org/r/65312/#review63224

Thanks for the great suggestions, the fix and its test are even better now!

> There's a getSplitConsole helper function available here that takes care of all this, including the pref cleanup which could then be removed: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/head.js#1228

great! last updated patch use the `getSplitConsole` test helper to open the splitConsole and get the jsterm object.

> No need to keep track of _lastConsoleInputEvaluationDebugger, this should do the same:
> 
>     if (this._lastConsoleInputEvaluation &&
>         this._lastConsoleInputEvaluation.global !== dbgWindow) {
>         this._lastConsoleInputEvaluation = dbg.adoptDebuggeeValue(
>           this._lastConsoleInputEvaluation
>         );
>     }

This is way better! the above suggestion has been applied to the updated patch.

> See above, this can be removed

removed.

Comment 8

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

Comment on attachment 8772519 [details]
Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65312/diff/2-3/

Comment 9

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

(In reply to Luca Greco [:rpl] from comment #8)
> Comment on attachment 8772519 [details]
> Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger
> changes.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/65312/diff/2-3/

Patch rebased on a recent fx-team tip.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8772519 [details]
Bug 1287889 - Adopt webconsole _lastConsoleInputEvaluation when the debugger changes.

https://reviewboard.mozilla.org/r/65312/#review64136

Thanks!

::: devtools/server/actors/webconsole.js:1305
(Diff revision 3)
> +    // adopt this._lastConsoleInputEvaluation value in the new debugger,
> +    // to prevents "Debugger.Object belongs to a different Debugger" exceptions
> +    // related to the $_ bindings.
> +    if (this._lastConsoleInputEvaluation &&
> +        this._lastConsoleInputEvaluation.global !== dbgWindow) {
> +      this._lastConsoleInputEvaluation = dbg.adoptDebuggeeValue(

As far as I can tell, this will never throw in our case since _lastConsoleInputEvaluation is assigned from a completion value's return or yield property which is a Debugger.Object, and adoptDebuggeeValue doesn't throw when receiving a Debugger.Object.  So this should be OK as is with no need to try/catch.

Comment 11

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

Push to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f57eae90435

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/37974a5b6927
Adopt webconsole _lastConsoleInputEvaluation when the debugger changes. r=bgrins

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37974a5b6927