Closed Bug 879341 Opened 8 years ago Closed 8 years ago

Do something about the awful CheckForDebugMode call in PushJSContext

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files)

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.
Summary: Do something about the awful CheckForDebugMode call in PushJSContext. → Do something about the awful CheckForDebugMode call in PushJSContext
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.
Attachment #758021 - Flags: review?(gkrizsanits)
We can do this now, since it won't cause infinite recursion.
Attachment #758023 - Flags: review?(gkrizsanits)
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 :)
Attachment #758021 - Flags: review?(gkrizsanits) → review+
Attachment #758022 - Flags: review?(gkrizsanits) → review+
Attachment #758023 - Flags: review?(gkrizsanits) → review+
You need to log in before you can comment on or make changes to this bug.