1295753 - New debugger can break console

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

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.

Comment 1

[Jason Laster [:jlast]]

Attachment: console.patch

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.

Comment 2

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7805a0438661

Comment 3

[Jason Laster [:jlast]]

here's a new try run that does not pull in the new mochitest
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2108efc5c8d

Comment 4

[James Long (:jlongster)]

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.

Comment 5

[Jason Laster [:jlast]]

Attachment: console2.patch

Comment 6

[James Long (:jlongster)]

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.

Comment 7

[James Long (:jlongster)]

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 8

[James Long (:jlongster)]

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.

Comment 9

[Jason Laster [:jlast]]

Attachment: console3.patch

Comment 10

[James Long (:jlongster)]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/37f554c467ca
Fix console. r=jlongster

Comment 12

[Jason Laster [:jlast]]

Thanks jlongster.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11396647&repo=fx-team

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

Comment 14

[Jason Laster [:jlast]]

Attachment: console4.patch

Comment 15

[Jason Laster [:jlast]]

sorry Wes, I did not include the right patch.

Comment 16

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d6681ffb9d6

Comment 17

[James Long (:jlongster)]

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.

Comment 18

[James Long (:jlongster)]

This try run should be the latest attached patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=300b7e682bd5

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9748e79666b9
Fix console interaction with new debugger r=jlongster

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9748e79666b9

Comment 21

[Kazi Nuzhat Tasnem]

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]