Closed Bug 1541485 Opened 10 months ago Closed 8 months ago

Stop using AutoJSContext in JavaCallbackDelegate::Call

Categories

(Firefox for Android :: General, task)

task
Not set

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

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

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?

Flags: needinfo?(snorp)

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.

Type: defect → task

(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.

Flags: needinfo?(snorp)

What about the question in comment 3?

Flags: needinfo?(snorp)

(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.

Flags: needinfo?(snorp)

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?

Flags: needinfo?(snorp)

(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.

Flags: needinfo?(snorp)

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

Flags: needinfo?(jimnchen+bmo)

(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).

Flags: needinfo?(jimnchen+bmo)

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?

Flags: needinfo?(jimnchen+bmo)

(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

Flags: needinfo?(jimnchen+bmo)

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.

Flags: needinfo?(jimnchen+bmo)

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

Flags: needinfo?(jimnchen+bmo)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abaf133a4055
Stop using AutoJSContext in android widget code.  r=snorp
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee: nobody → bzbarsky
You need to log in before you can comment on or make changes to this bug.