Closed Bug 1172567 Opened 6 years ago Closed 6 years ago

Use GetLongField for JNI pointers

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox41 affected, firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: jchen, Assigned: droeh)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1032460 fixed the call to get the fieldID to use long types, but didn't fix the actual GetIntField call to get the value. It should be changed to GetLongField for newer Android versions.
I can confirm that with the patch proposed on IRC (Did I get it from you, jchen?) I can now run my own builds on a Nexus 5 running M preview.

> diff --git a/widget/android/AndroidBridge.cpp b/widget/android/AndroidBridge.cpp
> --- a/widget/android/AndroidBridge.cpp
> +++ b/widget/android/AndroidBridge.cpp
> @@ -736,17 +736,17 @@ AndroidBridge::CreateEGLSurfaceForCompos
>  
>      auto eglSurface = mGLControllerObj->CreateEGLSurfaceForCompositorWrapper();
>      if (!eglSurface) {
>          return nullptr;
>      }
>  
>      JNIEnv* const env = GetJNIForThread(); // called on the compositor thread
>      return reinterpret_cast<EGLSurface>(
> -            env->GetIntField(eglSurface.Get(), jEGLSurfacePointerField));
> +            env->GetLongField(eglSurface.Get(), jEGLSurfacePointerField));
>  }
>  
>  bool
>  AndroidBridge::GetStaticIntField(const char *className, const char *fieldName, int32_t* aInt, JNIEnv* jEnv /* = nullptr */)
>  {
>      ALOG_BRIDGE("AndroidBridge::GetStaticIntField %s", fieldName);
>  
>      if (!jEnv) {

Before patching Fennec was crashing with the following error:
> 06-17 15:49:10.628  24965-25373/org.mozilla.fennec_sebastian A/art﹕ art/runtime/java_vm_ext.cc:409] JNI DETECTED ERROR IN APPLICATION: attempt to access field long com.google.android.gles_jni.EGLSurfaceImpl.mEGLSurface of type long with the wrong type int: 0xb0a1d2e0
> 06-17 15:49:10.629  24965-25373/org.mozilla.fennec_sebastian A/art﹕ art/runtime/java_vm_ext.cc:409]     in call to GetIntField
> [..]
Assignee: nobody → droeh
There are now two calls to GetIntField() in AndroidBridge.cpp that I locally patch whenever I need to test something on M.
Attached patch Proposed patchSplinter Review
Checks API version to determine whether to use GetIntField or GetLongField for jEGLSurfacePointerField.
Attachment #8632125 - Flags: review?(snorp)
Comment on attachment 8632125 [details] [diff] [review]
Proposed patch

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

lgtm
Attachment #8632125 - Flags: review?(snorp) → review+
(In reply to :Sebastian Kaspari from comment #2)
> There are now two calls to GetIntField() in AndroidBridge.cpp that I locally
> patch whenever I need to test something on M.

Where's the other one? I only see one field of type "J" (long)
Attached patch m-startup.patch (obsolete) — Splinter Review
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #5)
> Where's the other one? I only see one field of type "J" (long)

I attached the patch I have in mq for whenever I want to deploy to my phone running M. Without the patch fennec crashes after startup.
I'm confused, there's two patches here and the second one doesn't look like it fully subsumes the first one. Please set one as obsolete.
Keywords: checkin-needed
Whoops, sorry for the confusion.

NI snorp: Just have a look at the second patch: Does the second call needs to be fixed too or is this part of another bug?
Flags: needinfo?(snorp)
(In reply to :Sebastian Kaspari from comment #8)
> Whoops, sorry for the confusion.
> 
> NI snorp: Just have a look at the second patch: Does the second call needs
> to be fixed too or is this part of another bug?

The jSurfacePointerField is not used on M right now, and it is an integer (not a long) in the places we do use it, so I don't think we want that patch right now.
Flags: needinfo?(snorp)
Attachment #8632719 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/67b2c40bbaef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.