872135 - Use the SafeJSContext in nsXPConnect::CheckForDebugMode

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The stuff it does right now seems pretty darn crazy, and it's not clear to me why it's necessary at all. I'll write a patch and see what try has to say.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Use the SafeJSContext in nsXPConnect::CheckForDebugMode. v2

Last patch was infinite recursion fail. This patch passes what limited JSD
test coverage we have, so it probably isn't worth pushing to try again. Flagging
gabor for review.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 750781 [details] [diff] [review]
Use the SafeJSContext in nsXPConnect::CheckForDebugMode. v2

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

-    if (!NS_IsMainThread()) {
-        return;
-    }
+    if (!NS_IsMainThread())
+        MOZ_CRASH();

missing {}

Comment 4

[Gabor Krizsanits (INACTIVE)]

+    if (!JS_SetDebugModeForAllCompartments(unpushedCx, gDesiredDebugMode))
         goto fail;

{}

Comment 5

[Andrew McCreight [:mccr8]]

XPConnect uses JS style which doesn't always require braces.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> XPConnect uses JS style which doesn't always require braces.

That's totally true. I would argue that mixing 2 styles in one function is wrong... but I'll let Bobby decide what he prefers.

Comment 7

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=6869a88c9461

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6869a88c9461

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 750781 [details] [diff] [review]
Use the SafeJSContext in nsXPConnect::CheckForDebugMode. v2

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +21,5 @@
>  #include "nsNullPrincipal.h"
>  #include "nsIURI.h"
>  #include "nsJSEnvironment.h"
>  #include "nsThreadUtils.h"
> +#include "nsContentUtils.h"

Out of interest, why?

Comment 10

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #9)
> Out of interest, why?

Must have been an artifact of the earlier iteration of the patch. Seems to build fine without it. Pushed a fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd114424cc86

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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