Closed Bug 1367052 Opened 5 years ago Closed 5 years ago

Debugger Server prematurely removes out of scope variables

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(1 file, 4 obsolete files)

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.
Here's a test case that isolates the bug: https://gist.github.com/jasonLaster/6c8f1d13383a57bd8d106bdb8546d226
Attached patch scope-bug.patch (obsolete) — Splinter Review
Here's a test the demonstrates the bug. It is pretty ugly and should not be checked in though
Attached patch scope-bug2.patch (obsolete) — Splinter Review
Another test that fails in an async way
Attached patch scope-bug-2.patch (obsolete) — Splinter Review
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
Attachment #8870933 - Attachment is obsolete: true
Attachment #8871090 - Attachment is obsolete: true
Attachment #8871307 - Flags: review?(nfitzgerald)
(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 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.
Attachment #8871307 - Flags: review?(nfitzgerald) → review+
And also: thanks for digging into this!
> if we pause again in the same frame, then the actor is not destroyed, and we reuse it, right?

Yes, I believe so.
Attached patch scope-bug-3.patch (obsolete) — Splinter Review
Attachment #8871307 - Attachment is obsolete: true
Attachment #8871357 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → jlaster
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c93b74884b
Debugger Server prematurely removes out of scope variables. r=fitzgen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5c93b74884b
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.