Closed Bug 1287889 Opened 4 years ago Closed 4 years ago

WebConsole should adopt the _lastConsoleInputEvaluation when the debugger changes

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: rpl, Assigned: rpl)

Details

Attachments

(1 file)

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 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.
Attachment #8772519 - Flags: review?(jryans) → feedback?(jryans)
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.
Attachment #8772519 - Flags: feedback?(jryans) → feedback?(bgrinstead)
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
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Attachment #8772519 - Flags: feedback?(bgrinstead) → feedback+
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 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/
Attachment #8772519 - Flags: feedback+ → review?(jryans)
Attachment #8772519 - Flags: review?(jryans)
Attachment #8772519 - Flags: review?(bgrinstead)
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 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/
(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 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.
Attachment #8772519 - Flags: review?(bgrinstead) → review+
Iteration: --- → 50.4 - Aug 1
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/37974a5b6927
Adopt webconsole _lastConsoleInputEvaluation when the debugger changes. r=bgrins
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/37974a5b6927
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.