1541485 - Stop using AutoJSContext in JavaCallbackDelegate::Call

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, task)

Description

[Boris Zbarsky [:bzbarsky]]

It's not clear to me where the JS value this thing is getting comes from and whether it actually expects to be called with JS on the stack or not. If it does, the JSContext should be passed in. If not, it's not clear to me how what it's doing makes any sense.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Should nsIAndroidBridge.idl just have implicit_jscontext on onSuccess/onError and then thread the JSContext through?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So I tried that, and found that mobile/android/components/geckoview/GeckoViewHistory.cpp has implementations of nsIAndroidEventCallback that do just use AutoJSAPI on the stack, inited with mGlobalObject. But what guarantees that that's same-compartment with the incoming aData values?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Also, I don't understand this bit:

 NS_ENSURE_SUCCESS(rv, JS_IsExceptionPending(cx) ? NS_OK : rv);

Why is it returning NS_OK if there's an exception pending? That seems pretty broken to me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix for this, module the issue in comment 3

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

Should nsIAndroidBridge.idl just have implicit_jscontext on onSuccess/onError and then thread the JSContext through?

This seems reasonable to me. I didn't write this and can't remember the rationale for why we're not already doing it this way.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What about the question in comment 3?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #6)

What about the question in comment 3?

Whoops sorry about the late reply. Yeah, that looks wrong to me. BoxData intentionally returns an error in the case where there is an exception pending, and then we reverse that here in the NS_ENSURE_SUCCESS body? Weird. NS_ENSURE_SUCCESS(rv, rv) seems correct.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, one more question now that I got back to this... In https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/widget/android/EventDispatcher.cpp#593 why is the AutoNoJSAPI there? It really matters in terms of how this is going to end up working, since threading a JSContext past that AutoNoJSAPI is not a reasonable thing to do.

In general, it would be good to understand the exact model here; typically use of AutoNoJSAPI means either you're about to spin the event loop or you're doing something is security-sensitive, and in the latter case it should come with comments explaining what's going on. Is this case an instance of the former or of the latter, or neither?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

OK, one more question now that I got back to this... In https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/widget/android/EventDispatcher.cpp#593 why is the AutoNoJSAPI there? It really matters in terms of how this is going to end up working, since threading a JSContext past that AutoNoJSAPI is not a reasonable thing to do.

In general, it would be good to understand the exact model here; typically use of AutoNoJSAPI means either you're about to spin the event loop or you're doing something is security-sensitive, and in the latter case it should come with comments explaining what's going on. Is this case an instance of the former or of the latter, or neither?

We're calling out to a Java method, so I think the answer in this case is neither. Maybe we could try NI jchen? I don't remember any specific reason we have that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We could try, but jchen's account is explicitly marked inactive....

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

Also, I don't understand this bit:

 NS_ENSURE_SUCCESS(rv, JS_IsExceptionPending(cx) ? NS_OK : rv);

Why is it returning NS_OK if there's an exception pending? That seems pretty broken to me.

I think that's because if we returned an error code here, for some reason xpconnect would override the pending JS exception with a generic xpconnect exception, and we didn't want that.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

OK, one more question now that I got back to this... In https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/widget/android/EventDispatcher.cpp#593 why is the AutoNoJSAPI there? It really matters in terms of how this is going to end up working, since threading a JSContext past that AutoNoJSAPI is not a reasonable thing to do.

In general, it would be good to understand the exact model here; typically use of AutoNoJSAPI means either you're about to spin the event loop or you're doing something is security-sensitive, and in the latter case it should come with comments explaining what's going on. Is this case an instance of the former or of the latter, or neither?

IIRC it was a security precaution because it didn't make sense for the callback to have script access, and we couldn't guarantee the callback wouldn't do something like spinning the event loop (though in practice I don't think that happens).

Comment 12

[Boris Zbarsky [:bzbarsky]]

for some reason xpconnect would override the pending JS exception with a generic xpconnect exception, and we didn't want that.

Ah. Was there a test that exercised that, by any chance?

because it didn't make sense for the callback to have script access

But the callback is being handed JS values, no? How is it expected to work with those?

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #12)

for some reason xpconnect would override the pending JS exception with a generic xpconnect exception, and we didn't want that.

Ah. Was there a test that exercised that, by any chance?

I don't think there was one

because it didn't make sense for the callback to have script access

But the callback is being handed JS values, no? How is it expected to work with those?

JavaCallbackDelegate::Call converts the JS objects to Java objects, and the JS objects are not used after the conversion

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, I see. My apologies. I had linked to the wrong AutoNoJSAPI above, so we've been talking past each other! The one that's confusing me is the one at https://searchfox.org/mozilla-central/rev/99a2a5a955960b0e58ceade1db1f7652d9db4ba1/widget/android/EventDispatcher.cpp#640

I agree that for the JavaCallbackDelegate case it's pretty clear what the story is and AutoNoJSAPI makes sense there. The question is about NativeCallbackDelegateSupport::Call.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Ah we can probably remove that AutoNoJSAPI? Now looking at it, it doesn't make a lot of sense

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1541485. Stop using AutoJSContext in android widget code. r=snorp

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abaf133a4055
Stop using AutoJSContext in android widget code.  r=snorp

Comment 18

[Andreea Pavel [:apavel]]

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