1367052 - Debugger Server prematurely removes out of scope variables

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jason Laster [:jlast]]

STR: 
1. go to https://devtools-html.github.io/debugger-examples/examples/interval.html
2. add a breakpoint on line 13: `arr.push`
3. resume
4. expand the second scope and arr object

Expected: see the arr properties
Actual: it fails because the arr actor can not be found

Julian and I tracked the bug down to this commit (https://github.com/mozilla/gecko-dev/commit/9f7a43f362961cca68dc6e05ec90e673f612da59#diff-58d9dfd9ca670d23e714c743a045d90eR1728), which was landed on June 1st 2015.

I believe it's related to how object actors are retained in the pool and `aPool.objectActors` weak map. When the debugger resumes it releases the arr actor from the pool, but when it pauses the grip value is still in the weak map so it is not re-added to the pool.

Comment 1

[Jason Laster [:jlast]]

Here's a test case that isolates the bug: https://gist.github.com/jasonLaster/6c8f1d13383a57bd8d106bdb8546d226

Comment 2

[Jason Laster [:jlast]]

Attachment: scope-bug.patch

Here's a test the demonstrates the bug. It is pretty ugly and should not be checked in though

Comment 3

[Jason Laster [:jlast]]

Attachment: scope-bug2.patch

Another test that fails in an async way

Comment 4

[Jason Laster [:jlast]]

Attachment: scope-bug-2.patch

The issue we were seeing was that the debugger could not get "out of scope" variables after the first pause.

The issue is that Debugger.environment objects are assigned an actor when the Environment Actor is created in createEnvironmentActor: 

http://searchfox.org/mozilla-central/source/devtools/server/actors/script.js#1663

Unfortunately, we treat the environment actors very poorly... We add them to the frame lifetime pool, which is cleared when the debugger is resumed. When the debugger resumes, the environment actors are destroyed and the corresponding frame lifetime pool is taken out of the connection pool list.

It would be nice to address this underlying problem in a larger architectural change. 
I think we can fix this by having environment actors in their own weak map so that we can access them given an environment. Note, this is similar to how we handle Objects.

This patch is the simpler quick fix. We null out the Debugger.Environment actor field when the actor is removed so that the next time we pause the Environment creates a new actor. This means that we do not persist the same actor, but I think it is better than persisting an orphaned actor that points to the old frame lifetime pool.




 triggering the environment actors to be destroyed. 



This keeps the environment actor from

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jason Laster [:jlast] from comment #4)
> Created attachment 8871307 [details] [diff] [review]
> scope-bug-2.patch
> 
> The issue we were seeing was that the debugger could not get "out of scope"
> variables after the first pause.
> 
> The issue is that Debugger.environment objects are assigned an actor when
> the Environment Actor is created in createEnvironmentActor: 
> 
> http://searchfox.org/mozilla-central/source/devtools/server/actors/script.
> js#1663
> 
> Unfortunately, we treat the environment actors very poorly... We add them to
> the frame lifetime pool, which is cleared when the debugger is resumed. When
> the debugger resumes, the environment actors are destroyed and the
> corresponding frame lifetime pool is taken out of the connection pool list.
> 
> It would be nice to address this underlying problem in a larger
> architectural change. 
> I think we can fix this by having environment actors in their own weak map
> so that we can access them given an environment. Note, this is similar to
> how we handle Objects.

This wouldn't give us any place to destroy + clean up the environment actors (such as unregister them with the connection) after the environment is GC'd. We would leak them for the duration of the devtools session.

The way to work around that would be adding custom finalizer hooks for Debugger.Environment (yuck!)

I think the approach you've come up with is a better solution.

> This patch is the simpler quick fix. We null out the Debugger.Environment
> actor field when the actor is removed so that the next time we pause the
> Environment creates a new actor.

This isn't necessarily the next time we pause right? Its the next time we pause after the frame we were in has been popped, its pool emptied, and actors within said pool destroyed. In other words, if we pause again in the same frame, then the actor is not destroyed, and we reuse it, right?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8871307 [details] [diff] [review]
scope-bug-2.patch

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

r=me with a comment on the importance of that null write.

Please request uplift, since this is a pretty severe bug.

::: devtools/server/actors/environment.js
@@ +27,5 @@
>      this.threadActor = threadActor;
>    },
>  
> +  destroy: function() {
> +    this.obj.actor = null;

This could certainly use a comment after the days of debugging you've done.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

And also: thanks for digging into this!

Comment 8

[Jason Laster [:jlast]]

> if we pause again in the same frame, then the actor is not destroyed, and we reuse it, right?

Yes, I believe so.

Comment 9

[Jason Laster [:jlast]]

Attachment: scope-bug-3.patch

Comment 10

[Jason Laster [:jlast]]

Attachment: scope-bug-4.patch

Comment 11

[Jason Laster [:jlast]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bae45a890ff61a8b5c21b0924c962c56c935e850&selectedJob=102051029

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c93b74884b
Debugger Server prematurely removes out of scope variables. r=fitzgen

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5c93b74884b