793672 - Allow a RDP client to get events from the remote server

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Send RDP notifications when using console.profile/profileEnd

This also applies the review comments from bug 751034, that you didn't apply when landing.

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

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

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.

Comment 4

[Mike Hommey [:glandium]]

I will do that. I'll also replace the notifications added in bug 751034.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

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.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

I wanted unregistration as well.

Comment 7

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

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.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Please ignore - testing bzexport review failures

bzexport has been known to fail to add reviewers on attachments to this bug. Please ignore; testing.

Comment 9

[Steve Fink [:sfink] [:s:]]

Oops, should've used --no-take

Comment 10

[Mike Hommey [:glandium]]

Attachment: Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

With for .. of instead of for each .. in.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 667929 [details] [diff] [review]
Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

Carrying over r+

Comment 12

[Mike Hommey [:glandium]]

Attachment: Tests for the debugging server profiler actor

Comment 13

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

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

Comment 14

[Mike Hommey [:glandium]]

Attachment: Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

remove observers before (automatically) shutting down profiler. This avoids possibly trying to send notifications on a closed connection.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 667964 [details] [diff] [review]
Allow to get notifications for arbitrary observer topics through RDP with the profiler actor

Carrying over r+

Comment 16

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

Attachment: Log from failing test

Comment 17

[Mike Hommey [:glandium]]

(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.

Comment 18

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

Confirmed! I also just noticed that this test leaks, but I'm not sure if it's the test or the profiler actor.

Comment 19

[Mike Hommey [:glandium]]

I think it's the profiler alone.

Comment 20

[Mike Hommey [:glandium]]

Attachment: Additional test

Comment 21

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8ad53cab46
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c3b137c10d

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7d8ad53cab46
https://hg.mozilla.org/mozilla-central/rev/c0c3b137c10d