Allow a RDP client to get events from the remote server

RESOLVED FIXED in Firefox 18

Status

defect
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
Firefox 18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 4 obsolete attachments)

console.profile/profileEnd send local notifications for the addon to catch and act upon. These notifications need to be sent over the remote debugging protocol as well.
Assignee: nobody → mh+mozilla
This also applies the review comments from bug 751034, that you didn't apply when landing.
Attachment #664037 - Flags: review?(past)
Blocks: 792855
Since I also need other notifications, let's morph this bug.

As discussed on irc, the idea is to be able to register callbacks on the client that would be called when registered topics are observed on the server side.

Basically, the client would register an observer for some topic,
the server would register its own observer for the topic,
that observer would send an "unsolicited notification" to the client
the client would call the corresponding observer when the notification is received.

The tricky part is what to do about the data and subject arguments the observer service gives. I guess we can forward the data if it's jsonable javascript, and ignore it otherwise, and probably just ignore the subject.
Assignee: mh+mozilla → nobody
Component: Gecko Profiler → Developer Tools: Debugger
Product: Core → Firefox
Summary: Send RDP notifications when using console.profile/profileEnd → Allow a RDP client to get events from the remote server
Do you want to change the patch to add an observerNotification packet type to the UnsolicitedNotifications like we discussed? Then you could change the actor to send that type plus a console-api-profiler subtype and you'd be all set.
I will do that. I'll also replace the notifications added in bug 751034.
Assignee: nobody → mh+mozilla
The unWrapper thing is kind of gross, but short of duplicating the js object, which may be even worse, i have no better idea. Tested with the events i do care about at the moment, namely profiler-started, profiler-stopped, garbage-collection-statistics, cycle-collection-statistics and console-api-profiler.
Attachment #664037 - Attachment is obsolete: true
Attachment #664037 - Flags: review?(past)
Attachment #667404 - Flags: review?(past)
Attachment #667404 - Attachment is obsolete: true
Attachment #667404 - Flags: review?(past)
Attachment #667453 - Flags: review?(past)
Comment on attachment 667453 [details] [diff] [review]
Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

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

Looks good to me. I assume you exercise this code path in some of the existing profiler tests? Otherwise a new test would be appropriate.

Actually, with the current profiler client being an add-on, I'm not sure what the testing situation is, but I just want to bring this to your attention.

::: toolkit/devtools/debugger/server/dbg-profiler-actors.js
@@ +22,5 @@
>      if (this._profiler && this._started) {
>        this._profiler.StopProfiler();
>      }
>      this._profiler = null;
> +    for each (var event in this._observedEvents) {

We prefer for...of instead of the deprecated for each...in nowadays. You have a bunch of these in this patch.
Attachment #667453 - Flags: review?(past) → review+
bzexport has been known to fail to add reviewers on attachments to this bug. Please ignore; testing.
Assignee: mh+mozilla → sphink
Oops, should've used --no-take
Assignee: sphink → mh+mozilla
Depends on: 797717
With for .. of instead of for each .. in.
Attachment #667453 - Attachment is obsolete: true
Comment on attachment 667929 [details] [diff] [review]
Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

Carrying over r+
Attachment #667929 - Flags: review+
Attachment #667931 - Flags: review?(past)
Comment on attachment 667931 [details] [diff] [review]
Tests for the debugging server profiler actor

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

Nice! One check is failing for me locally, but I'm sure you can fix that.

::: toolkit/devtools/debugger/tests/unit/test_profiler_actor.js
@@ +119,5 @@
> +    var sample = aResponse.profile.threads[0].samples[Math.floor(aResponse.profile.threads[0].samples.length / 2)];
> +    do_check_eq(sample.name, "(root)");
> +    do_check_eq(typeof sample.frames, "object");
> +    do_check_neq(sample.frames.length, 0);
> +    do_check_true(sample.frames.some(function(f) {

This check fails locally on OS X 10.8 debug. I added some dumps and I get this:

0.70 f.line == stack.lineNumber: false
0.70 f.line: undefined
0.70 stack.lineNumber: 107
0.70 f.location == stack.name + etc.: false
0.70 f.location: 0x101e96441
0.70 stack.name + etc.: test_profile (/Users/past/src/fx-team/obj-ff-dbg/_tests/xpcshell/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js:100)
0.70 TEST-UNEXPECTED-FAIL| /Users/past/src/fx-team/obj-ff-dbg/_tests/xpcshell/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js | false == true - See following stack:
0.70 JS frame :: /Users/past/src/fx-team/testing/xpcshell/head.js :: do_throw :: line 451
0.70 JS frame :: /Users/past/src/fx-team/testing/xpcshell/head.js :: _do_check_eq :: line 545
0.70 JS frame :: /Users/past/src/fx-team/testing/xpcshell/head.js :: do_check_eq :: line 566
0.70 JS frame :: /Users/past/src/fx-team/testing/xpcshell/head.js :: do_check_true :: line 580
0.70 JS frame :: /Users/past/src/fx-team/obj-ff-dbg/_tests/xpcshell/toolkit/devtools/debugger/tests/unit/test_profiler_actor.js :: <TOP_LEVEL> :: line 123
0.70 JS frame :: resource://gre/modules/devtools/dbg-client.jsm :: DC_onPacket :: line 411
0.70 JS frame :: chrome://global/content/devtools/dbg-transport.js :: <TOP_LEVEL> :: line 212
0.70 native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
Attachment #667931 - Flags: review?(past) → review+
remove observers before (automatically) shutting down profiler. This avoids possibly trying to send notifications on a closed connection.
Attachment #667929 - Attachment is obsolete: true
Comment on attachment 667964 [details] [diff] [review]
Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

Carrying over r+
Attachment #667964 - Flags: review+
(In reply to Panos Astithas [:past] from comment #16)
> Created attachment 667971 [details]
> Log from failing test

Ah, you need the patch from bug 797717 for that to work.
Confirmed! I also just noticed that this test leaks, but I'm not sure if it's the test or the profiler actor.
I think it's the profiler alone.
Attachment #668117 - Flags: review?(past)
Blocks: 798047
No longer blocks: 798047
Attachment #668117 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/7d8ad53cab46
https://hg.mozilla.org/mozilla-central/rev/c0c3b137c10d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Depends on: 799323
Depends on: 810185
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.