Black boxing + source maps *should* work, but we should test it explicitly.

And it actually doesn't work.

I think we should also uplift this fix to 25. Would appreciate some guidance on that process, as I haven't done it yet. https://tbpl.mozilla.org/?tree=Try&rev=c29c07dbd197

Comment on attachment 787801 [details] [diff] [review] bug-901712-sourcemaps-and-black-boxing.patch Review of attachment 787801 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/script.js @@ +758,5 @@ > + new Error("ThreadActor.prototype.synchronize can't exit the event" > + + " loop because someone else started one.")); > + return; > + } > + DebuggerServer.xpcInspector.exitNestedEventLoop(); If another tab is paused, this will never resume. We need some way to say "exit this particular nestRequestor when it's the top requestor". So you could add some exitNest() hooks that recheck lastNestRequestor and exit if needed. But it might be nicer if nsIJSInspector's exitNestedEventLoop took the nestRequestor and then somehow stored that if it wasn't the top. But nsIJSInspector doesn't really know/care about the structor of lastNestRequestor, so it's probably best to just add an existNest hook. But it needs to be a global exitNest hook, not per-thread or per-connection or anything.

Not completely sure what the best way to test this new nesting behavior would be. https://tbpl.mozilla.org/?tree=Try&rev=60948a1df07c

Posting an IRC convo with dcamp for posterity: 18:22 <@dcamp> fitzgen: instead of passing exitWhenOnTop as a bool, maybe pass it as an object that contains a bool 18:22 <@dcamp> initially true 18:22 < fitzgen> oh I see: spin to wait for "foo", someone else spins, someone else exits, we auto exit "foo", "foo" hasn't happened yet 18:22 < fitzgen> right? ^ 18:22 <@dcamp> then toggle that bool to false once "foo" happens 18:22 <@dcamp> right 18:22 <@dcamp> exactly 18:23 <@dcamp> actually, y'know, _nest could just optionally take a requestor 18:23 <@dcamp> because that conn === conn check could actually be wrong too; 18:23 <@dcamp> because the second pause could be on this same pause. 18:23 < fitzgen> right, we need to be checking requestors because before conns == requestors 18:23 < fitzgen> but thats not true anymore 18:24 <@dcamp> so basically, this usage of nest should have a completely different requestor 18:24 <@dcamp> that can uniquely identify this nesting 18:24 <@dcamp> maybe conn+a serial number+a "finished" property that you'll toggle when you're done? 18:25 < fitzgen> perhaps, I feel like there has to be a simpler way 18:25 < fitzgen> maybe we can wrap this stuff in something nicer 18:25 < fitzgen> s/simpler/nicer 18:25 <@dcamp> quite possibly, yeah 18:26 < fitzgen> I'm gonna call it a night, come back to this tomorrow 18:26 <@dcamp> I could imagine a NestedEventLoop obj that hides xpcInspector entirely 18:26 <@dcamp> sounds good 18:26 < fitzgen> dcamp: yeah, we are hiding entering event loops, but not exiting 18:26 <@dcamp> right 18:26 < fitzgen> we really should just abstract over the whole thing

Adds NestedEventLoopStack and NestedEventLoop objects and a test_nesting.js. Test is failing and I'm not 100% sure if it is bad test logic, or bad implementation.

Wow, my test was failing because the xpcshell runner had a strict error. Why was there a strict error? Because apparently you can't pass in a message to your assertions, that argument is for a stack object (WTF). I've been doing it wrong for so long. https://tbpl.mozilla.org/?tree=Try&rev=a639733d0b0a

Comment on attachment 789837 [details] [diff] [review] bug-901712-sourcemaps-and-black-boxing.patch Review of attachment 789837 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/tests/unit/head_dbg.js @@ +348,5 @@ > StubTransport.prototype.close = function () {}; > + > +function executeSoon(aFunc) { > + Services.tm.mainThread.dispatch({ > + run: function() { Can you make sure this try/catch/dumps an error? I've seen mainThread.dispatch swallow exceptions.

Ok the reason this is crashing and burning is the damn strict error I split out and fixed in bug 904196

New try push, now that the other bug is fixed: https://tbpl.mozilla.org/?tree=Try&rev=3de2835dd927

Comment on attachment 789837 [details] [diff] [review] bug-901712-sourcemaps-and-black-boxing.patch Review of attachment 789837 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/script.js @@ +901,5 @@ > + let eventLoop; > + let returnVal; > + > + aPromise > + .then(function (resolvedVal) { arrow function? @@ +905,5 @@ > + .then(function (resolvedVal) { > + needNest = false; > + returnVal = resolvedVal; > + }) > + .then(null, (aError) => { style nit, one function uses resolvedVal and the the other uses aError.

patch that landed

(This is just the existing patch rebased onto Aurora.) [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 877682, although the UI to expose black boxing to users was implemented in bug 877686. User impact if declined: Our new black boxing feature does not work if the user is using source maps. Testing completed (on m-c, etc.): This patch has been in m-c for a few days, nothing worrying coming back. Here is a try push on aurora: https://tbpl.mozilla.org/?tree=Try&rev=c2e8300cd379 Risk to taking this patch (and alternatives if risky): This patch refactors the way nested event loops work in the debugger to allow us to pause and resume. While all of our tests pass (including new ones intro'd by this patch), and debugging pages in Nightly wfm, it is possible that this patch introduced a subtle bug we haven't caught yet related to pausing and resuming the debugger. String or IDL/UUID changes made by this patch: None.

Comment on attachment 792424 [details] [diff] [review] source-maps-and-black-boxing-aurora.patch We'll call 24 unaffected since the feature isn't user visible and therefore is basically disabled. Approving for Aurora 25.

Fix landed with tests so assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.