834756 - Browser debugger dies on step in

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

If I set a debugger statement in my code, open the browser debugger and then click step-in (actually, any time I press step-in) the debugger dies and I get the following output in my console:
DBG-SERVER: Got: {
  "to": "conn0.chromeDebugger2",
  "type": "resume",
  "resumeLimit": {
    "type": "step"
  },
  "pauseOnExceptions": false
}
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921
###!!! ASSERTION: Double-freezing?: '!IsFrozen()', file .../FirefoxSourceDebug/dom/base/nsGlobalWindow.h, line 921

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

To make this clear, this only happens when attempting to debug browser chrome files.

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

This still happens on http://hg.mozilla.org/mozilla-central/rev/29dd80c95b7d

Comment 3

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

This needs an owner. It makes the browser debugger not very friendly.

Comment 4

[Michael Kraft [:morac]]

I'm seeing this on the night loads under Windows.  Any time I click the step in, debugging simply freezes.  The debugger window is not frozen it simply isn't stepping.  The browser being debugged is frozen though since it's stopped, but closing the debugger browser unfreezes the debuggee browser.

It would be nice if this could get fixed as it makes the debugger more or less useless as it is.

Comment 5

[Szczepan Hołyszewski]

As of this comment's date, the nightly is still affected by this bug. Time to bump priority a notch?

Comment 6

[Szczepan Hołyszewski]

Everyone affected please add your votes for this bug.

Comment 7

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

Attachment: Patch v1

I finally found some time to look into this and the main issue is that execution pauses in debugger code, inside the promise library. The problem must be that the promise library is loaded in another compartment before the debugger server is instantiated, so its stack frames appear as debuggee frames.

Black-boxing the promise.js script works as a workaround, but I'll see if I can find a way to make sure the debugger server gets a chance to load the library first.

Comment 8

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

Oh, and obviously using the native promise implementation (bug 856410) would also fix this.

Comment 9

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

Attachment: Black-boxing promises v2

This is a cleaner version of the black-boxing approach.

Comment 10

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

Attachment: Load promises in the debugger's compartment

This approach allows debugging the promise implementation as well (not the one loaded in the server's compartment of course). I've been testing it by stepping frantically through the web console and debugger front-end code and it seems to work fine. The debugger server will use its own undebuggable promise.js instance, while the rest of the devtools will keep on using the debuggable JSM version.

Dave, the change to promise.js is copied almost verbatim from loader.js, with the only difference being that __URI__ is not defined when I load it via loadSubscript.

Victor, the fix in Parser.jsm is not strictly related to this bug, but it came up only when I inspected browser debugger logs. I also updated an unrelated comment in main.js.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 770896 [details] [diff] [review]
Load promises in the debugger's compartment

r+ for the promise.js change. If you could submit that to our github repo that'd be great, otherwise I can do it.

Comment 12

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

Will do.

Comment 13

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

Try run: https://tbpl.mozilla.org/?tree=Try&rev=1d4e6239fe1d

Comment 14

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

Pull request: https://github.com/mozilla/addon-sdk/pull/1077

Comment 15

[:Paolo Amadini]

Note that, in the meantime, we're looking into importing the non-recursive
implementation "resource://gre/modules/Promise.jsm" directly in bug 881050,
unless some of the helper functions of the SDK module are specifically needed.

And bug 881047 is going to import that module as the back-end implementation.

Not sure about how this would affect this specific bug, as we'll be loading the
JSM as a back-end from the script in any case (in order to have only one
centralized walker loop, loaded once per process).

Comment 16

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

I know about that plan and I might even end up volunteering for that work. I expect that loading Promise.jsm via Cu.import will cause this problem to reappear, but I hope to be wrong about it. In any case I feel that the best long-term solution for the debugger server is to use the C++ DOMPromise implementation so that we don't have to worry about those stack frames.

I should note that we use Promise.all a lot in devtools code, which neither Promise.jsm nor DOMPromise provide yet AFAIK.

Comment 17

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #16)
> I know about that plan and I might even end up volunteering for that work.

That would be great, since you're very familiar with the code internals here! :-)

> I expect that loading Promise.jsm via Cu.import will cause this problem to
> reappear, but I hope to be wrong about it. In any case I feel that the best
> long-term solution for the debugger server is to use the C++ DOMPromise
> implementation so that we don't have to worry about those stack frames.

Good to hear that DOMPromise will makes things simpler. We also plan to run the
JSM test suite on that implementation to ease migration.

> I should note that we use Promise.all a lot in devtools code, which neither
> Promise.jsm nor DOMPromise provide yet AFAIK.

True, it's a simple function implemented on top of the core, so we could add that
to Promise.jsm if there is a real need, or make it available in another module.

Comment 18

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

Comment on attachment 770896 [details] [diff] [review]
Load promises in the debugger's compartment

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

Promise changes look good to me. r+ with an unrelated comment below.

::: addon-sdk/source/lib/sdk/core/promise.js
@@ +22,4 @@
>        return imports;
>      }, this[factory.name], { uri: __URI__, id: id });
>      this.EXPORTED_SYMBOLS = [factory.name];
> +  } else if (~String(this).indexOf('Sandbox')) { // Sandbox

Sexy!

::: browser/devtools/shared/Parser.jsm
@@ +1474,4 @@
>        aCallbacks.onArrayExpression(aNode);
>      }
>      for (let element of aNode.elements) {
> +      if (element && typeof this[element.type] == "function") {

This works, but it sounds like we're not supporting a certain type of child in an array expression node. Which we should.
Could you please file a bug about it so I can add a proper fix?

Comment 19

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

Attachment: Patch to land after the updated SDK appears in m-c

(In reply to Victor Porof [:vp] from comment #18)
> This works, but it sounds like we're not supporting a certain type of child
> in an array expression node. Which we should.
> Could you please file a bug about it so I can add a proper fix?

Filed bug 890913.

Dropped the promise.js changes to get a landable patch. I'll check this in after my PR gets merged and the updated SDK gets to fx-team.

Comment 20

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/848e67accaf0fa4d9c7035afef37853e66e5dce0
Make promise.js loadable via mozIJSSubScriptLoader.loadSubscipt (bug 834756)

https://github.com/mozilla/addon-sdk/commit/360c266d1b215facf7a7a4913d3cebaa2810f453
Merge pull request #1077 from past/promise-subscriptloader

Bug 834756, make promise.js loadable via mozIJSSubScriptLoader.loadSubscipt r+=@jsantell

Comment 21

[[github robot]]

Commits pushed to firefox25 at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/848e67accaf0fa4d9c7035afef37853e66e5dce0
Make promise.js loadable via mozIJSSubScriptLoader.loadSubscipt (bug 834756)

https://github.com/mozilla/addon-sdk/commit/360c266d1b215facf7a7a4913d3cebaa2810f453
Merge pull request #1077 from past/promise-subscriptloader

Comment 22

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

https://hg.mozilla.org/integration/fx-team/rev/6e58747730cf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e58747730cf