Closed Bug 1004562 Opened 5 years ago Closed 5 years ago

Web console actors need to update their use of Debugger.Environment.prototype.getVariable

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: jimb, Assigned: past)

References

Details

Attachments

(2 files, 2 obsolete files)

The web console's actor implementations in the server have not been updated to cope with the { optimizedOut: true } sentinel objects now returned by Debugger.Environment.prototype.getVariable when asked for the value of a variable that the JavaScript engine decided not to store.

The only actual use is here:

https://hg.mozilla.org/mozilla-central/file/43cd629879c2/toolkit/devtools/webconsole/utils.js#l1092

As an intermediate step, that actor could simply not mention optimized-out variables, treating them like other 'invalid identifiers'. But in the long run, the UI should display unavailable variables in some helpful way, and this code should provide the data to drive that display.
Duplicate of this bug: 1006922
There is an additional vector for optimized-out values to reach the console, WCA_evalWithDebugger. From bug 1006922 comment 0:

1. Go to http://todomvc.com/architecture-examples/backbone/
2. Add a breakpoint on line 221 of backbone.js
(the line should read " var listeningTo = ...")
3. Reload the page
4. Wait for the breakpoint to hit
5. Switch to the web console
6. Type 'method' (without quotes)
7. Press enter.

Sadness: https://pastebin.mozilla.org/5086006
This fixes all that I could see as broken in the web console actor's handling of optimized-out values. I couldn't verify inspection of such values works, as that triggers an assertion which I have filed a separate bug for. Conditional breakpoints with these values seem to work OK, and watch expressions break in the debugger frontend, which is bug 941287. Hiding an optimized-out value (by creating one with the same name in the local scope) works. Assigning to an optimized-out variable behaves rather unexpectedly (as if the assignment actually succeeded), but it doesn't break the tools.

It seems to me that we should just fix bug 941287 right away instead of keep piling up such band-aids. It may be useful to land these fixes in the meantime, but it's unclear how many people are hitting these cases, compared to bug 1002456 which plagued a large number of people.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Shu is going to fix this in bug 1007164.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1007164
I'm going to add an integration test in this bug to make sure we don't regress the web console in the future.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
This is similar to the STR in comment 2 and the debugger test from bug 1002456.
Attachment #8420096 - Flags: review?(vporof)
Turns out that the error string changed before landing.
New try: https://tbpl.mozilla.org/?tree=Try&rev=ab586766f100
Attachment #8420124 - Flags: review?(vporof)
Attachment #8420096 - Attachment is obsolete: true
Attachment #8420096 - Flags: review?(vporof)
Comment on attachment 8420124 [details] [diff] [review]
Add a console test for inspection of optimized out variables v2

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

::: browser/devtools/webconsole/test/browser_console_optimized_out_vars.js
@@ +12,5 @@
> +    let { toolbox, panel, panelWin } = yield openDebugger();
> +
> +    yield (() => {
> +      let deferred = promise.defer();
> +      panelWin.gThreadClient.addOneTimeListener("resumed", (aEvent, aPacket) => {

Could use waitForThreadEvents here right, and avoid the extra yield block.

yield waitForThreadEvents(panel, "resumed");
ok(true, "Debugger resumed");

@@ +14,5 @@
> +    yield (() => {
> +      let deferred = promise.defer();
> +      panelWin.gThreadClient.addOneTimeListener("resumed", (aEvent, aPacket) => {
> +        ok(true, "Debugger resumed");
> +        deferred.resolve({ toolbox: toolbox, panelWin: panelWin });

Any real reason we're resolving this promise with the toolbox and the panel window? It doesn't seem like they're yielded into anything.

@@ +23,5 @@
> +    let sources = panelWin.DebuggerView.Sources;
> +    yield panel.addBreakpoint({ url: sources.values[0], line: 18 });
> +    yield ensureThreadClientState(panel, "resumed");
> +
> +    yield (() => {

Why the yield block? Because of the panelWin.once callback I suppose? You can get around it and yield everything normally. See below.

@@ +26,5 @@
> +
> +    yield (() => {
> +      let deferred = promise.defer();
> +
> +      panelWin.once(panelWin.EVENTS.FETCHED_SCOPES, (aEvent, aPacket) => {

I think `once` returns a promise now, so you can yield this to avoid extra indentation.

let fetchedScopes = panelWin.once(panelWin.EVENTS.FETCHED_SCOPES);
let button = ...
ok(button, "Button element found");
button.click();

let packet = yield fetchedScopes;
ok(true, "Scopes were fetched");

yield toolbox.selectTool("webconsole");

hud.jsterm.execute("upvar");
yield waitForMessages(...);

finishTest();
Attachment #8420124 - Flags: review?(vporof) → review+
Attached patch v3Splinter Review
Thanks for the suggestions and cleanups. In addition to your comments I had to use executeSoon for the click event in order for the test to reach the subsequent yield.

New try: https://tbpl.mozilla.org/?tree=Try&rev=a8d149e1f91e
Attachment #8420124 - Attachment is obsolete: true
Attachment #8421645 - Flags: review+
Try was green. Landed with a couple of file mode fixes caused by bug 1006027:

https://hg.mozilla.org/integration/fx-team/rev/db667e867a25
Status: REOPENED → ASSIGNED
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/db667e867a25
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.