Closed Bug 834756 Opened 7 years ago Closed 6 years ago

Browser debugger dies on step in

Categories

(DevTools :: Debugger, defect, P2)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: miker, Assigned: past)

References

Details

(Whiteboard: [chrome-debug])

Attachments

(2 files, 2 obsolete files)

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
To make this clear, this only happens when attempting to debug browser chrome files.
Whiteboard: [chrome-debug]
This needs an owner. It makes the browser debugger not very friendly.
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.
OS: Linux → All
Assignee: nobody → past
Priority: -- → P2
As of this comment's date, the nightly is still affected by this bug. Time to bump priority a notch?
Everyone affected please add your votes for this bug.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Status: NEW → ASSIGNED
Oh, and obviously using the native promise implementation (bug 856410) would also fix this.
Attached patch Black-boxing promises v2 (obsolete) — Splinter Review
This is a cleaner version of the black-boxing approach.
Attachment #770344 - Attachment is obsolete: true
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.
Attachment #770896 - Flags: review?(vporof)
Attachment #770896 - Flags: review?(dtownsend+bugmail)
Attachment #770807 - Attachment is obsolete: true
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.
Attachment #770896 - Flags: review?(dtownsend+bugmail) → review+
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).
Blocks: 881050, 881047
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.
(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 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?
Attachment #770896 - Flags: review?(vporof) → review+
(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.
Attachment #770896 - Attachment is obsolete: true
Attachment #770896 - Attachment is obsolete: false
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
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
Blocks: 892813
https://hg.mozilla.org/integration/fx-team/rev/6e58747730cf
Whiteboard: [chrome-debug] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e58747730cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.