879341 - Do something about the awful CheckForDebugMode call in PushJSContext

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This is causing all sorts of problems, and I think we need to tackle it. I recently discovered that the ActivateDebugger currently invokes xpc->InitClasses using an unpushed cx, and we currently can't push a cx because that would cause infinite recursion. :-(

The IDL docs for setDebugModeWhenPossible have this to say:

>/**
> * When we place the browser in JS debug mode, there can't be any
> * JS on the stack. This is because we currently activate debugMode 
> * on all scripts in the JSRuntime when the debugger is activated.
> * This method will turn debug mode on or off when the context 
> * stack reaches zero length.
> */

This is currently quite far from reality (we currently do this stuff while _pushing_, not while popping). But it gives us some indication of the original intent here, which I think we can use to our advantage. I have a plan.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Move CheckForDebugMode into the main thread event observer. v1

This gets invoked at the very end of nsThread::ProcessNextEvent(). If the cx stack
is empty here, that means the event loop isn't nested - if it it was, we'd at
least have a null cx on the stack from a higher-level call to OnProcessNextEvent
for the main thread observer.

As such, this seems like a much more reasonable place to make debug mode changes.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use an AutoSafeJSContext in CheckForDebugMode like we should. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use an AutoPushJSContext in JSD. v1

We can do this now, since it won't cause infinite recursion.

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 758021 [details] [diff] [review]
Part 1 - Move CheckForDebugMode into the main thread event observer. v1

Review of attachment 758021 [details] [diff] [review]:
-----------------------------------------------------------------

I'm glad to see this fixed :)

Comment 6

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9b95c25646
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c06f0863d73e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a75ac6f7e2

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4e9b95c25646
https://hg.mozilla.org/mozilla-central/rev/c06f0863d73e
https://hg.mozilla.org/mozilla-central/rev/a5a75ac6f7e2