795368 - Add "sources" and "newSource" packets to the RDP, and use them instead of "scripts" and "newScript"

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We are using "scripts" and "newScript" to learn about sources. We should actually deal in sources.

The client should send

{
  type: "sources",
  to: threadID
}

and receive

{
  from: threadID,
  sources: [
    SourceForm1,
    SourceForm2,
    ...
    SourceFormN
  ]
}

where a SourceForm is

{
  actor: sourceActorID,
  url: string
}

for now, and maybe some more additions down the line when Debugger.Source is written.

The unsolicited "newSource" packet should have the form

{
  type: "newSource",
  source: SourceForm
}

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This work was implemented during the team meetup in London, although the relevant changes might need to be pulled out from among other changes, and I assume there is some bit rot.

Everything should be listed in here somewhere:

https://github.com/fitzgen/mozilla-central/commits/source-maps

Specifically,

https://github.com/fitzgen/mozilla-central/commit/95b539ca4f8194743758760dc5fc3a86a62384b4

https://github.com/fitzgen/mozilla-central/commit/277a4e3633640618b71e585ba981661be455c542

Comment 2

[Victor Porof [:vporof][:vp]]

Hey dcamp, are you going to work on this?

Comment 3

[Girish Sharma [:Optimizer]]

Should this be blocking bug 766001 ?
If yes, I think priority can be a little higher :)

Comment 4

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

(In reply to Girish Sharma [:Optimizer] from comment #3)
> Should this be blocking bug 766001 ?
> If yes, I think priority can be a little higher :)

No, bug 766001 does not need this.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

https://tbpl.mozilla.org/?tree=Try&rev=d0ea5abcc921

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This patch adds "sources" and "newSource" packets, but doesn't switch debugger-controller to using them instead of "scripts" and "newScript" yet.

This will need another patch.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v2

https://tbpl.mozilla.org/?tree=Try&rev=e31d01228318

Noticed that for some reason the url for sources and scripts wasn't the full path on B2G, unlike other platforms. This was causing some failures in the try push.

I changed the tests to only check for whether the filename is in the url property, not if the whole thing is the same. Feels kind of dirty, but I'm not sure what else to do and nothing I'm coding should be causing B2G urls to be different, afaict.

Comment 8

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

Comment on attachment 700768 [details] [diff] [review]
v2

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

I actually do like the regexp matches better for the URL tests. We don't need to be concerned with intricate details of the test harness implementation.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +1368,4 @@
>  
>    get threadActor() { return this._threadActor; },
>  
> +  form: function SA_grip() {

s/SA_grip/SA_form/

@@ +1371,5 @@
> +  form: function SA_grip() {
> +    return {
> +      actor: this.actorID,
> +      url: this._url
> +      // TODO fitzgen: introductionScript?

We prefer to tag TODOs with bug numbers instead of people. Bug 637572?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v3

Actually using the sources and newSource packets in debugger-controller.js now.

No try push because my hg account was disabled due to "inactivity" despite me pushing things last week. I'm working on getting it activated again, but I would appreciate it if someone would push to try for me.

I am getting an intermittent TypeError in browser_dbg_bug740825_conditional-breakpoints-01.js and I am not sure if this is related to bug 814311 or if this is my own doing.

Comment 10

[Dave Camp (:dcamp)]

How will this degrade if using Nightly to debug an aurora/beta/release android build?  Are we comfortable requiring a matched desktop/android build for now?

Same question for Firefox OS - I know debugging actors were disabled on devices, but will this make it impossible to debug desktop b2g/simulator?

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So I rebased this patch on fx-team before I uploaded it and I forgot to test again. Turns out a couple script searching tests broke during the rebase.


(In reply to Dave Camp (:dcamp) from comment #10)
> How will this degrade if using Nightly to debug an aurora/beta/release
> android build?  Are we comfortable requiring a matched desktop/android build
> for now?
> 
> Same question for Firefox OS - I know debugging actors were disabled on
> devices, but will this make it impossible to debug desktop b2g/simulator?

This definitely requires a same version debug server and client. Not sure if we have reached the point where we care about backwards compat yet, I will leave that to others to decide.

If we decided we do need to start worrying about backwards compat, does that mean we need a series of feature detection tests when we start up? Or do we just add the same listeners to onScript and onSource? Or...?

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v4

https://tbpl.mozilla.org/?tree=Try&rev=982936457943

Got the backwards compatibility working, based on feature detection. Delays the detection until it is first needed, and then patches in the best methods to use based on the server's compatibility.

Also added a new test debugger server in the xpcshell tests specifically for testing backwards compatibility in the RDP.

All xpcshell tests and mochitests are passing locally.

Comment 13

[Victor Porof [:vporof][:vp]]

Comment on attachment 706001 [details] [diff] [review]
v4

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

I'm chiming in a bit, hope you don't mind :) I have some concerns with the patching of browser_dbg_scripts-searching-files_ui.js, see below.

::: browser/devtools/debugger/debugger-controller.js
@@ +1111,3 @@
>      else {
>        window.clearTimeout(this._newScriptTimeout);
>        this._newScriptTimeout = window.setTimeout(function() {

Since you rename everything else regarding "scripts", maybe also use _newSourceTimeout here instead of _newScriptTimeout?

@@ +1141,4 @@
>     */
> +  _onSourcesAdded: function SS__onSourcesAdded(aResponse) {
> +    if (aResponse.error) {
> +      dumpn(new Error("Error loading sources: " + e.message));

Are you avoiding Cu.reportError for a specific reason? (just curious).

@@ +1233,5 @@
>          return;
>        }
>        aSource.loaded = true;
>        aSource.text = aResponse.source;
>        aCallback(aSource.url, aResponse.source);

aSource.source.url here, right?

::: browser/devtools/debugger/debugger-view.js
@@ +255,5 @@
>      if (!this.editor) {
>        return;
>      }
> +
> +    dumpn("Setting the DebuggerView editor source: " + aSource.source.url.split(/\//g)[-1] +

Why do this split..[-1]?

::: browser/devtools/debugger/test/browser_dbg_scripts-searching-files_ui.js
@@ -116,5 @@
> -  gDebugger.addEventListener("popupshown", function _onEvent2(aEvent) {
> -    gDebugger.removeEventListener(aEvent.type, _onEvent2);
> -    popupshown = true;
> -    executeSoon(proceed);
> -  });

You probably shouldn't remove these listeners if you don't want race conditions emerging. What happens if a Debugger:SourceShown fires too late and interferes with the listener in one of the other functions like goDown or goDownAndWrap?

If you removed these listeners because the source is never changed between firstSearch, secondSearch and thirdSearch, then we have a bigger problem, since the whole purpose of this test is invalidated. While the first match didn't really matter (any result was fine), these following searches need to have exact matches both in the popup and the source editor. We shouldn't only test the url index in the UI.

One easy workaround would be switching the initial script when this test starts (thus waiting for update-editor-mode.html to be shown), and then proceeding with everything normally. This would be the only change needed to fix this test.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Forgot to tell try to actually build things, new push: https://tbpl.mozilla.org/?tree=Try&rev=61b52f4f3f3d

(In reply to Victor Porof [:vp] from comment #13)
> Comment on attachment 706001 [details] [diff] [review]
> v4
> 
> Review of attachment 706001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm chiming in a bit, hope you don't mind :) I have some concerns with the
> patching of browser_dbg_scripts-searching-files_ui.js, see below.

Of course!

> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +1111,3 @@
> >      else {
> >        window.clearTimeout(this._newScriptTimeout);
> >        this._newScriptTimeout = window.setTimeout(function() {
> 
> Since you rename everything else regarding "scripts", maybe also use
> _newSourceTimeout here instead of _newScriptTimeout?

Good call.

> 
> @@ +1141,4 @@
> >     */
> > +  _onSourcesAdded: function SS__onSourcesAdded(aResponse) {
> > +    if (aResponse.error) {
> > +      dumpn(new Error("Error loading sources: " + e.message));
> 
> Are you avoiding Cu.reportError for a specific reason? (just curious).

I should be using Cu.reportError, good catch. Also, should s/e.message/aResponse.error/

> 
> @@ +1233,5 @@
> >          return;
> >        }
> >        aSource.loaded = true;
> >        aSource.text = aResponse.source;
> >        aCallback(aSource.url, aResponse.source);
> 
> aSource.source.url here, right?

Another good catch.

> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +255,5 @@
> >      if (!this.editor) {
> >        return;
> >      }
> > +
> > +    dumpn("Setting the DebuggerView editor source: " + aSource.source.url.split(/\//g)[-1] +
> 
> Why do this split..[-1]?

This can and should be removed, it was just from some debugging that I was doing.

> 
> ::: browser/devtools/debugger/test/browser_dbg_scripts-searching-files_ui.js
> @@ -116,5 @@
> > -  gDebugger.addEventListener("popupshown", function _onEvent2(aEvent) {
> > -    gDebugger.removeEventListener(aEvent.type, _onEvent2);
> > -    popupshown = true;
> > -    executeSoon(proceed);
> > -  });
> 
> You probably shouldn't remove these listeners if you don't want race
> conditions emerging. What happens if a Debugger:SourceShown fires too late
> and interferes with the listener in one of the other functions like goDown
> or goDownAndWrap?
> 
> If you removed these listeners because the source is never changed between
> firstSearch, secondSearch and thirdSearch, then we have a bigger problem,
> since the whole purpose of this test is invalidated. While the first match
> didn't really matter (any result was fine), these following searches need to
> have exact matches both in the popup and the source editor. We shouldn't
> only test the url index in the UI.
> 
> One easy workaround would be switching the initial script when this test
> starts (thus waiting for update-editor-mode.html to be shown), and then
> proceeding with everything normally. This would be the only change needed to
> fix this test.

I will revisit this test.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v5

Addressed the issues brought up by Victor.

Reverted the scripts searching test file to what it was before. I am no longer having the problems I was before, so I assume it was related to a bug in my patch that I have since fixed.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=8cce39431aaa

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Somehow some 'a's got tacked on...

Real link to the try push: https://tbpl.mozilla.org/?tree=Try&rev=8cce39431

Comment 17

[Dave Camp (:dcamp)]

Can we file a bug to remove the compat cruft once we can reasonably stop supporting old clients?

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Dave Camp (:dcamp) from comment #17)
> Can we file a bug to remove the compat cruft once we can reasonably stop
> supporting old clients?

I added this to the etherpad for the march meet up[0], but we need to hash out some kind of policy for how long we want to support older versions of the protocol.

[0]: https://etherpad.mozilla.org/devtools-march-meetup

Comment 19

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

Comment on attachment 706085 [details] [diff] [review]
v5

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

Sending an extra |sources| request just to figure out if the server speaks the new or old protocol is problematic, because these packets incur significant load on the server. In the case of chrome debugging in particular, the server will have to generate the list of all scripts loaded in the browser twice.

I like the idea of memoizing the right protocol handler after discovery, but I think it would be best if we could do it on the first opportune moment, which may be the first newScript/newSource packet, if we're lucky (which will not be uncommon), or the first |sources| packet response. The reason the optimal case will not be that uncommon is that the toolbox opens in the web console by default, and we already have 2 (and soon 3) tools that use the same remote client. The first time a new script will be loaded in the page, no matter which tool is open, the client will be able to patch in the right hanlder from the (possibbly unused) newSource or newScript packet.

I also think that you could hide that process from the client API consumers, by making the client treat addListener("newSource", foo) as a request to trigger foo on both newSource and newScript. That could be in a number of ways:
a) by special-casing these names in EV_addListener
b) by adding some magic in EV_notify to route the packet in the existing handlers of either packet type
c) by adding the same magic in DC_onPacket

I'm not sure if this will end up being less invasive, but I believe that it will pay off in the future when breaking existing client embeddings will be more expensive. Note that even today, with such a small API change we will have to modify the Gecko Profiler add-on, the Firebug WIP, the Graphical Timeline add-on, and maybe others that I'm not aware of.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +715,5 @@
>     * @param string aFile
>     *        The source location to search for.
>     */
>    _performFileSearch: function DVF__performFileSearch(aFile) {
> +

Nit: inadvertent whitespace change.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +849,5 @@
> +   *        Called when the server supports "sources" and "newSource".
> +   * @param aElse Function
> +   *        Called when the server does not support "sources" and "newSource".
> +   */
> +  _ifServerSupportsSources: function TC__ifServerSupportsSources(aThen, aElse) {

We always use verbs for method names, can't you use something like "checkIfSourcesSupported"?

@@ +894,5 @@
> +          } }, 0);
> +        };
> +      threadClient.addNewSourceListener(aOnNewSource, aCallback);
> +    }, function () {
> +      // FITZGEN: does this need to be blown away on detach?

I think it would, if we supported reusing a closed debugger client. Unfortunately (or fortunately in your case!), I don't think this works right now, and AFAIK we always close() the client and throw it away on detach.

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +645,2 @@
>      for (let s of this.dbg.findScripts()) {
> +      if (!this._allowSource(s.url)) {

This check is redundant here, it's the first thing _addScript does.

@@ +654,5 @@
> +  /**
> +   * Handle a protocol request to return the list of loaded scripts.
> +   */
> +  onScripts: function TA_onScripts(aRequest) {
> +    this._discoverScriptsAndSources();

Are you trying to be compatible with old clients here, too? It's a noble goal and doesn't seem too hard in this case, but I don't think it is something we need to support.

@@ +1483,5 @@
>      } catch (e) {
>        // In the xpcshell tests, the script url is the absolute path of the test
>        // file, which will make a malformed URI error be thrown. Add the file
>        // scheme prefix ourselves.
> +      url = "file://" + this._url;

Why this change?

::: toolkit/devtools/debugger/tests/unit/test_sources_backwards_compat-01.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Check basic newSource packet sent from server.

You probably meant to write something like:
"Check that "scripts" and "newScript" packets are handled in a backwards compatible way".

::: toolkit/devtools/debugger/tests/unit/testcompatactors.js
@@ +22,5 @@
> +      }
> +
> +      return { from: "root",
> +               applicationType: "xpcshell-tests",
> +               testConnectionPrefix: this.conn.prefix };

I don't see testConnectionPrefix being used anywhere, you can leave it out. Also, how about adding a |traits| property to be more spec-compliant?

@@ +26,5 @@
> +               testConnectionPrefix: this.conn.prefix };
> +    },
> +
> +    listGlobals: function(aRequest) {
> +      return { from: "root", contexts: [ g.json() for each (g in this._globalActors) ]};

You could even fire a fake newScript packet from this method before returning, for broader coverage.

@@ +31,5 @@
> +    },
> +  };
> +
> +  actor.requestTypes = {
> +    "listContexts": actor.listGlobals,

|listTabs| has been officially specified now, so we can start retiring listContexts even from tests.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #19)
> Comment on attachment 706085 [details] [diff] [review]
> v5
> 
> Review of attachment 706085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sending an extra |sources| request just to figure out if the server speaks
> the new or old protocol is problematic, because these packets incur
> significant load on the server. In the case of chrome debugging in
> particular, the server will have to generate the list of all scripts loaded
> in the browser twice.
> 
> I like the idea of memoizing the right protocol handler after discovery, but
> I think it would be best if we could do it on the first opportune moment,
> which may be the first newScript/newSource packet, if we're lucky (which
> will not be uncommon), or the first |sources| packet response. The reason
> the optimal case will not be that uncommon is that the toolbox opens in the
> web console by default, and we already have 2 (and soon 3) tools that use
> the same remote client. The first time a new script will be loaded in the
> page, no matter which tool is open, the client will be able to patch in the
> right hanlder from the (possibbly unused) newSource or newScript packet.

Can we specify that newSource should always be sent before newScript? This would enable the backwards compatibility code checking for the first newScript/newSource packet a bit cleaner.

It kind of makes sense to me intuitively because you can't create scripts before you have a source.

> 
> I also think that you could hide that process from the client API consumers,
> by making the client treat addListener("newSource", foo) as a request to
> trigger foo on both newSource and newScript. That could be in a number of
> ways:
> a) by special-casing these names in EV_addListener
> b) by adding some magic in EV_notify to route the packet in the existing
> handlers of either packet type
> c) by adding the same magic in DC_onPacket
> 
> I'm not sure if this will end up being less invasive, but I believe that it
> will pay off in the future when breaking existing client embeddings will be
> more expensive. Note that even today, with such a small API change we will
> have to modify the Gecko Profiler add-on, the Firebug WIP, the Graphical
> Timeline add-on, and maybe others that I'm not aware of.

Given this, I think the best option might be to add some kind of abstraction layer inside the debugger client and DC_onPacket where we can centralize all of our backwards compatibility testing and shimming and hook in to this from other places.

I would prefer this approach to messing with EV_notify/EV_addListener, which my first impression of feels really gross b/c it is shared between many of our clients, while the packets in question are only consumed by one of the clients, at the moment. Better to catch and fix compatibility problems before the packet is routed to an actor, me thinks.

--------------------------------

I will make these changes, as well as the changes you suggested inline and upload a new patch.

Comment 21

[Victor Porof [:vporof][:vp]]

Comment on attachment 706085 [details] [diff] [review]
v5

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

Changes in the frontend look excellent, but browser_dbg_scripts-searching-files_ui.js is the same as the last patch.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Victor Porof [:vp] from comment #21)
> Comment on attachment 706085 [details] [diff] [review]
> v5
> 
> Review of attachment 706085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes in the frontend look excellent, but
> browser_dbg_scripts-searching-files_ui.js is the same as the last patch.

Hmmmm, I thought I checked out the fx-team version of that file... Thanks for pointing it out, I will fix this in the next iteration of the patch.

Comment 23

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #20)
> (In reply to Panos Astithas [:past] from comment #19)
> > I like the idea of memoizing the right protocol handler after discovery, but
> > I think it would be best if we could do it on the first opportune moment,
> > which may be the first newScript/newSource packet, if we're lucky (which
> > will not be uncommon), or the first |sources| packet response. The reason
> > the optimal case will not be that uncommon is that the toolbox opens in the
> > web console by default, and we already have 2 (and soon 3) tools that use
> > the same remote client. The first time a new script will be loaded in the
> > page, no matter which tool is open, the client will be able to patch in the
> > right hanlder from the (possibbly unused) newSource or newScript packet.
> 
> Can we specify that newSource should always be sent before newScript? This
> would enable the backwards compatibility code checking for the first
> newScript/newSource packet a bit cleaner.
> 
> It kind of makes sense to me intuitively because you can't create scripts
> before you have a source.

It makes sense to me, too. And if I'm reading your patch correctly that's already the case.

> > I also think that you could hide that process from the client API consumers,
> > by making the client treat addListener("newSource", foo) as a request to
> > trigger foo on both newSource and newScript. That could be in a number of
> > ways:
> > a) by special-casing these names in EV_addListener
> > b) by adding some magic in EV_notify to route the packet in the existing
> > handlers of either packet type
> > c) by adding the same magic in DC_onPacket
> > 
> > I'm not sure if this will end up being less invasive, but I believe that it
> > will pay off in the future when breaking existing client embeddings will be
> > more expensive. Note that even today, with such a small API change we will
> > have to modify the Gecko Profiler add-on, the Firebug WIP, the Graphical
> > Timeline add-on, and maybe others that I'm not aware of.
> 
> Given this, I think the best option might be to add some kind of abstraction
> layer inside the debugger client and DC_onPacket where we can centralize all
> of our backwards compatibility testing and shimming and hook in to this from
> other places.
> 
> I would prefer this approach to messing with EV_notify/EV_addListener, which
> my first impression of feels really gross b/c it is shared between many of
> our clients, while the packets in question are only consumed by one of the
> clients, at the moment. Better to catch and fix compatibility problems
> before the packet is routed to an actor, me thinks.

Agreed.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> It makes sense to me, too. And if I'm reading your patch correctly that's already the case.

It is already the case, but it wouldn't be something that I would want to rely on unless we made it official.

Comment 25

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

I don't expect anyone else to come up with a different debugger server implementation, so this should be as official as it gets :-)

If you mean to mention this in the spec and/or test it in our test suite to make sure this behavior does not regress, then I'm all for it.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Panos,

> |listTabs| has been officially specified now, so we can start retiring
> listContexts even from tests.

Are you saying here that you want me to change the way that `addTestGlobal` and `getTestGlobalContext`/`attachTestGlobalClient` work so that they are using `listTabs` and `attachTab` instead of `listContexts` and `attachContext`? Or for only the new `testcompatactors.js`? I just wanted to confirm with you since this seems like a relatively large change to all of our tests.

Comment 27

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

I meant that we shouldn't add another instance of listContexts, not that you should remove all existing ones in this patch.

Comment 28

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #26)
> Panos,
> 
> > |listTabs| has been officially specified now, so we can start retiring
> > listContexts even from tests.
> 
> Are you saying here that you want me to change the way that `addTestGlobal`
> and `getTestGlobalContext`/`attachTestGlobalClient` work so that they are
> using `listTabs` and `attachTab` instead of `listContexts` and
> `attachContext`? Or for only the new `testcompatactors.js`? I just wanted to
> confirm with you since this seems like a relatively large change to all of
> our tests.

(If you're keen to do it, making the test server behave more like the real server would be great!)

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v6

https://tbpl.mozilla.org/?tree=Try&rev=406d1e1fc28f

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 711928 [details] [diff] [review]
v6

Disregard, the patch broke when rebased on fx-team.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v7

https://tbpl.mozilla.org/?tree=Try&rev=7b501d5e2581

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jim Blandy :jimb from comment #28)
> (If you're keen to do it, making the test server behave more like the real
> server would be great!)

I added a bug to track porting the rest of the bugs over: bug 840292

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v7.1

Very small change from the last patch. Just fixing a small bug where calling getSources from a newSource event listener resulted in an extra "sources" packet getting sent.

Comment 34

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

Comment on attachment 713195 [details] [diff] [review]
v7.1

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

You did very impressive work with the backwards compatibility stuff, kudos! I'm wondering if it's a tad too much to do all that checking for every incoming packet though, and I think Mihai could help us find out if it's significant or not. Mihai, could you run your web console benchmarks with this patch (but without your new console changes) and let us know if the performance is still good?

However I still get the initial "sources" packet twice, in both content and chrome debugging. r=me if you fix that.

We also discussed earlier today that it would probably be a good idea to land this patch after the next uplift, since we will be breaking the remote protocol for the web console in Fx 22 anyway, and the debugger won't be supporting source maps in Fx 21.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +457,5 @@
>     */
> +  onPacket: function DC_onPacket(aPacket, aIgnoreCompatibility=false) {
> +    let packet = aIgnoreCompatibility
> +      ? aPacket
> +      : this.compat.onPacket(aPacket);

I wonder if this will induce significant delay in the local connection case after stable state has been reached (all shims are patched in). A good benchmark will be the web console, since Mihai has been measuring its performance for quite some time now.

@@ +701,5 @@
> +    return this.SUPPORTED;
> +  } else if (aPacket.type === "newScript" ||
> +             aPacket.type === "scripts") {
> +    return this.NOT_SUPPORTED;
> +  } else {

Nit: "else" is redundant.

@@ +1623,5 @@
> + * Takes an array of promises, resolves each of them, and returns a new promise
> + * that resolves to an array of each of those resolved values from the original
> + * array of promises.
> + */
> +function resolveAll(aPromises) {

Joe has another implementation of this in Toolbox.jsm. Can you comment in bug 790195 about this and push for standardizing around a common one?

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +649,5 @@
> +
> +  /**
> +   * Handle a protocol request to return the list of loaded scripts.
> +   */
> +  onScripts: function TA_onScripts(aRequest) {

Shouldn't we add a deprecation comment in the method in order to remove it after some reasonable time, like 6 months?

@@ +1387,4 @@
>   * @param aThreadActor ThreadActor
>   *        The current thread actor.
>   */
> +function SourceActor(aUrl, aThreadActor) {

Nit: I actually think that when we get Debugger.Source it would be more straightforward to convert the script actor if it were still dealing with Debugger.Script objects, but I don't feel strongly about it.

::: toolkit/devtools/debugger/tests/unit/head_dbg.js
@@ +120,5 @@
>  }
>  
> +function getTestTab(aClient, aName, aCallback) {
> +  gClient.listTabs(function (aResponse) {
> +    dump("FITZGEN: aResponse = " + JSON.stringify(aResponse) + "\n");

Don't forget to remove this.

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #34)
> Comment on attachment 713195 [details] [diff] [review]
> v7.1
> 
> Review of attachment 713195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You did very impressive work with the backwards compatibility stuff, kudos!
> I'm wondering if it's a tad too much to do all that checking for every
> incoming packet though, and I think Mihai could help us find out if it's
> significant or not. Mihai, could you run your web console benchmarks with
> this patch (but without your new console changes) and let us know if the
> performance is still good?

Thanks. It will be interesting to see some numbers. I tried to minimize the amount to work we do for each packet by never attempting feature detection after we know whether the server supports it or not, and we only do packet shimming for features we know are not supported. I wouldn't really call it being smart about it, more like not being stupid about it. I don't really see what else I could do to minimize overhead while maintaining our ability to shim packets in a generic fashion.

> 
> However I still get the initial "sources" packet twice, in both content and
> chrome debugging. r=me if you fix that.

Interesting, I thought I fixed that. I will add a test explicitly to make sure we aren't repeating requests.

> 
> We also discussed earlier today that it would probably be a good idea to
> land this patch after the next uplift, since we will be breaking the remote
> protocol for the web console in Fx 22 anyway, and the debugger won't be
> supporting source maps in Fx 21.
> 
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ +457,5 @@
> >     */
> > +  onPacket: function DC_onPacket(aPacket, aIgnoreCompatibility=false) {
> > +    let packet = aIgnoreCompatibility
> > +      ? aPacket
> > +      : this.compat.onPacket(aPacket);
> 
> I wonder if this will induce significant delay in the local connection case
> after stable state has been reached (all shims are patched in). A good
> benchmark will be the web console, since Mihai has been measuring its
> performance for quite some time now.
> 
> @@ +701,5 @@
> > +    return this.SUPPORTED;
> > +  } else if (aPacket.type === "newScript" ||
> > +             aPacket.type === "scripts") {
> > +    return this.NOT_SUPPORTED;
> > +  } else {
> 
> Nit: "else" is redundant.

I think this whole thing would be better as a switch statement, since I am switching branches of logic based on what the packet type is. Pretty much exactly what switches are for. Stylistic thoughts on this?

  switch (aPacket.type) {
  case "newSource":
  case "sources":
    return this.SUPPORTED;
  case "newScript":
  case "sources":
    return this.NOT_SUPPORTED;
  default:
    return this.SKIP;
  }

> 
> @@ +1623,5 @@
> > + * Takes an array of promises, resolves each of them, and returns a new promise
> > + * that resolves to an array of each of those resolved values from the original
> > + * array of promises.
> > + */
> > +function resolveAll(aPromises) {
> 
> Joe has another implementation of this in Toolbox.jsm. Can you comment in
> bug 790195 about this and push for standardizing around a common one?

Sure thing.

> 
> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +649,5 @@
> > +
> > +  /**
> > +   * Handle a protocol request to return the list of loaded scripts.
> > +   */
> > +  onScripts: function TA_onScripts(aRequest) {
> 
> Shouldn't we add a deprecation comment in the method in order to remove it
> after some reasonable time, like 6 months?

My understanding was that they would continue to live side by side. I was under the impression that just because we aren't using scripts now, didn't mean we wouldn't ever use them in the future. I don't know how useful they might be down the line, although I can't think of anything off the top of my head.

> 
> @@ +1387,4 @@
> >   * @param aThreadActor ThreadActor
> >   *        The current thread actor.
> >   */
> > +function SourceActor(aUrl, aThreadActor) {
> 
> Nit: I actually think that when we get Debugger.Source it would be more
> straightforward to convert the script actor if it were still dealing with
> Debugger.Script objects, but I don't feel strongly about it.

See my comments above. I was under the impression that one wasn't going to replace the other.

> 
> ::: toolkit/devtools/debugger/tests/unit/head_dbg.js
> @@ +120,5 @@
> >  }
> >  
> > +function getTestTab(aClient, aName, aCallback) {
> > +  gClient.listTabs(function (aResponse) {
> > +    dump("FITZGEN: aResponse = " + JSON.stringify(aResponse) + "\n");
> 
> Don't forget to remove this.

Woops! Thanks.

Comment 36

[Mihai Sucan [:msucan]]

Comment on attachment 713195 [details] [diff] [review]
v7.1

Panos: thank you for looping me in.

I just did a simple 50 000 messages test with the web console (via console API).

Without this patch: 11-12 seconds.
With this patch: 20-21 seconds.

This is a performance regression, unfortunately. Can we avoid it?

Comment 37

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #35)
> > You did very impressive work with the backwards compatibility stuff, kudos!
> > I'm wondering if it's a tad too much to do all that checking for every
> > incoming packet though, and I think Mihai could help us find out if it's
> > significant or not. Mihai, could you run your web console benchmarks with
> > this patch (but without your new console changes) and let us know if the
> > performance is still good?
> 
> Thanks. It will be interesting to see some numbers. I tried to minimize the
> amount to work we do for each packet by never attempting feature detection
> after we know whether the server supports it or not, and we only do packet
> shimming for features we know are not supported. I wouldn't really call it
> being smart about it, more like not being stupid about it. I don't really
> see what else I could do to minimize overhead while maintaining our ability
> to shim packets in a generic fashion.

If it proves to be a problem, we could stop consulting PC_onPacket after the shims have been patched (like always calling DC_onPacket with aIgnoreCompatibility=true after that).

> > However I still get the initial "sources" packet twice, in both content and
> > chrome debugging. r=me if you fix that.
> 
> Interesting, I thought I fixed that. I will add a test explicitly to make
> sure we aren't repeating requests.

That would be excellent, thanks.

> > @@ +701,5 @@
> > > +    return this.SUPPORTED;
> > > +  } else if (aPacket.type === "newScript" ||
> > > +             aPacket.type === "scripts") {
> > > +    return this.NOT_SUPPORTED;
> > > +  } else {
> > 
> > Nit: "else" is redundant.
> 
> I think this whole thing would be better as a switch statement, since I am
> switching branches of logic based on what the packet type is. Pretty much
> exactly what switches are for. Stylistic thoughts on this?
> 
>   switch (aPacket.type) {
>   case "newSource":
>   case "sources":
>     return this.SUPPORTED;
>   case "newScript":
>   case "sources":
>     return this.NOT_SUPPORTED;
>   default:
>     return this.SKIP;
>   }

That was how I would have written it, but I didn't want to force my style on you :)

> > ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> > @@ +649,5 @@
> > > +
> > > +  /**
> > > +   * Handle a protocol request to return the list of loaded scripts.
> > > +   */
> > > +  onScripts: function TA_onScripts(aRequest) {
> > 
> > Shouldn't we add a deprecation comment in the method in order to remove it
> > after some reasonable time, like 6 months?
> 
> My understanding was that they would continue to live side by side. I was
> under the impression that just because we aren't using scripts now, didn't
> mean we wouldn't ever use them in the future. I don't know how useful they
> might be down the line, although I can't think of anything off the top of my
> head.

OK, I hadn't thought that we might use it again. Let's leave it as it is for now then.

Comment 38

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

(In reply to Mihai Sucan [:msucan] from comment #36)
> Comment on attachment 713195 [details] [diff] [review]
> v7.1
> 
> Panos: thank you for looping me in.
> 
> I just did a simple 50 000 messages test with the web console (via console
> API).
> 
> Without this patch: 11-12 seconds.
> With this patch: 20-21 seconds.
> 
> This is a performance regression, unfortunately. Can we avoid it?

Hmm, what happens if you remove the first 3 lines in DC_onPacket effectively always ignoring this.compat.onPacket? Does that make a difference?

If not, you should describe your test setup so that Nick ca do some profiling on his own.

Comment 39

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #38)
> (In reply to Mihai Sucan [:msucan] from comment #36)
> > Comment on attachment 713195 [details] [diff] [review]
> > v7.1
> > 
> > Panos: thank you for looping me in.
> > 
> > I just did a simple 50 000 messages test with the web console (via console
> > API).
> > 
> > Without this patch: 11-12 seconds.
> > With this patch: 20-21 seconds.
> > 
> > This is a performance regression, unfortunately. Can we avoid it?
> 
> Hmm, what happens if you remove the first 3 lines in DC_onPacket effectively
> always ignoring this.compat.onPacket? Does that make a difference?

Yes, it does make a difference. If I disable compatibility, the patch has no longer any perf impact for the web console.

> If not, you should describe your test setup so that Nick ca do some
> profiling on his own.

I tested with:

http://mihaisucan.github.com/mozilla-work/test.html

(yes, ugly, I know. it's my "recycle bin" where I throw things in, for daily web console testing - quite effective for catching regressions)

The above doesn't test the time it takes for the Web Console async output to finish - I checked manually, using the system time.

I just wrote a cleaner script that tests the performance of the web console, including output performance. Get it from:

http://mihaisucan.github.com/mozilla-work/scratchpad-perf-test.js

Run the above in a browser-context Scratchpad. See the terminal output for results.

With this patch I get:
  elapsed time, 2819ms, loops, 50000
  output flush, 20027ms

Without this patch:
  elapsed time, 2786ms, loops, 50000
  output flush, 12226ms

Comment 40

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #37)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #35)
> > > You did very impressive work with the backwards compatibility stuff, kudos!
> > > I'm wondering if it's a tad too much to do all that checking for every
> > > incoming packet though, and I think Mihai could help us find out if it's
> > > significant or not. Mihai, could you run your web console benchmarks with
> > > this patch (but without your new console changes) and let us know if the
> > > performance is still good?
> > 
> > Thanks. It will be interesting to see some numbers. I tried to minimize the
> > amount to work we do for each packet by never attempting feature detection
> > after we know whether the server supports it or not, and we only do packet
> > shimming for features we know are not supported. I wouldn't really call it
> > being smart about it, more like not being stupid about it. I don't really
> > see what else I could do to minimize overhead while maintaining our ability
> > to shim packets in a generic fashion.
> 
> If it proves to be a problem, we could stop consulting PC_onPacket after the
> shims have been patched (like always calling DC_onPacket with
> aIgnoreCompatibility=true after that).

We can't do this because we can't just shim a method and be done with handling "newSource" packets. We have to actually watch for "newScript" and possibly create a fake "newSource" packet for each of them.

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Here are my results:

WITH PATCH:

start 1360974337327
end 1360974340633
elapsed time 3306ms
loops 50000

WITHOUT PATCH:

start 1360974498633
end 1360974501771
elapsed time 3138ms
loops 50000

This seems like a pretty trivial perf hit to me, considering that this is 50000 console logs.

Comment 42

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v8

https://tbpl.mozilla.org/?tree=Try&rev=b61f3a109321

Fixes nits from previous review, fixes duplicate sources requests being sent on debugger start up.

Comment 43

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #41)
> Here are my results:
> 
> WITH PATCH:
> 
> start 1360974337327
> end 1360974340633
> elapsed time 3306ms
> loops 50000
> 
> WITHOUT PATCH:
> 
> start 1360974498633
> end 1360974501771
> elapsed time 3138ms
> loops 50000
> 
> This seems like a pretty trivial perf hit to me, considering that this is
> 50000 console logs.

These results are not complete.

Sorry, the test is confusing - my bad there. The test shows the elapsed time for executing the 50000 console.log() calls, but all the calls are async in the DOM implementation of the console API. So, while the results you provided are correct, they are not complete - they do not include any of the debugger's code path.

Please rerun the scratchpad perf test and include the last "output flush" - that shows the elapsed time to receive and display the 50000 messages.

Thanks!


(I included both timings in the perf test because I keep track of the DOM console API performance as well. I need to watch them both.)

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: webconsole-perf-test.scratchpad.js

Woops, I had edited the test a little to display results in the webconsole rather than dumping them because I was piping stdout/stderr to /dev/null because I had protocol logging turned on. Basically I forgot to also console.log the flush time. I am attaching my updated version of the perf test.

Anyways, I am seeing a little perf hit, but it still doesn't seem very large to me. Not sure exactly what is going on with the flushing, so maybe you can tell me if this is worrying, Mihai.

------------------------------------------------------------------------------

Without the patch:

start 1361316288793
end 1361316291858
elapsed time 3065ms
loops 50000
output flush 4397ms

With the patch:

start 1361316175504
end 1361316178601
elapsed time 3097ms
loops 50000
output flush 4731ms

Comment 45

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #44)
> Created attachment 715759 [details]
> webconsole-perf-test.scratchpad.js
> 
> Woops, I had edited the test a little to display results in the webconsole
> rather than dumping them because I was piping stdout/stderr to /dev/null
> because I had protocol logging turned on. Basically I forgot to also
> console.log the flush time. I am attaching my updated version of the perf
> test.
> 
> Anyways, I am seeing a little perf hit, but it still doesn't seem very large
> to me. Not sure exactly what is going on with the flushing, so maybe you can
> tell me if this is worrying, Mihai.
...
> output flush 4397ms
...
> output flush 4731ms

This looks to me you are only outputting the first output flush elapsed time. Output all flushes, and take note only of the last output flush, when output of all 50000 messages ends. This is when all execution completes. As previously explained, first output flush starts when the first batch of console API messages is received from the DOM calls. Calls are batched, so the first output flush you see doesn't include the time it took to send all of the messages over the protocol - more are still scheduled to be processed, then displayed.

Comment 46

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #44)
> Created attachment 715759 [details]
> webconsole-perf-test.scratchpad.js

Just checked this script. You kill the output flush callback after the first invocation, which confirms my previous comment.

Comment 47

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for misunderstanding the test! I think I have it right this time.

-------------------------------------------------------------

With patch:

console.log: start, 1361395089349
console.log: end, 1361395092446
console.log: elapsed time, 3097ms
console.log: loops, 50000
console.log: output flush, 4480ms
console.log: output flush, 6120ms
console.log: output flush, 7641ms
console.log: output flush, 9016ms
console.log: output flush, 10569ms
console.log: output flush, 11831ms
console.log: output flush, 13347ms
console.log: output flush, 14619ms
console.log: output flush, 16117ms
console.log: output flush, 17867ms
console.log: output flush, 19011ms
console.log: output flush, 20567ms
console.log: output flush, 21999ms
console.log: output flush, 23154ms

Without patch:

console.log: start, 1361395476618
console.log: end, 1361395479665
console.log: elapsed time, 3047ms
console.log: loops, 50000
console.log: output flush, 4647ms
console.log: output flush, 6093ms
console.log: output flush, 7506ms
console.log: output flush, 9060ms
console.log: output flush, 10270ms
console.log: output flush, 11662ms
console.log: output flush, 12906ms


---------------------------------------------------------------------

So it is almost twice as slow after the patch to completely flush the console messages to the ui. Hrm.

Will look in to things I can do to improve this.

Comment 48

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sorry for misunderstanding the test! I think I have it right this time.

-------------------------------------------------------------

With patch:

console.log: start, 1361395089349
console.log: end, 1361395092446
console.log: elapsed time, 3097ms
console.log: loops, 50000
console.log: output flush, 4480ms
console.log: output flush, 6120ms
console.log: output flush, 7641ms
console.log: output flush, 9016ms
console.log: output flush, 10569ms
console.log: output flush, 11831ms
console.log: output flush, 13347ms
console.log: output flush, 14619ms
console.log: output flush, 16117ms
console.log: output flush, 17867ms
console.log: output flush, 19011ms
console.log: output flush, 20567ms
console.log: output flush, 21999ms
console.log: output flush, 23154ms

Without patch:

console.log: start, 1361395476618
console.log: end, 1361395479665
console.log: elapsed time, 3047ms
console.log: loops, 50000
console.log: output flush, 4647ms
console.log: output flush, 6093ms
console.log: output flush, 7506ms
console.log: output flush, 9060ms
console.log: output flush, 10270ms
console.log: output flush, 11662ms
console.log: output flush, 12906ms


---------------------------------------------------------------------

So it is almost twice as slow after the patch to completely flush the console messages to the ui. Hrm.

Will look in to things I can do to improve this.

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Here are the results of with the patch applied but, without any shims passed to the ProtocolCompatibility layer:

console.log: start, 1361406164431
console.log: end, 1361406167616
console.log: elapsed time, 3185ms
console.log: loops, 50000
console.log: output flush, 4663ms
console.log: output flush, 5987ms
console.log: output flush, 7179ms
console.log: output flush, 8401ms
console.log: output flush, 9616ms
console.log: output flush, 11111ms
console.log: output flush, 12309ms
console.log: output flush, 13993ms
console.log: output flush, 15222ms
console.log: output flush, 16467ms
console.log: output flush, 17806ms
console.log: output flush, 18941ms

Comment 50

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, as discussed on irc, I removed the promise support from PC__detectFeatures/FeatureCompatibilityShim.onPacketTest and this caused a large speed up. I also did a few small tweaks here and there to help speed things up just a bit more. Notably, I added a "sourcesAndNewSource" trait to the hello packet. I know we discussed avoiding traits as much as possible, but the early resolution of feature support pays off in performance gains. Specifically when the user is using the webconsole, but hasn't opened the debugger so the client won't have received any newSource/newScript packets from which to determine support for sources.

Without the patch, the average output flush over three runs of the script was 14666.66ms, with the patch it is 16037.66ms. Is this close enough that we can land the patch?

Comment 51

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v9

https://tbpl.mozilla.org/?tree=Try&rev=9463b13036a5

Comment 52

[Mihai Sucan [:msucan]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #50)
> Ok, as discussed on irc, I removed the promise support from
> PC__detectFeatures/FeatureCompatibilityShim.onPacketTest and this caused a
> large speed up. I also did a few small tweaks here and there to help speed
> things up just a bit more. Notably, I added a "sourcesAndNewSource" trait to
> the hello packet. I know we discussed avoiding traits as much as possible,
> but the early resolution of feature support pays off in performance gains.
> Specifically when the user is using the webconsole, but hasn't opened the
> debugger so the client won't have received any newSource/newScript packets
> from which to determine support for sources.
> 
> Without the patch, the average output flush over three runs of the script
> was 14666.66ms, with the patch it is 16037.66ms. Is this close enough that
> we can land the patch?

This is much better. Thank you!

I see about 10500ms here without the patch, and about 13900ms on average. This might still be a noticeable slowdown for some people, it's quite a lot in percentages.

Comment 53

[Mihai Sucan [:msucan]]

Comment on attachment 716823 [details] [diff] [review]
v9

Giving f+ but please ping Rob and check if this is an acceptable slowdown. I do not have experience with the criteria for performance. If Rob believes this is fine, go ahead and land the patch.

Comment 54

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Update: I am in the process of rebasing the patch on fx-team. I hadn't done so for a couple weeks and now I am getting mochitest failures. Need to get in the habit of rebasing more often.

Comment 55

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v10

Rebased, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4bfa447d1441

I'm getting a single local mochitest failure, and so is Anton on his machine, even though it doesn't show up on try. However, I'm getting it on fx-team and Anton says that he was experiencing this failure on beta as well. browser_dbg_bug723069_editor-breakpoints.js for posterity. Do we have a bug open for this? I'll file one if we need it.

Rob, is the performance hit small enough for us to land this?

Comment 56

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

> I'm getting a single local mochitest failure, and so is Anton on his
> machine, even though it doesn't show up on try. However, I'm getting it on
> fx-team and Anton says that he was experiencing this failure on beta as
> well. browser_dbg_bug723069_editor-breakpoints.js for posterity. Do we have
> a bug open for this? I'll file one if we need it.

bug 845616

Comment 57

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #56)
> bug 845616

This has been happening to me since forever and it's terribly annoying. However, you can bypass it if you open Nightly in low resolution. May be fixed by bug 832920.

Comment 58

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 718659 [details] [diff] [review]
v10

yeah, I don't know about "acceptable" or not. This still seems like a pretty sizeable perf hit, but I don't see much of a way around it.

Maybe land this and file a follow-up bug to investigate a permanent solution.

Comment 59

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

Comment on attachment 718659 [details] [diff] [review]
v10

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

Just a couple of nits and a few suggestions for optimizations. Feel free to ignore them if your testing shows that their effect is insignificant.

::: browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
@@ +263,5 @@
>    function onEditorBreakpointRemoveSecond(aEvent)
>    {
> +    dump("FITZGEN: inside onEditorBreakpointRemoveSecond\n");
> +    dump("FITZGEN:     aEvent = " + JSON.stringify(aEvent) + "\n");
> +

Debugging leftovers.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +659,5 @@
> +const FeatureCompatibilityShim = {
> +  // Constants returned by onPacketTest
> +  get SUPPORTED()     1,
> +  get NOT_SUPPORTED() 2,
> +  get SKIP()          3,

Since we are into perf optimizations, a const number would be faster than a getter.

Also, I think that having them as module-scope constants will be slightly faster, since it would avoid the prototype chain traversal.

@@ +662,5 @@
> +  get NOT_SUPPORTED() 2,
> +  get SKIP()          3,
> +
> +  // The name of the feature
> +  get name() null,

Same here, a string would be better optimized by the compiler. It wouldn't be immutable, but then again we tend to use code review to guard against that around here.

::: toolkit/devtools/debugger/server/dbg-browser-actors.js
@@ +47,5 @@
>      return {
>        from: "root",
>        applicationType: "browser",
> +      traits: {
> +        sourcesAndNewSource: true

Since we added them both at the same time, wouldn't "sources" suffice?

Comment 60

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v10.1

Updated to fix nits. All tests still passing locally.

Can you check this in for me, Panos? Thank you very much!

Comment 61

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

https://hg.mozilla.org/integration/fx-team/rev/5fd83591fc2e

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fd83591fc2e