Closed Bug 901712 Opened 6 years ago Closed 6 years ago

black boxing doesn't work with source maps

Categories

(DevTools :: Debugger, defect, P2)

25 Branch
x86
macOS
defect

Tracking

(firefox24 unaffected, firefox25 fixed, firefox26 fixed)

RESOLVED FIXED
Firefox 26
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

Black boxing + source maps *should* work, but we should test it explicitly.
Assignee: nobody → nfitzgerald
And it actually doesn't work.
Summary: add test(s) for source maps + black boxing → black boxing doesn't work with source maps
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
Attachment #787801 - Flags: review?(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.
Attachment #787801 - Flags: review?(dcamp) → review-
Attached patch v2 (obsolete) — Splinter Review
Not completely sure what the best way to test this new nesting behavior would be.

https://tbpl.mozilla.org/?tree=Try&rev=60948a1df07c
Attachment #787801 - Attachment is obsolete: true
Attachment #787841 - Flags: review?(dcamp)
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
Attachment #787841 - Flags: review?(dcamp)
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #787841 - Attachment is obsolete: true
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
Attachment #788409 - Attachment is obsolete: true
Attachment #789837 - Flags: review?(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.
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
Priority: -- → P2
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.
Attachment #789837 - Flags: review?(dcamp) → review+
patch that landed
Attachment #789837 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a9123b9b0a83
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
(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.
Attachment #792424 - Flags: approval-mozilla-aurora?
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.
Attachment #792424 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 878307
Fix landed with tests so assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.
Flags: in-testsuite+
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.