Closed Bug 1132501 Opened 10 years ago Closed 9 years ago

Turn on the debugger whenever the devtools are opened

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

(Whiteboard: [polish-backlog][difficulty=hard] )

Attachments

(1 file, 18 obsolete files)

1132501.patch
(deleted), patch
Details | Diff | Splinter Review 
This is in prep for bug 670002. Let's go ahead and turn on the debugger if it's not already started when you are in the console. Note that this will be most of the time people open devtools since it's the first tool open.

shu has done amazing work to make sure that it doesn't slow down the page a ton (everything can still go through Ion). We can make it even better over time.

My argument is this: if turning it on really is that bad, we need to fix that instead of avoiding it here. I would guess that the majority of users use our debugger a lot, so we should accept this instead of trying to optimize for the other case.

It also allows us to pause on `debugger` statements finally instead of just ignoring them if you haven't clicked the debugger stop, which is *super* weird.

I'm going to do some research to see what the perf hit is, but in my early experiments it's been fine so far.
Depends on: 1130214
Marking as blocking 956087, since this is specific to the console but would provide the groundwork for potentially enabling the debugger statement across the toolbox.
Blocks: 956087
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Marking as blocking 956087, since this is specific to the console but would
> provide the groundwork for potentially enabling the debugger statement
> across the toolbox.

It definitely would. I think we should go through a release just enabling it on the toolbox to make sure there aren't complaints about the console making the page slower. I also need to look at the other tools because many of them (profiler, etc) have to enable debug mode as well, and if that's the case it really shouldn't be a problem making it toolbox-wide.
Interesting graph from canuckistani: http://devtools-dash.paas.canuckistani.ca/tools-relative.html
I just tested the performance impact. I found this demo: https://github.com/turbulenz/gargantia_editor. It uses the 3d Turbulenz engine, which is pretty large and complex, and runs a somewhat complex 3d flight game on it.

I added code in one of the entities that averages out the amount of time between updates for that specific entity. This roughly measures the time between frames. The pseudo-code would be:

      if(numSamples < 200) {
        var now = Date.now();
        avg = ((avg * numSamples) + (now - lastRunTime)) / (numSamples + 1);
        numSamples++;
        lastRunTime = Date.now();
      }
      else {
        console.log('avg:', avg);
      }


I don't even do any console logs, because I don't care how about the performance of the console. All I care about is if enabling the debugger incurs a significant perf hit. Results:

with devtools closed: 16.53
with devtools open, but debugger not initialized: 16.76
with devtools open and debugger initialized: 16.54

The game is obviously running within it's 60 FPS budget, and requestAnimationFrame is limiting the frames to run 16.6ms apart (= 60 FPS). The may be a performance hit (which we don't see because of rAF throttling) but the important part is that it seems to be negligible. The game can still run smoothly, and this is a lot of code.

I can find more test cases.

It definitely affects startup time, but that because the debugger is fully initialized and is fleshing out the whole UI with all the sources even though I'm not looking at it. I think that startup time is solvable; all we need is the Debugger object from it.
jryans said he has a b2g app he'd be willing to fire up on a real b2g phone, and to see if there's an obvious performance degradation when enabling the debugger. Try interacting with the game and the console first before clicking the debugger tab, then click the debugger tab and see if it's noticeably slower. Thanks!
Flags: needinfo?(jryans)
(In reply to James Long (:jlongster) from comment #5)
> jryans said he has a b2g app he'd be willing to fire up on a real b2g phone,
> and to see if there's an obvious performance degradation when enabling the
> debugger. Try interacting with the game and the console first before
> clicking the debugger tab, then click the debugger tab and see if it's
> noticeably slower. Thanks!

I did not notice a notable slow down with the debugger tab opened.  It seemed to run at the same speed as before.

The game does use asm.js.
Flags: needinfo?(jryans)
Depends on: 1137384
Blocks: 1147988, 1177279
Attached patch 1132501.patch (obsolete) (deleted) — — Splinter Review 
This is just a prototype. I'm waiting on a clobber build to finish to even test it, but I'm turning in for the night and wanted to post this. Think of it as pseudo-code.

As I dug into it more, it seems like if we are going to do this that it makes sense to do it on the `TabTarget`. It's unclear to me exactly the difference of `TabTarget` and `TabClient` though. Looking at `makeRemote` in `TabTarget`, though, it does basic bootstrapping of what a connected tab needs. If we are going to always be attaching the thread, we should just do it here in the tab, and the tab always has a `tab.threadClient` available.

Or is there a better place for this? I think we should do this "right", and it seems like we could potentially clean up some of this other code (it assigns a `threadActor` variable in `makeRemote`, not sure why).
Attachment #8667064 - Flags: review?(bgrinstead)
Assignee: nobody → jlong
Attached patch 1132501.patch (obsolete) (deleted) — — Splinter Review 

        Attachment #8667064 -
        Attachment is obsolete: true

        Attachment #8667064 -
        Flags: review?(bgrinstead)

        Attachment #8667066 -
        Flags: review?(bgrinstead)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Had to fix the reference to `Prefs`. Just tested, and this actually seems to work!

Now I just need to change the debugger so the the tab shows that it's paused if it hasn't been initialized yet.

        Attachment #8667066 -
        Attachment is obsolete: true

        Attachment #8667066 -
        Flags: review?(bgrinstead)

        Attachment #8667069 -
        Flags: review?(bgrinstead)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is working really well. This patch makes the target save the paused packet so that the debugger frontend can get it when it's initializing, and set the UI to the paused state if it should be (highlighting the line that paused, etc).

This also changes the debugger behavior to automatically switch to the debugger if a breakpoint or debugger statement is it. I've always wanted it to do that, and I think most people would expect that.

        Attachment #8667069 -
        Attachment is obsolete: true

        Attachment #8667069 -
        Flags: review?(bgrinstead)

        Attachment #8667343 -
        Flags: review?(bgrinstead)

    
    

    
  
One thing I don't understand is what the `WindowTarget` class is for in target.js. It won't have a threadClient, but now it sort of needs one. The protocol is for targets to always have one now.

    
    

    
  
Comment on attachment 8667343 [details] [diff] [review]
1132501.patch

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

My recommendation is we should still use a workflow like the inspector, as in the toolbox should cause these additional requests when it starts.

Adding additional startup tasks to target or DebuggerClient is less good, because WebIDE creates many of these things for tab tracking, etc. and won't necessarily open a toolbox with them.

Since we want this special behavior for the toolbox, it should be the toolbox that triggers the requests that make it happen.

Now, the toolbox is a complicated module, so let's break it out if you want.  Simple modules are certainly nicer. :)

        Attachment #8667343 -
        Flags: review-

    
    

    
  
(In reply to James Long (:jlongster) from comment #11)
> One thing I don't understand is what the `WindowTarget` class is for in
> target.js. It won't have a threadClient, but now it sort of needs one. The
> protocol is for targets to always have one now.

WindowTargets were **meant** to be used when making a target for an entire chrome window[1] (so that would suggest Browser Toolbox).

However, I think it's really an outdated, pre-RDP API.  The only uses I see end up in dead code.  I filed bug 1209634 to remove WindowTargets.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/gDevTools.jsm#555

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
I like how this is turning out. Basically I rip the thread state stuff out of the target, and the toolbox automatically attaches/detaches the thread. The toolbox still manually fires paused/resumed events on the target because it seems like listening to those events on the target is used a lot in other code.

The toolbox handles highlighting and selecting the debugger when paused/resumed. And if you want to check the state of the thread, you can just use the threadClient instance.

        Attachment #8667343 -
        Attachment is obsolete: true

        Attachment #8667343 -
        Flags: review?(bgrinstead)

        Attachment #8667475 -
        Flags: review?(jryans)

    
    

    
  
Comment on attachment 8667475 [details] [diff] [review]
1132501.patch

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

ETOOMANYNITS

Overall, I think it's looking, but it would be good to clean up the thread-* event listeners from target, instead of emitting the event back there.

::: devtools/client/debugger/debugger-controller.js
@@ +216,5 @@
> +    DebuggerView.Toolbar.toggleResumeButtonState(
> +      this.activeThread.state,
> +      !!pausedPacket
> +    );
> +    if(pausedPacket) {

Nit: if (

(Try ESLint!)

::: devtools/client/framework/attach-thread.js
@@ +2,5 @@
> +const Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
> +const promise = require("promise");
> +
> +function handleThreadState(toolbox, event, packet) {
> +  toolbox.target.emit("thread-" + event);

Would it make more sense for the other consumers who listen for this on `target` to instead get the `threadClient` from the toolbox?

It seems surprising to me to emit this on the target, now that there is nothing else about thread state in the target.

@@ +4,5 @@
> +
> +function handleThreadState(toolbox, event, packet) {
> +  toolbox.target.emit("thread-" + event);
> +
> +  if(event === "paused") {

Nit: if (

@@ +7,5 @@
> +
> +  if(event === "paused") {
> +    toolbox.highlightTool("jsdebugger");
> +
> +    if(packet.why.type === 'debuggerStatement' ||

Nit: if (, use " not '

@@ +13,5 @@
> +       packet.why.type === 'exception') {
> +      toolbox.raise();
> +      toolbox.selectTool("jsdebugger");
> +    }
> +  }

Nit: } else {

@@ +37,5 @@
> +    }
> +    threadClient.addListener("paused", handleThreadState.bind(null, toolbox));
> +    threadClient.addListener("resumed", handleThreadState.bind(null, toolbox));
> +
> +    if (!threadClient.paused) {

Why are we always paused?  It seems like we did not assume that before.

@@ +62,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function detachThread(threadClient) {

Add a comment that we'll send a `detach` request when the target detaches from the entire DebuggerClient.

::: devtools/client/framework/toolbox.js
@@ +253,5 @@
>    get target() {
>      return this._target;
>    },
>  
> +  get threadClient() {

Add a comment about why this is at the toolbox level.

::: devtools/shared/client/main.js
@@ +84,5 @@
>      if (!this._listeners || !this._listeners[aName]) {
>        return;
>      }
> +
> +    if(!aListener) {

Nit: if (

@@ +86,5 @@
>      }
> +
> +    if(!aListener) {
> +      this._listeners[aName] = [];
> +    }

Nit: } else {

        Attachment #8667475 -
        Flags: review?(jryans) → feedback+

    
    

    
  
Pushed to try with DAMP enabled.  Will do some retriggers once it finishes to see if we can detect any regressions

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=769bce3ee777
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=769bce3ee777

    
    

    
  
I fixed a few of the tests so this should look better. Still probably some related failures though.

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd420cc0170c
remote:
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=bd420cc0170c

    
    

    
  
So it looks like there's definitely a perf regression associated with this change (scroll down to 'damp opt'): https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=288d12e9236b&newProject=try&newRevision=769bce3ee777

I've retriggered the ones that don't have many runs, but linux64 and windowsxp are both showing ~8% regressions.

Having a bit of trouble parsing the subtest view but here it is: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=288d12e9236b&newProject=try&newRevision=769bce3ee777&originalSignature=25169736932930bc20f0437757e9399dadf68976&newSignature=25169736932930bc20f0437757e9399dadf68976

    
    

    
  
OK an update about the subtest view on perfherder - I've asked on IRC and the :wlach has confirmed that there's a frontend bug in the perfherder UI that's causing some data to not show up in that page, and that there should be a fix deployed soon for that.

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #19)
> OK an update about the subtest view on perfherder - I've asked on IRC and
> the :wlach has confirmed that there's a frontend bug in the perfherder UI
> that's causing some data to not show up in that page, and that there should
> be a fix deployed soon for that.

See Bug 1210009 for that work

    
    

    
  
Here's a push that doesn't block on attaching a thread. Code looks like this:

      attachThread(this).then(thread => {
        this._threadClient = thread;
      });


remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=43aeb71f4a88
remote:
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=43aeb71f4a88

    
    

    
  
Just for kicks, here is a push with the attach/detach commented out. There should be no difference. Any of the jsdebugger runs will fail though.

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab59f681438
remote:
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=7ab59f681438

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Here's my latest patch. This still blocks opening on attaching the thread, but most of the tests should be fixed. I'm not sure about this though:

 851 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_split_persist.js | leaked until shutdown [nsGlobalWindow #2774 outer about:blank] - expected PASS

    1133487 Intermittent browser_webconsole_split_persist.js | uncaught exception - TypeError: self._init is not a function at chrome://global/content/bindings/toolbar.xml:144

TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_view_source.js | application terminated with exit code 1

1148225 Intermittent browser_webconsole_view_source.js | application terminated with exit code -11
1145481 Intermittent browser_webconsole_view_source.js | application crashed [@ nsStandardURL::~nsStandardURL()] 

I don't really know even how to debug the crash. And that leak happened in an opt build, which is weird so I'm not sure how to debug that either if it's not the typical debug leak (I can't repro either locally).

Any help with this would be great. And with reading DAMP and making decisions on what we should do.

    
  

        Attachment #8667475 -
        Attachment is obsolete: true

    
    

    
  
Are you able to reproduce locally? If so, make a debug build and add `--debugger gdb` to the end of the mach command. Then go from there.

    
    

    
  
@fitzgen sort of, for me it just hangs (note that it's the very last test, and individually the test runs fine, so there's something else going on). Thanks, I'll see what I can do.

    
    

    
  
I'd say first step is try with a debug build.

    
    

    
  
(In reply to James Long (:jlongster) from comment #22)
> Just for kicks, here is a push with the attach/detach commented out. There
> should be no difference. Any of the jsdebugger runs will fail though.
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ab59f681438
> remote:
> remote: It looks like this try push has talos jobs. Compare performance
> against a baseline revision:
> remote:  
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=7ab59f681438

Ah, looks like DAMP is failing with that patch applied:

2015-09-30 13:08:46,714 DEBUG : BROWSER_OUTPUT: Full message: TypeError: this._toolbox.threadClient is undefined
2015-09-30 13:08:46,730 DEBUG : BROWSER_OUTPUT: Message: TypeError: this._toolbox.threadClient is undefined
2015-09-30 13:08:46,809 DEBUG : BROWSER_OUTPUT: Full message: TypeError: panel is undefined

    
    

    
  
@bgrins yeah, that must be my other changes that assume it's there (thought that code wouldn't be hit). if we think this case is really important I can get it working, but seems like comparing it to a normal rev is the same thing.

    
    

    
  
FWIW, to make sure there is no impact (even small), I just ran the Octane benchmarks with and without the Debugger attached and there's literally zero difference. I looked at the actual Octane scores and the total time spent from our performance tool. I don't think there's even a small impact. This has always been the theory (JS executes normally now under a debugger), but it's nice to see it play out that way.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
rebased latest patch

        Attachment #8668042 -
        Attachment is obsolete: true

    
  
Whiteboard: [polish-backlog][difficulty=hard]

    
    

    
  
One of the last failing tests is failing because of this specific line: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/webconsole/test/browser_webconsole_bug_595350_multiple_windows_and_tabs.js#L63

I guess we never opened multiple toolboxes at the same time with the debugger enabled. I looked at the protocol log and for some reason *all* toolboxes end up targeting the *same* thread, so somehow when we are connecting we're using something like `activeTab` and because everything opens up at once it ends up being the last opened tab. I'm not exactly sure if that's what happening, but it sure seems like it. I can't find any code where this obviously happens.

If I wait 1 second between opening tabs, the test passes. This is what I changed the code to;

    setTimeout(() => {
      openConsole(tab).then(function(index, hud) {
        ok(hud, "HUD is open for tab " + index);
        let window = hud.target.tab.linkedBrowser.contentWindow;
        window.console.log("message for tab " + index);
        consolesOpen++;
        if (consolesOpen == 4) {
          // Use executeSoon() to allow the promise to resolve.
          executeSoon(closeConsoles);
        }
      }.bind(null, i));
    }, i * 1000);

    
    

    
  
Actually I was wrong, they are different threads. I didn't realize the different `conn#`.

JWL added threadClient server1.conn0.context26
JWL added threadClient server1.conn1.context26
JWL added threadClient server1.conn2.context26
JWL added threadClient server1.conn3.context26

Still though, opening the consoles after 1s each time fixed it so not sure what's going on.

    
    

    
  
I figured it out (not really working on this today but still posting my results from yesterday). Threads are paused when they are initially attached, and the client sends a subsequent resume request. We force resuming the event loops to happen in a LIFO order: the last one to pause needs to be the next one that resumes. I'm assuming that 2 tabs are opening toolboxes, attaching, but resuming in a different order that they attached. The code to enforce this is here: https://github.com/mozilla/gecko-dev/blob/master/devtools/server/actors/script.js#L989-L991

If there are multiple event loops in a ThreadActor, does that mean that there's only one ThreadActor for the content process and thus *all* pages, even across tabs and windows? I actually forgot it worked that way. I need to look at this patch and see if I'm doing things correctly. I don't know how sending multiple `attach` requests works; it appears that multiple ThreadActors are created but somehow the event loops are communicating to each other (to know which order they are resumed).

    
    

    
  
I fixed that test failure. Let's see what else shows up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0732ca504bb4

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
rebased

        Attachment #8668688 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Previous patch was rebased wrongly, here's one where all console tests pass locally.

        Attachment #8673824 -
        Attachment is obsolete: true

    
    

    
  
Oops, pasted the wrong link. https://treeherder.mozilla.org/#/jobs?repo=try&revision=58a051dd4bcb

Looks like those errors magically went away! Getting close.

    
    

    
  
Oops, previous comment was not meant for this bug.

    
    

    
  
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> 
> ::: devtools/client/framework/attach-thread.js
> @@ +2,5 @@
> > +const Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
> > +const promise = require("promise");
> > +
> > +function handleThreadState(toolbox, event, packet) {
> > +  toolbox.target.emit("thread-" + event);
> 
> Would it make more sense for the other consumers who listen for this on
> `target` to instead get the `threadClient` from the toolbox?
> 
> It seems surprising to me to emit this on the target, now that there is
> nothing else about thread state in the target.
> 

I know it's been a while, but I realized I never actually responded to this. I agree, but can I do that as a follow-up? This already feels a bit tricky to me so I'd like to go head and land it, and then it shouldn't be hard to update listeners of those events.

    
    

    
  
(In reply to James Long (:jlongster) from comment #45)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15)
> > 
> > ::: devtools/client/framework/attach-thread.js
> > @@ +2,5 @@
> > > +const Services = Cu.import("resource://gre/modules/Services.jsm", {}).Services;
> > > +const promise = require("promise");
> > > +
> > > +function handleThreadState(toolbox, event, packet) {
> > > +  toolbox.target.emit("thread-" + event);
> > 
> > Would it make more sense for the other consumers who listen for this on
> > `target` to instead get the `threadClient` from the toolbox?
> > 
> > It seems surprising to me to emit this on the target, now that there is
> > nothing else about thread state in the target.
> > 
> 
> I know it's been a while, but I realized I never actually responded to this.
> I agree, but can I do that as a follow-up? This already feels a bit tricky
> to me so I'd like to go head and land it, and then it shouldn't be hard to
> update listeners of those events.

I can live with that assuming you file the follow up and add comments with the bug number where the emitting and listening happens.

    
  
Depends on: 1224280

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501-asmjs.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This is a small patch on top of the last one. It keeps asm.js enabled until the debugger is actually used, which is very important if we're going to attach the thread eagerly.

r? fitzgen because this actually change the default of all Debugger instances to *not* disable asm.js, and only the debugger disables it when it's opened. Might affect other tools that use Debugger instances.

        Attachment #8686703 -
        Flags: review?(nfitzgerald)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501-asmjs.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Forgot a few cleanups

        Attachment #8686703 -
        Attachment is obsolete: true

        Attachment #8686703 -
        Flags: review?(nfitzgerald)

        Attachment #8686704 -
        Flags: review?(nfitzgerald)

    
    

    
  
I am going to write a few tests before landing this, but I want to see if anything is already breaking: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58984cb0c26c

    
    

    
  
Comment on attachment 8686704 [details] [diff] [review]
1132501-asmjs.patch

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

LGTM, just a couple comments below.

::: devtools/client/debugger/debugger-controller.js
@@ +226,5 @@
>      }
>  
> +    // Disable asm.js so that we can set breakpoints and other things
> +    // on asm.js scripts
> +    this.reconfigureThread({ disableAsmJS: true });

I think we should name this "observeAsmJS" or "observeAndDisableAsmJS" because "disableAsmJS" doesn't explain at all *why* you might want to do that.

@@ +320,5 @@
> +      aResponse => {
> +        if (aResponse.error) {
> +          let msg = "Couldn't reconfigure thread: " + aResponse.message;
> +          Cu.reportError(msg);
> +          dumpn(msg);

While you're here, would you mind removing Cu.reportError and dumpn and using DTU.reportException in their place? Thanks!

::: devtools/server/actors/script.js
@@ +686,5 @@
> +    if ('disableAsmJS' in options) {
> +      this.dbg.allowUnobservedAsmJS = !options.disableAsmJS;
> +    }
> +
> +    update(this._options, options || {});

Apparently options can be null/undefined and we handle that with this line, but the "in" operator above will throw before we get here. Make a default for options, or guard all this stuff on it or something.

        Attachment #8686704 -
        Flags: review?(nfitzgerald) → review+

    
  
No longer depends on: 1224280

    
    

    
  

      
      
      
      

        Attached patch
          
          
        p.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
update w/fitzgen's comments (now calling the flag observeAsmJS).

Also fixed a major problem with regards to the initial pausing & resuming. The engine is initially paused, so we have to send a resume. I'm doing that in `attachThread` and blocking on it, so tools don't need to wait for a resume anymore. Needed to clean up a few places to make tests running again. Will post a try run soon.

We probably should talk more if we should block on that initial resume or not. I think it's fine to block

        Attachment #8674297 -
        Attachment is obsolete: true

        Attachment #8686704 -
        Attachment is obsolete: true

    
    

    
  
I think I've addressed all the above failures. optimistic that this'll work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3f1a581756

    
    

    
  
Argh. Lost almost a whole day to this one. Sending the "observeAsmJS" flag in the "reconfigure" message when the debugger starts up was clearing out the sources, which made for some weird side effects (like multiple newSource notifications for an HTML file instead of just one, which triggered multiple `fetchEventListeners` requests...). Long story short, I definitely found the problem for this one. Let's see if there's anything else.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dff80766c55

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8687454 -
        Attachment is obsolete: true

    
  
Blocks: 1225492

    
    

    
  
I think this patch is ready to do. Unfortunately, we seem to have several intermittent tests that make it hard to decide if it's my fault or not. But looking through all those failures, I don't think any of them are mine.

The red dt2 failures, for instance, were blobuploader failures. All of the tests passed, but for some reason the machines had trouble uploaded stuff afterwards.

    
    

    
  
Oops, meant to post this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d204ea38d3d. I'm going to use checkin-needed to get the sheriff's opinion if I can push this or not.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8688132 -
        Attachment is obsolete: true

    
    

    
  
sheriffs: there are failures in the above try but they seem like known intermittents and I can't imagine they are related. I don't know why they failed more than once or twice though.
Keywords: checkin-needed

    
    

    
  
I went ahead and pushed this myself because I'm very eager to see if this is going to stick. I want this to land before thanksgiving next week.
Keywords: checkin-needed

    
    

    
  
All of the Gaia unit tests started failing with https://treeherder.mozilla.org/logviewer.html#?job_id=5800458&repo=fx-team when this landed. Backing you out to see if that's just a coincidence:

https://hg.mozilla.org/integration/fx-team/rev/8e22c8aac023
Flags: needinfo?(jlong)

    
    

    
  
I'm at an absolute loss as to why this is making the mulet builds fail for gaia. I was able to reproduce it locally by building a mulet build and running gaia locally. This file opens up the devtools: https://github.com/mozilla/gecko-dev/blob/master/b2g/chrome/content/desktop.js

Line 187 is where it opens them. If I delay opening them by a second or two, everything works fine. There is an error in the console that acts is if these two files are loaded in the wrong order: https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/index.html#L49-L50. I have *no* idea why attaching the thread would make them load in the wrong order.

This is so out of my comfort zone I'm worried about landing this before Thanksgiving, which is a real bummer :( Any insight would be appreciated...
Flags: needinfo?(jlong)

    
    

    
  
Alex you've worked with this stuff, and maybe can give me some clues. How does the mulet tests get notified that the devtools is finished loading? The devtools is going to load a little slower now, which is the only reason I can think of why it might be affecting things.
Flags: needinfo?(poirot.alex)

    
    

    
  
I found it! 

Well, I found the crash. I finally figured out how to run the gaia unit tests locally under a real mulet build & config (this isn't actually documented anywhere, it's a combination of multiple setups), and sure enough, Firefox crashes and burns hard right before the test starts.

This is clearly a deeper issue within SpiderMonkey, where hooking a Debugger up freaks something out. Now I just need to dig around for the crash log which doesn't seem to be anywhere. Once I find more information about the crash I can file a new bug and it's not my problem anymore!

    
    

    
  
I give up for now. I made a video, see end of my comment. I'm definitely going to need somebody else's help who is more familiar with how Gaia runs unit tests. It appears that Firefox isn't actually crashing; for some insane reason the test agent connection just randomly disconnects right before the tests start running. I have *zero* idea why this is connected to my patch.

To sum up: this patch connects a Debugger to the page whenever the toolbox opens, instead of waiting for the debugger to be used. The broke the gaia unit test runner under mulet, as seen here on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=464bbc35cd63. Mulet runs gaia within Firefox with responsive mode turned on and the devtools automatically opened (so there is a connection to my patch but this shouldn't really affect it). The tests just stops early with the message "Build shut down unexpectedly" with my patch. I was able to reproduce this locally after setting up a mulet build and running gaia unit tests.

Somehow, connecting a debugger to the window under mulet makes it freak out. But Firefox is *not* crashing. I paused the python test runner and Firefox actually ran the tests fine and continued running; it's just the connection between the test client & server that closes for some reason.

Watch this for a thorough walkthrough and to watch it happen: http://jlongster.com/s/failing-gaia.mp4

    
    

    
  
STR:

1. apply this patch on fx-team or master
2. Build it with `ac_add_options --enable-application=b2g/dev`
2. Set $FIREFOX to your build: `export FIREFOX=/Users/james/projects/mozilla/gecko-dev-quickfix/obj-x86_64-apple-darwin15.0.0/dist/NightlyDebug.app/Contents/MacOS/firefox`
3. checkout gaia and `cd` into it
4. Run `NO_LOCK_SCREEN=1 DEBUG=1 DESKTOP=0 make`
5. Run `python tests/python/gaia-unit-tests/gaia_unit_test/main.py --binary $FIREFOX --profile ~/projects/mozilla/gaia/profile-debug system/test/unit/hierarchy_manager_test.js`

The last command runs a unit test in the mulet build with our patch. You'll see the "build shutdown unexpectedly" message.

    
    

    
  
Hum, this isn't really optimal to run tests with devtools displayed to start with.
But your patch shouldn't also break anything...
I'll take a look, but I can't promise the outcome, as you already chased this quite well!

    
    

    
  
If you look at every failure on try push, you can see that it is always the same stdout.
We see a stacktrace from devtools toolbox opening, due to a dbg_assert call.
Then various stuff related to gaia test harness and b2g initialization.
And it always fails like this:
15:55:14     INFO -  XXX FIXME : Dispatch a mozChromeEvent: apps-update-check
15:55:15     INFO -  JavaScript error: jar:file:///home/worker/build/application/firefox/omni.ja!/components/BrowserElementParent.js, line 1176: NS_ERROR_NOT_IMPLEMENTED: SetNFCFocus for in-process mode is not yet supported
15:55:17     INFO -  Build shut down unexpectedly

I'm wondering if that's just a race in the test harness itself.
Like, attaching debuggers slow down the startup of everything (gecko, gaia and the test harness) and the test misbehaves and kill itself because of a nasty race?
Flags: needinfo?(poirot.alex)

    
    

    
  
Not really a race, but yes, the debugger is messing up with javasript behavior.
If you comment this line, tests work:
  http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#1935
  1935     this.unsafeSynchronize(this.sources.createSourceActors(aSource));

I have to idea why we use unsafeSynchronize in so many places, but may be we really shouldn't ?!?

I tried hard to figure out what exactly go wrong on test-agent side,
but haven't been able to figure out anything more than you. The Websocket shuts down on the python/server side and then the python automatically kills firefox.
If unsafeSynchronize breaks WebSocket API itself and make it close unexpectedly, it is going hard to find any additional clue.

Note that unsafeSynchronize is being called when onNewScript fires for app://test-agent.gaiamobile.org/common/test/agent.js.

    
    

    
  
That's too bad. I'm actually not sure about that call to it, though there's a comment that tries to explain it. I know we need the 2nd call to it further down because we need to fetch and parse sourcemaps, but block on it because we may need to set breakpoints on the script.

I think we can eventually get rid of that first call to it. But not before this patch lands...

Thanks for figuring out that line specifically. I don't know why it would mess up websockets; we've never had any reports of the debugger having problem with websockets.

I see two potential fixes:

1. Delay opening the devtools for a second or two (after the test files load). Presumably that should fix the issue; no onNewScript events will fire.

2. Add an option to `openToolbox` to not automatically attach the thread. We can remove this later once we get a proper fix in.

    
    

    
  
(In reply to James Long (:jlongster) from comment #71)
> I see two potential fixes:
> 
> 1. Delay opening the devtools for a second or two (after the test files
> load). Presumably that should fix the issue; no onNewScript events will fire.
> 
> 2. Add an option to `openToolbox` to not automatically attach the thread. We
> can remove this later once we get a proper fix in.

A time-based delay sounds like a recipe for more intermittents, so I'd suggest going with #2.

    
    

    
  
Yeah, that's probably best for now. I'm *very* curious why it breaks things, but the synchronize is a hack and I'm pretty sure we can the one that seems to break this so I guess we'll just plan for that in the future.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Alright, this patch adds an option to `showToolbox` to not attach the thread. I don't know if you all want to look at it or not.

Now I think there are still some other test failures...

        Attachment #8688539 -
        Attachment is obsolete: true

    
    

    
  
I also filed bug 1227317 to get rid of that nasty `unsafeSynchronize`.

    
    

    
  
This is now blocked on bug 1113930, because it made a previous intermittent crash basically perma-fail. On linux debug it consistently fails with the crash linked to that bug. Luckily, somebody is working on it.
Depends on: 1113930

    
    

    
  
Oops, wrong bug: bug 1224822
Depends on: 1224822
No longer depends on: 1113930

    
    

    
  
I applied the patch from bug 1224822 to confirm that it will actually fix it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f298456fde7e

If that works, I think this patch will be ready to land once the other one does.

    
    

    
  
The patch that I applied was faulty, I got a new one from BenWa. Let's see if it fixes it... https://treeherder.mozilla.org/#/jobs?repo=try&revision=2598191dad78

    
  
Blocks: 1231258

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
rebased & slightly updated patch

        Attachment #8691051 -
        Attachment is obsolete: true

    
    

    
  
Getting very, very close. Fixed several nasty failures, I think there is one last test to fix that is leaking a window. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e22dfda61b1c

    
  
Depends on: 1113930

    
    

    
  
Latest try with the patch from bug 1113930. There are still some obscure failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0986c2e5bdbe

    
  
Depends on: 1232709

    
  
Depends on: 1234756

    
    

    
  
I found a fix for bug 1234756. Here's the latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63eb6db15805

Also running talos for this one: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=63eb6db15805

Side note: we *really* need to clean up `_addSource` in the debugger server (that's where I had to fix the aforementioned bug)

    
    

    
  
\o/ -o/ \o- I think the try gods finally looked on my favorably! This patch actually works finally!!

The above run had the patch from bug 1113930 applied. Along the way I discovered the tweaking something within the debugger server made things a lot more calm, so I'm curious if I don't need that fix anymore. Here's a run without it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dcace340f02

It's not a big deal if that's the only bug blocking this. Looks like they are close to a solution.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This patch has changed quite a bit, so time for a new review. I don't expect it to be done over Christmas, of course.

        Attachment #8701662 -
        Flags: review?(bgrinstead)

    
    

    
  
Comment on attachment 8701662 [details] [diff] [review]
1132501.patch

Nick, it would be good if you checked out the debugger server changes. I discovered that we were calling `unsafeSynchronize` even if we don't have any breakpoints to set, which is pretty bad. Before we were strict on that, but we've introduced a second call to it which happens all the time.

That means we're calling `unsafeSynchronize` and spinning up an event loop for every newSource notification. Now that we attach the thread by default, that is happening for *every* devtools test, which seems to freak out a lot of stuff. Just look at the try run a few comments up. Various obscure weird C++ crashes happened, but removing the `unsafeSynchronize` call by default fixed all of that.

That probably means there are real bugs with it, but not calling it when we have no breakpoints is a good fix. It should also make startup faster.

We really should clean up `_addSource`, it's very ugly and confusing now.

        Attachment #8701662 -
        Flags: feedback?(nfitzgerald)

    
    

    
  
A note for my future self: I just realized that by avoiding the `unsafeSynchronize` call I just fixed that weird mulet test runner problem, described in comment 70. It was the source of all of that, so now I should be able to revert special-casing that test runner.

    
  

        Attachment #8699554 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8701662 [details] [diff] [review]
1132501.patch

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

::: devtools/server/actors/script.js
@@ +689,2 @@
>      // Update the global source store
> +    this.sources.reconfigure(options);

Why doesn't this pass `this._options` anymore? The idea is that each reconfigure request needn't send the whole new option state, just the changed attributes. Thus, `options` here is only the changed attributes, not the whole state, which is what `this._options` is.

        Attachment #8701662 -
        Flags: feedback?(nfitzgerald)

    
  
No longer depends on: 1232709

    
  
Summary: Turn on the debugger whenever the console is opened → Turn on the debugger whenever the devtools are opened

    
    

    
  
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #91)
> Comment on attachment 8701662 [details] [diff] [review]
> 1132501.patch
> 
> Review of attachment 8701662 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/server/actors/script.js
> @@ +689,2 @@
> >      // Update the global source store
> > +    this.sources.reconfigure(options);
> 
> Why doesn't this pass `this._options` anymore? The idea is that each
> reconfigure request needn't send the whole new option state, just the
> changed attributes. Thus, `options` here is only the changed attributes, not
> the whole state, which is what `this._options` is.

If you look at the `reconfigure` method `TabSources.js`, I also changed that to accept a partial configuration object. It will only change options that exist in the object, instead of applying the entire object (assuming values are false by default).

I may look into this again and see if there's a cleaner way to do it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Rebased patch

        Attachment #8701662 -
        Attachment is obsolete: true

        Attachment #8701662 -
        Flags: review?(bgrinstead)

        Attachment #8703856 -
        Flags: review?(bgrinstead)

    
    

    
  
Comment on attachment 8703856 [details] [diff] [review]
1132501.patch

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

FYI I've started reviewing this and will continue tomorrow

::: devtools/client/framework/gDevTools.jsm
@@ -381,5 @@
>     * @param {Toolbox.HostType} hostType
>     *        The type of host (bottom, window, side)
>     * @param {object} hostOptions
>     *        Options for host specifically
> -   *

Looks like an unintentional whitespace-only change

::: devtools/client/framework/target.js
@@ +526,5 @@
>      }
>    },
>  
> +  getPausedDetails: function() {
> +    return this._pausedPacket;

I don't think this function is ever called or this._pausedPacket is ever set

::: devtools/client/webconsole/test/browser_webconsole_bug_1050691_click_function_to_source.js
@@ -13,5 @@
>  add_task(function*() {
>    yield loadTab(TEST_URI);
>    let hud = yield openConsole();
>  
> -  yield testWithoutDebuggerOpen(hud);

This removal appears to be removing actual code coverage (i.e. does clicking on a variable before the debugger has been opened work?).  Does this code have to be removed for the test to pass?

    
    

    
  
You are right on the first two comments (that functionality was moved to devtools/shared/client/main.js). Good catch.

For the test, yes it has to be removed. The problem before is that we didn't have the debugger backend available to get an object actor to pass to the debugger. Therefore if the debugger hadn't been initialized yet, it would open the variables side panel in the console. If it had been, it will go to the function in the source.

Now the debugger is *always* initialized, so we can always go to the function in the source when clicking on it in the console. We'll never show it in a variables view. Whether or not we want to change that is a different issue, but now there is no concept of "debugger not open".

    
    

    
  
Another try run just in cast the review is mostly formatting; wanted a fresh run done by the end of today.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=447ef726c875
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=447ef726c875

    
    

    
  
Comment on attachment 8703856 [details] [diff] [review]
1132501.patch

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

Mostly just some questions and small comments.  Please respond to the questions before landing, but r=me.

I noticed a visual glitch when triggering a debugger statement while the inspector is opened.  The message with "Debugger is paused.  Some features like mouse selection will not work." slides down for a second then back up.  It's a visual polish item, but can you file a follow up to fix that?

::: devtools/client/debugger/content/reducers/sources.js
@@ +19,5 @@
>    case constants.ADD_SOURCE:
>      emitChange('source', action.source);
>      return mergeIn(state, ['sources', action.source.actor], action.source);
>  
> +  case constants.LOAD_SOURCES:

Why is this needed?

::: devtools/client/debugger/debugger-controller.js
@@ +304,5 @@
>      // emit individual `newSource` notifications, which trigger
>      // separate actions, so this won't do anything other than force
>      // the server to traverse sources.
> +    this.dispatch(actions.loadSources()).then(() => {
> +      // If the engine is already paused, update the UI to represent the

How is the pause state UI related to loading sources?  Could this code happen on it's own outside of the promise resolution?

@@ -401,5 @@
> -      this.connectThread();
> -
> -      if (aThreadClient.paused) {
> -        aThreadClient.resume(res => {
> -          this._ensureResumptionOrder(res)

There are two places that got removed where _ensureResumptionOrder was called - are they not needed anymore?  It also used to guard resuming on `if (aThreadClient.paused)` but in attach-thread.js it resumes unconditionally.  Is this OK?

::: devtools/client/debugger/test/mochitest/browser_dbg_break-unselected.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test setting breakpoints on an eval script

This comment needs to be updated to match what the tests is doing

@@ +24,5 @@
> +      // Wait until it's actually fully loaded
> +      yield toolbox.loadTool("jsdebugger");
> +
> +      const panel = toolbox.getCurrentPanel();
> +      const actions = bindActionCreators(panel);

Nit: `actions` and `constants` are unused

::: devtools/client/debugger/test/mochitest/browser_dbg_bug-896139.js
@@ +21,5 @@
> +
> +    let Sources = win.DebuggerView.Sources;
> +    yield waitForDebuggerEvents(panel, win.EVENTS.SOURCE_SHOWN);
> +    if (Sources.selectedItem.attachment.source.url.indexOf(SCRIPT_URL) === -1) {
> +      Sources.selectedValue = getSourceActor(win.DebuggerView.Sources, EXAMPLE_URL + SCRIPT_URL)

I don't understand this - why is the selectedValue not set correctly already / why can't we still use waitForSourceShown?

::: devtools/client/framework/attach-thread.js
@@ +21,5 @@
> +      toolbox.raise();
> +      toolbox.selectTool("jsdebugger");
> +    }
> +  }
> +  else if (event === "resumed") {

Nit: please move the else up onto the same line as brace

::: devtools/client/framework/toolbox.js
@@ +1963,5 @@
>      // Destroy the profiler connection
>      outstanding.push(this.destroyPerformance());
>  
> +    // Detach the thread
> +    detachThread(this._threadClient);

There is a potential race here if destroy is called before the thread is finished being attached.  This isn't something we handle very well generally in the toolbox now, but I have a feeling we should yield on opening before starting a destroy.  This could be done in a follow up since it might end up being a fair amount of work

::: devtools/client/webide/test/test_runtime.html
@@ +17,5 @@
>      <script type="application/javascript;version=1.8">
>        window.onload = function() {
>          SimpleTest.waitForExplicitFinish();
>  
> +        const { DebuggerServer } = require("devtools/server/main");

Is this change intentional?

::: devtools/server/actors/script.js
@@ +677,5 @@
>    onReconfigure: function (aRequest) {
>      if (this.state == "exited") {
>        return { error: "wrongState" };
>      }
> +    const options = aRequest.options || {};

I'm not sure about this file - Nick has reviewed it, right?

        Attachment #8703856 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Thanks for the review!

(In reply to Brian Grinstead [:bgrins] from comment #98)
> Comment on attachment 8703856 [details] [diff] [review]
> 1132501.patch
> 
> Review of attachment 8703856 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly just some questions and small comments.  Please respond to the
> questions before landing, but r=me.
> 
> I noticed a visual glitch when triggering a debugger statement while the
> inspector is opened.  The message with "Debugger is paused.  Some features
> like mouse selection will not work." slides down for a second then back up. 
> It's a visual polish item, but can you file a follow up to fix that?

Oh, interesting. I will file a follow up and look at that very soon. It looks like the inspector shows that message when it's highlighted and the debugger is paused? So the expected behavior is that the message stays up?

> 
> ::: devtools/client/debugger/content/reducers/sources.js
> @@ +19,5 @@
> >    case constants.ADD_SOURCE:
> >      emitChange('source', action.source);
> >      return mergeIn(state, ['sources', action.source.actor], action.source);
> >  
> > +  case constants.LOAD_SOURCES:
> 
> Why is this needed?

The source loading behavior before was a little weird: when you attach the debugger, you get `newSource` notifications for all future sources that come into existence. So when you reload the page, you'll get a dump of `newSource` notifications, which triggers the `ADD_SOURCE` action and adds a single source.

But what about the initial list of sources when the tools open? Previously, we would call the `sources` RDP command which gets the current list of sources. This forces all the sources to *be* created, and the server actually sends us a bunch of individual `newSource` packets. So even though we got a big list of sources from the `sources` RDP request, we already received all of them as individual `newSource` notifications. We just ignored the result of the `LOAD_SOURCES` action for optimization.
 
Yeah. Super non-intuitive.

Now, the `sources` RDP request will most likely *not* trigger a flood of `newSource` notifications because the backend has probably already created them (because it was attached earlier, and something else could have requested specific sources to sourcemap or something). So we need to actually use the result of the `LOAD_SOURCES` action and "merge in" all of the source into our sources state.

That makes a lot more sense code-wise, anyway. We still might duplicate work (we had already added the source to our state because we also got a `newSource` notification), but all of this only happens when the toolbox initially loads, so the duplicate work isn't a huge deal.

This code will be a good bit cleaner when we use React to render the sources, because I can forego all the `emitChange` calls which is most of the code.

> 
> ::: devtools/client/debugger/debugger-controller.js
> @@ +304,5 @@
> >      // emit individual `newSource` notifications, which trigger
> >      // separate actions, so this won't do anything other than force
> >      // the server to traverse sources.
> > +    this.dispatch(actions.loadSources()).then(() => {
> > +      // If the engine is already paused, update the UI to represent the
> 
> How is the pause state UI related to loading sources?  Could this code
> happen on it's own outside of the promise resolution?

The engine might pause before the debugger UI is even created. We need to see if that's the case, and put the UI in the position of "paused at this location".

We need to wait until the sources are loaded because the `this.StackFrames._onPaused("paused", pausedPacket);` call manually triggers a UI update to select the right source. We can't select a source until it's loaded into the UI.

When we switch out sources with a React component we probably don't need to put this much code here, and could declaratively select a source before it even exists in the UI. Hm, we might be able to do that now actually. I'd rather clean this code up after landing it, if that's ok.

> @@ -401,5 @@
> > -      this.connectThread();
> > -
> > -      if (aThreadClient.paused) {
> > -        aThreadClient.resume(res => {
> > -          this._ensureResumptionOrder(res)
> 
> There are two places that got removed where _ensureResumptionOrder was
> called - are they not needed anymore?  It also used to guard resuming on `if
> (aThreadClient.paused)` but in attach-thread.js it resumes unconditionally. 
> Is this OK?

Good questions. For the latter, I think it's ok to unconditionally resume. The problem before (maybe?) was that the user could somehow request a resume before everything was done loading. Now, we block the entire toolbox opening on the resume, so it should be deterministic. I haven't noticed any test failures regarding this, and I like keeping this straight-forward unless there's a good reason.

About `_ensureResumptionOrder`, the only checks I removed were the once for initially attaching the thread. Seems like we should also check that when resuming in other places in the debugger... But I think you're right. I should check for the `wrongOrder` return value in attach-thread.js and display a warning if we hit that.

> 
> ::: devtools/client/debugger/test/mochitest/browser_dbg_break-unselected.js
> @@ +1,5 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +/**
> > + * Test setting breakpoints on an eval script
> 
> This comment needs to be updated to match what the tests is doing
> 
> @@ +24,5 @@
> > +      // Wait until it's actually fully loaded
> > +      yield toolbox.loadTool("jsdebugger");
> > +
> > +      const panel = toolbox.getCurrentPanel();
> > +      const actions = bindActionCreators(panel);
> 
> Nit: `actions` and `constants` are unused
> 
> ::: devtools/client/debugger/test/mochitest/browser_dbg_bug-896139.js
> @@ +21,5 @@
> > +
> > +    let Sources = win.DebuggerView.Sources;
> > +    yield waitForDebuggerEvents(panel, win.EVENTS.SOURCE_SHOWN);
> > +    if (Sources.selectedItem.attachment.source.url.indexOf(SCRIPT_URL) === -1) {
> > +      Sources.selectedValue = getSourceActor(win.DebuggerView.Sources, EXAMPLE_URL + SCRIPT_URL)
> 
> I don't understand this - why is the selectedValue not set correctly already
> / why can't we still use waitForSourceShown?

It's not a very well-written test. Basically, there are 2 sources, the HTML and the single JS source. It's not deterministic which one gets selected. Honestly, I can't remember exactly why, because it always *should* load it in the same order, and it should choose the first one in the list, but something was happening. It was probably related to GC issues, like the HTML source was GCed too quickly so it didn't even show up.

We wait for a source to be displayed, and if it's not the right one, we just make sure to select the one we want.

In most tests, we use `debugger` to actually break in the script that we want shown, and then we can just wait for that specific source to be shown.

I'd rather not get bogged down in cleaning up too many tests... But I agree we should make this test better at some point.

> 
> ::: devtools/client/framework/attach-thread.js
> @@ +21,5 @@
> > +      toolbox.raise();
> > +      toolbox.selectTool("jsdebugger");
> > +    }
> > +  }
> > +  else if (event === "resumed") {
> 
> Nit: please move the else up onto the same line as brace
> 
> ::: devtools/client/framework/toolbox.js
> @@ +1963,5 @@
> >      // Destroy the profiler connection
> >      outstanding.push(this.destroyPerformance());
> >  
> > +    // Detach the thread
> > +    detachThread(this._threadClient);
> 
> There is a potential race here if destroy is called before the thread is
> finished being attached.  This isn't something we handle very well generally
> in the toolbox now, but I have a feeling we should yield on opening before
> starting a destroy.  This could be done in a follow up since it might end up
> being a fair amount of work

Agreed, seems like it could be destroyed before the `open` promise finishes. I can file follow-up to make sure it gets looked into.

> 
> ::: devtools/client/webide/test/test_runtime.html
> @@ +17,5 @@
> >      <script type="application/javascript;version=1.8">
> >        window.onload = function() {
> >          SimpleTest.waitForExplicitFinish();
> >  
> > +        const { DebuggerServer } = require("devtools/server/main");
> 
> Is this change intentional?

Yep, I ended up not needing to change that test but it was erroring before because it was referencing `DebuggerServer` in the cleanup code but it was importing it below it.

> 
> ::: devtools/server/actors/script.js
> @@ +677,5 @@
> >    onReconfigure: function (aRequest) {
> >      if (this.state == "exited") {
> >        return { error: "wrongState" };
> >      }
> > +    const options = aRequest.options || {};
> 
> I'm not sure about this file - Nick has reviewed it, right?

Yes, I think it's OK the way it is. I renamed the `TabSources` method to make it clear that it's not a full `reconfigure`.

    
    

    
  
See bug 1132501 for visual glitch

See bug 1237110 for `destroy` waiting until the toolbox is fully opened

    
    

    
  
(In reply to James Long (:jlongster) from comment #99)
> Thanks for the review!
> 
> (In reply to Brian Grinstead [:bgrins] from comment #98)
> > Comment on attachment 8703856 [details] [diff] [review]
> > 1132501.patch
> > 
> > Review of attachment 8703856 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Mostly just some questions and small comments.  Please respond to the
> > questions before landing, but r=me.
> > 
> > I noticed a visual glitch when triggering a debugger statement while the
> > inspector is opened.  The message with "Debugger is paused.  Some features
> > like mouse selection will not work." slides down for a second then back up. 
> > It's a visual polish item, but can you file a follow up to fix that?
> 
> Oh, interesting. I will file a follow up and look at that very soon. It
> looks like the inspector shows that message when it's highlighted and the
> debugger is paused? So the expected behavior is that the message stays up?

I think the expected behavior is that the message goes away, but not in such a slow way.  The message is expected to be visible only when the inspector is opened but since we switch to debugger panel it should go away.  Maybe even just preventing the slide animation when it closes, or in the situation where it hasn't finished opening before it goes away.  We may end up revisiting this UI altogether with a sort of overlay over the page when paused.

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #101)
> 
> I think the expected behavior is that the message goes away, but not in such
> a slow way.  The message is expected to be visible only when the inspector
> is opened but since we switch to debugger panel it should go away.  Maybe
> even just preventing the slide animation when it closes, or in the situation
> where it hasn't finished opening before it goes away.  We may end up
> revisiting this UI altogether with a sort of overlay over the page when
> paused.

Yeah! I've talked to Helen about that. I think that would be cool, and we could even have some interactions on the page and description about why it's paused.

Also about the resumption order, I definitely need to implement it in this patch. It looks the debugger handles it right now, and I should probably just make `attach-thread.js` own that functionality (always checking the return value of the resume request). Although the debugger shows a debugger-specific overlay, so I need to think about this.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1132501.patch (deleted)
        — 
          — Splinter Review
      

    


  
Tweak patched based on review. Not many changes, and try run looks good.

        Attachment #8703856 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/256e3e81fed7
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox46:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

    
    

    
  
In the dupe bug 1237636, a user gave a testcase to verify it's fixed.
STR:
1) Load http://theapsgroup.github.io/firefox-43-endless-parsing/index.html
2) Open dev tools (F12)
3) Select the Debugger tab
4) Press F5 to refresh
5) After the browser tab finished to spin, click on the Console tab.
Result:
"script tag" 
"empty.js"
"end of body"
6) Repeat steps 3 to 5

When the bug appears, the browser tab never stops spinning and "end of body" is not displayed.
Depends on: 1258154

    
    

    
  
Is this really x86 Mac OS X only? It seems it caused bug 1258154 at least on Windows 64 bit.

    
    

    
  
(In reply to Oriol from comment #109)
> Is this really x86 Mac OS X only? It seems it caused bug 1258154 at least on
> Windows 64 bit.

No, the platform fields are often out of date since it defaults to the platform the user who filed the bug
No longer depends on: 1258154

    
    

    
  
It looks like the commits in this bug caused a regression, see bug 1259831.

    
    

    
  
Let's discuss on the other bug.

    
    

    
  
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Successful.

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Actual Results: 
As expected

Expected Results: 
Debugger has started as soon as console opens.
Depends on: 1237795

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: