STR: 1. Add "debugger;" to the top of your favorite file * I used browser/devtools/webide/content/webide.js 2. Start Firefox with this change 3. Open the Browser Toolbox 4. Ensure Debugger panel is active 5. Trigger your file to load * Open WebIDE in my example 6. "debugger;" statement is hit correctly * The stack looks normal 7. Move to the next line with "step over" ER: We should move to the next line in the chosen file. AR: Instead, we end up somewhere inside Promise-backend.js no matter what the debugged file was. Further stepping attempts seem to just grow the stack inside this file with no hope of escape.

I'm experiencing the same issue, since yesterday, so this is a recent regression. My STR are similar to Ryan's, except that I'm not using a debugger statement, this happens too if I just set a breakpoint: - Open the browser toolbox - Set a breakpoint you know you'll hit - Wait for the debugger to pause - Step over => The debugger now shows Promise-backend.js and the first line of Handler.prototype.process to be the current line. The variables sidebar also shows properties and variables for this line.

I fixed the same bug a couple of years ago, by making sure that the promise library was loaded in a compartment invisible to the debugger. Since we haven't move to native promises yet, we need to keep doing this.

I've got the window down to one merge[1], but there are still quite a few interesting changes in there. [1]: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f0a4844f0ccd&tochange=43ef69c1f6b5

Building locally confirms the cause is bug 1137384. James, are you able to investigate this?

[Tracking Requested - why for this release]: Need to keep this working for add-on devs

I can look into this later today!

Tracking for 39+ since this is a recent regression and important for developers on aurora. This would be good to uplift once we have a fix. Also nominating for most poetic bug title of the day.

I've tried to debug this but I don't understand the fundamental behavior: how in the world is the JS execution flow getting into the promise lib when it's paused inside a file like webide.js and you simply step to the next line? It happens even if you are just paused on a meaningless statement; you aren't calling into the lib or anything. What in the world. Panos, how did you make sure the promise lib was invisible to the debugger? All I did was require the promise lib like normal: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/utils/TabSources.js#L7

I think I understand what's going on: * I added this file: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/utils/TabSources.js, which is loaded in in webbrowser.js and child-content.js. It manages a list of sources and is used by the debugger to add sources, but is available to all tools now. * It's definitely being loaded in a different compartment, so the promise lib must not be invisible to the debugger. I don't understand how though because webbrowser.js also imports the promise library, and child-process.js looks like it's loaded by a loader with `invisibleToDebugger = true`. I don't know why introducing it here breaks this. * When we pause at a breakpoint, the promise lib must schedule a function to run immediately when we resume which is why you are thrown into the lib when it resumes This is out of my depth at this point, I've tried several things but couldn't fix it. Panos said he could take a look because he knows a lot more about loaders and has fixed stuff like this before.

Panos, are you working on this? Just checking in since we're tracking this for Firefox 39. Thanks!

I just started looking into this.

WIP, still not working.

Loading the promise library in the same compartment does fix the step-over issue, but it doesn't fix the "too much recursion" issue with step-in. This is not a fix I'm happy with, but I think it points to a more general problem with the dependency order between webbrowser.js, TabSources.js and script.js.

Native promises work too and spare us from the ugliness of the subscript loader. But there is still the recursion problem.

Thanks a ton for your work on this, Panos. That's really helpful to know what fixes the stepping problem. I feel like there's something deeper going on that I'd really like to understand... I will start looking at this again next week. For now, let's go ahead and back out bug 1137384.

This reverts bug 1137384. Haven't done a backout before, I just reverted it with git and generated this patch. Will push to try soon. Or can I just ask a sheriff to back bug 1137384 out?

Both approaches will work. If you land the backout yourself, don't forget to use a message like "Backout cset XXX due to bug YYY".

Hold on with the backout, I have a fix for the second issue as well.

The infinite recursion problem turned out to be one that was already fixed in XPCOMUtils.defineLazyGetter, so I just used the same fix here. The step-into-promise-lib problem is avoided by using native promises in TabSources. I don't know why using the promise module exposed by the loader in this particular module doesn't make it invisible to the debugger. Luckily we don't use any exotic promise features in this file and we are going to move to native promises soon anyway. Asking Nick for an extra pair of eyes on the loader change, since he is the one who originally wrote it. All tests pass locally.

Comment on attachment 8594759 [details] [diff] [review] patch v4 Still learning git-bz...

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c74dbafb4a1b

Comment on attachment 8594759 [details] [diff] [review] patch v4 Review of attachment 8594759 [details] [diff] [review]: ----------------------------------------------------------------- Amazing work!

jryans helped me figure out the root of the problem with require("promise") and this patch contains an alternative fix to that issue. It's not as clean as using native promises, but I'll leave it here for future reference. I believe it would be illuminating, however, to explain the problem in more detail. The devtools loader preloads a set of modules so that we can both use the same code in main and worker threads, as well as declare all of our dependencies only using require(). It creates a sandbox that is shared among all loaded modules (in order to cut down on memory usage in the backend, see bug 1013997) and then makes the preloaded modules available with simple paths to require(). Promise.jsm was preloaded like that, but this means that it was loaded in a compartment not marked as invisibleToDebugger, since the decision to mark a loader sandbox as such happens at loader instantiation time, not Loader.jsm parsing time. Therefore Promise.jsm was always visible to the debugger, but how did chrome debugging work previously? Previously, before extracting it to TabSources.js, the sources code used to be in script.js, which confusingly enough loads 2 promise libraries: deprecated-sync-thenables and the loader-provided promise module. The former is obviously loaded after the sandbox creation, and therefore is invisible to the debugger, but the latter, as explained above, isn't. The reason loading 2 libraries works at all is because we use destructuring assignment for the first one and direct assignment for the second, guaranteeing that a single |promise| symbol is ever defined. Now the critical thing to realize about chrome debugging, is that the only code paths that must only traverse compartments marked as invisibleToDebugger are the ones taken by debugger API event handlers (breakpoints, debugger statement handler, etc.). It turns out that in those paths we were using the deprecated-sync-thenables library before bug 1137384, but switched to the loader-provided library afterwards (and for good reason!). The only consumer of loader-provided promises in script.js is _sendToPrettyPrintWorker, which is only called from a protocol request, not from a debugger API event handler. This illustrates that if we could ensure that the loader-provided promise library was loaded lazily and in an invisible to the debugger compartment, that would be enough to fix the bug. This is what this alternative patch does and I verified that it works as expected. I hope this clarifies things a bit.

Panos that makes perfect sense now! And that promise lib worked in all other tools because "stepping" was never activated. Wow. This is great because it's another step towards removing the sync promise dependency. I guess we could switch straight to native promises, but I wouldn't be surprised if we couldn't do that quite yet, so being able to use `require('promise')` in the debugger soon will probably be important.

(In reply to Panos Astithas [:past] from comment #25) > I hope this clarifies things a bit. Very much so! Thank you for that comment.

When cleaning up the latest iteration of the patch I took out one line that seemed unneeded, but the try server gently reminded me why I had it there in the first place. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a1d959de607

Comment on attachment 8594759 [details] [diff] [review] patch v4 Approval Request Comment [Feature/regressing bug #]: bug 1137384 [User impact if declined]: stepping in browser and add-on debugging will be broken [Describe test coverage new/current, TreeHerder]: manual testing in m-c and nightly [Risks and why]: small risk as the change is contained to devtools code only [String/UUID change made/needed]: none

Comment on attachment 8594759 [details] [diff] [review] patch v4 Now that this has been on m-c for a few days, let's get it into Dev Edition. Aurora+

The step debugger still seems totally broken on debug builds. Sure, the "too much recursion" problem is gone, but it now just crashes :(

(In reply to Marcos Caceres [:marcosc] from comment #34) > The step debugger still seems totally broken on debug builds. Sure, the "too > much recursion" problem is gone, but it now just crashes :( Can you file a new bug with the crash report?

(In reply to Nick Fitzgerald [:fitzgen] from comment #35) > (In reply to Marcos Caceres [:marcosc] from comment #34) > > The step debugger still seems totally broken on debug builds. Sure, the "too > > much recursion" problem is gone, but it now just crashes :( > > Can you file a new bug with the crash report? This is with an opt build right now: https://bugzilla.mozilla.org/show_bug.cgi?id=1158248 I'll see if I have the crash log from my debug build yesterday.