New debugger can break console

RESOLVED FIXED in Firefox 51

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jlong, Assigned: jlast)

Tracking

unspecified
Firefox 51
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

STR:

1. Enable the new debugger
2. Make sure it's the default tool selected (open the toolbox and select it and close it again)
3. Open the devtools (debugger should be selected)
4. Click on the console, and try to evaluate something

The console won't evaluate it. I don't know why, this is super weird, but it some sort of interation with the new debugger.
Blocks: 1294139
Posted patch console.patch (obsolete) — Splinter Review
This patch updates the way the console queries the Debugger's frames.

It used to use the DebuggerController's stack frames component directly. Now it goes through a panel getFrames function, which is available on both the new and the old debugger.

It's also worth noting that I updated the way the new debugger is bootstrapped.
The debugger used to expose several functions to the panel. Now there's a bootstrap function that returns useful getState and selectors. Together, these properties can be used to query the debugger's state. I was tempted to move these properties to a weakmap to encourage encapsulation, but instead left them as underscore prefixed to indicate that their internal.

I also added a `switchDeubgger` call inside of definitions so that the correct debugger is started when the toolbox is initialized.

This change requires a new bundle for the console to work with the new debugger, but shouldn't break the existing debugger.
Attachment #8784474 - Flags: review?(jlong)
here's a new try run that does not pull in the new mochitest
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2108efc5c8d
Comment on attachment 8784474 [details] [diff] [review]
console.patch

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

Sorry this took a while to review; this looks great! I suggested a tweak to how we reference the debugger data though. Let me know if there are any problems with that.

::: devtools/client/debugger/new/panel.js
@@ +23,5 @@
> +        tabTarget: this.toolbox.target
> +      });
> +
> +      this._getState = getState;
> +      this._selectors = selectors;

There is potential for leakage here, which is why you mentioned `WeakMap`. I think it might be better to leave these inside the page, and access them like `this.panelWin.Debugger.getState()`. That way we don't have to worry about it at all.

I don't mind converting the bootstrapping code into a function though; you can still do that but also expose the things you need (also actions for tests) as properties on `Debugger`.

::: devtools/client/definitions.js
@@ +196,5 @@
>      };
>    }
>  }
>  
> +switchDebugger();

This part will probably conflict with bug 1294686, depends on who lands it first. I feel like this patch was based on an older patch which had already switched this to use `addObserver`, as the current code in m-c does not do this. It's probably best if you removed this and let bug 1294686 implement the whole pref observer.
Posted patch console2.patch (obsolete) — Splinter Review
Attachment #8784474 - Attachment is obsolete: true
Attachment #8784474 - Flags: review?(jlong)
Assignee: nobody → jlaster
Comment on attachment 8787219 [details] [diff] [review]
console2.patch

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

Generally looks great, still have one concern. I'll r+ this though, if you make that one tweak. We should be able to eventually remove this normalization when we remove the old debugger, so it's just temporary.

::: devtools/client/debugger/new/panel.js
@@ +44,5 @@
> +
> +  getFrames: function() {
> +    let frames = this._selectors.getFrames(this._getState()).toJS();
> +    const selectedFrame = this._selectors.getSelectedFrame(this._getState());
> +    const selected = frames.findIndex(frmae => frmae.id == selectedFrame.id);

typo: frmae => frame

@@ +47,5 @@
> +    const selectedFrame = this._selectors.getSelectedFrame(this._getState());
> +    const selected = frames.findIndex(frmae => frmae.id == selectedFrame.id);
> +
> +    frames.forEach(frame => {
> +      frame.actor = frame.id;

Please map the objects, as I think you are mutating the actual redux app state here (we don't force them to be immutable... maybe we should freeze our JS objects).

Is this the only normalizing that needs to happen? A bit surprising, but great if so.
Attachment #8787219 - Flags: review+
Also, does this fix this problem?

1. Open devtools, make sure debugger is default tool highlighted. If not, click on debugger and re-open devtools.
2. Go to console and try to evaluate something
3. Code is not evaluated

This happens when you are not paused, so I'm suspicious that this doesn't fix the above problem. There might be another place where the console is using the old panel state. Definitely need to make sure the above works before we launch.
Comment on attachment 8787219 [details] [diff] [review]
console2.patch

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

::: devtools/client/debugger/new/panel.js
@@ +47,5 @@
> +    const selectedFrame = this._selectors.getSelectedFrame(this._getState());
> +    const selected = frames.findIndex(frmae => frmae.id == selectedFrame.id);
> +
> +    frames.forEach(frame => {
> +      frame.actor = frame.id;

Oh, nevermind. I see that you do have to call `toJS` up there. So this is fine.
Posted patch console3.patch (obsolete) — Splinter Review
Attachment #8787219 - Attachment is obsolete: true
Keywords: checkin-needed
We need to make sure to commit an updated Firefox debugger immediately after this, as it will break the current debugger. I can do that.
Thanks jlongster.
Attachment #8787257 - Attachment is obsolete: true
Flags: needinfo?(jlaster)
sorry Wes, I did not include the right patch.
Jason, that try run looks like it includes one of the new mochitests, and shows the leak. I'm going to post a new try run just with the attached patch to see if I can land this later tonight along with an updated debugger. Would like to have it ready to be tested next week by others.
This try run should be the latest attached patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=300b7e682bd5
Status: NEW → ASSIGNED
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/9748e79666b9
Fix console interaction with new debugger r=jlongster
https://hg.mozilla.org/mozilla-central/rev/9748e79666b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I have reproduced this bug with Firefox nightly 51.0a1 (build id:20160816030459)on
windows 7(64 bit)

I have verified as fixed this bug with Firefox beta 51.0b11(build id:20170103031119)
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[bugday-20170104]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.