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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ejpbruel, Assigned: past)

Tracking

unspecified
Firefox 34
x86
macOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.