Closed Bug 1037514 Opened 7 years ago Closed 7 years ago

Merging all clients into a single map breaks detaching from top-level actors

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: ejpbruel, Assigned: past)

Details

Attachments

(1 file)

Bug 797621 introduced a change that merged all top-level clients (tabs, addons, threads) into a single map. This caused one of my patches in bug 1035206 to break.

The problem is with this line:
http://lxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#901

Previously, this condition would only hold true if the type of the packet was in ThreadStateTypes *and* we had a corresponding thread client for the packet's actor id.

Note however, TabClient, AddonClient, and WorkerClient can also receive "detached" packets, even though they dont have an onThreadState method. This wasn't a problem before, but now that we've thrown all clients together in a single map, we try to call onThreadState for these clients as well when we receive a "detached" packet (for some reason this triggers a silent error, any idea why that is?).

I've been unable to find a test that properly exercises TabClient.detach, which is probably why this didn't turn up during testing. We do of course call TabClient.detach when the client connection is closed, but since close returns immediately, we would never notice if the callback for the first call to TabClient.detach never got called.
Maybe we should just add a check in that line for an _onThreadState handler. By having a handler the client will opt-in to state changes.
This seems to work fine. I also figured out why 'const noop' didn't work before.
Attachment #8461599 - Flags: review?(nfitzgerald)
Assignee: nobody → past
Status: NEW → ASSIGNED
Comment on attachment 8461599 [details] [diff] [review]
Make thread state changes opt-in for clients with an onThreadState handler

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

LGTM
Attachment #8461599 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/mozilla-central/rev/362b6f577380
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.