991688 - Breakpoints not properly restored when thread is detached/reattached

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Breakpoints are not properly restored after the client is detached from the backend thread and attached again.

These breakpoints are still in BreakpointStore, but not set to the current Debugger.Script objects.

STR:
1. Open Firebug on this page: https://getfirebug.com/tests/head/script/breakpoints/4889/issue4889.html
2. Create a breakpoint at line 10
3. Disable the Script panel and enable it again (no reload)
4. Click on "Call first non-existing function" button on the page
5. Breakpoint doesn't hit -> BUG

Firebug can be downloaded here:
https://getfirebug.com/releases/firebug/2.0/firebug-2.0a1.xpi

Originally reported here:
http://code.google.com/p/fbug/issues/detail?id=7295

---

My wild guess is that the list of scripts in BreakpointActor is not cleared on detach and when ThreadActor._restoreBreakpoints() is executed on attach, the breakpoint is not actually set due to the following condition in ThreadActor._addScript()

      if (!bp.actor.scripts.length
          && bp.line >= aScript.startLine
          && bp.line <= endLine) {
        this._setBreakpoint(bp);
      }

The bp.actor.scripts.length > 0 (there are old script objects?), which makes the condition false and the breakpoint is not set.

Note that the breakpoint is properly set if the page is reloaded.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Does my analyses make sense or the problem is rather somewhere else?

Honza

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Your analysis seems correct. Calling disableAllBreakpoints() when detaching should fix the issue, but I'm wondering if a better fix would be to add a disconnect() method to BreakpointActor that does it, which will be called automatically when the actor pool is removed.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Fix a bug in restoreBreakpoints + regression test

I can no longer reproduce this issue.

The check whether bp.actor.scripts.length is 0 used to be there to determine whether the breakpoint actor was pending or not. The code for dealing with pending breakpoints has been completely overhauled, which should have resolved this issue.

While investigating this, I found a bug in restoreBreakpoints: it properly readds each BreakpointActor in the BreakpointActorMap as a breakpoint handler for each Debugger.Script in the ScriptStore, but it does not re-add each BreakpointActor to the threadLifeTimePool.

That means that if we detach from the thread actor a second time, the disconnect method for those BreakpointActors won't be called, so that they will never be removed as breakpoint handler, and we end up leaking them.

I wrote a patch to address this problem, as well as a regression test for the issue at hand.

Comment 4

[Eddy Bruel [:ejpbruel]]

Try push for the above patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b843559ac4

Comment 5

[James Long (:jlongster)]

Comment on attachment 8617405 [details] [diff] [review]
Fix a bug in restoreBreakpoints + regression test

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

I still don't fully understand lifetimes, but this looks reasonable. Main thing I would suggest is to use helper functions to init the test, I don't like duplicating all that code everywhere. It will make it harder to change if we ever need to.

::: toolkit/devtools/server/actors/script.js
@@ +3186,5 @@
>    condition: null,
>  
> +  get isPending() {
> +    return this.scripts.size === 0;
> +  },

While this change makes sense, did you do it just for good measure? The breakpoint actors should be in the same state as before.

::: toolkit/devtools/server/tests/unit/test_restoreBreakpoints.js
@@ +19,5 @@
> +    let tab = findTab(tabs, "test");
> +    let [, tabClient] = yield attachTab(client, tab);
> +
> +    let [, threadClient] = yield attachThread(tabClient);
> +    yield resume(threadClient);

Why all of this boilerplate? Didn't we make some init functions that you could use?

Comment 6

[Jason Laster [:jlast]]

this now works