901712 - black boxing doesn't work with source maps

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

And it actually doesn't work.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-901712-sourcemaps-and-black-boxing.patch

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 3

[Dave Camp (:dcamp)]

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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v2

Not completely sure what the best way to test this new nesting behavior would be.

https://tbpl.mozilla.org/?tree=Try&rev=60948a1df07c

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-901712-sourcemaps-and-black-boxing.patch

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 8

[Dave Camp (:dcamp)]

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.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 11

[Dave Camp (:dcamp)]

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.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-901712-sourcemaps-and-black-boxing.patch

patch that landed

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a9123b9b0a83

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: source-maps-and-black-boxing-aurora.patch

(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 16

[Alex Keybl [:akeybl]]

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.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2be44fda7552

Comment 18

[u279076]

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