1827386 - NativeWeakPtr::Detach should return Promise to wait for detached completely

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[Makoto Kato [:m_kato]]

When debugging bug 1778039, I found the following.

void GeckoViewSupport::Transfer(const GeckoSession::Window::LocalRef& inst,
                                jni::Object::Param aQueue,
                                jni::Object::Param aCompositor,
                                jni::Object::Param aDispatcher,
                                jni::Object::Param aSessionAccessibility,
                                jni::Object::Param aInitData) {
...
  mWindow->mSessionAccessibility.Detach();
  if (aSessionAccessibility) {
    AttachAccessibility(inst, aSessionAccessibility);
  }

When calling Detach is done, We don't ensure that JNI object isn't released. so If re-attaching same JNI object, disposer by Detach can run after AttachAccessibility is done.

Because disposer have to run in Gecko main thread, but OnWeakNonIntrusiveDetach creates task to call Java method in Android UI thread. It means that some JNI implementation only creates disposing tasks by Detach, then it doesn't dispose the object yet.

So GeckoViewSupport::Transfer has dangerous logic for SessionAccessibility.

I would like to return MozPromise by NativeWeakPtr::Detach to resolve this issue. I guess that this issue is one of crash reason of bug 1778039, but I am not sure that it is all.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1827386 - Part 1. NativeWeakPtr::Detach returns MozPromise to detect whether dispoer is finished. r=#geckoview-reviewers

Actually, NativeWeakPtr::Detach may not release JNI ojbect immediately because
it depends on JNI object's OnWeakNonIntrusiveDetachi implementation.
SessionAccessibility's OnWeakNonIntrusiveDetach implementation uses the
runnable to run on Android UI thread then disposer runs on main thread, so
if Detach is finished, JNI object isn't detached yet.

If calling NativeWeakPtrHolder::Attach immediately with same/recycled Java
object after NettiveWeakPtr::Detach, it is possible to run disposer for JNI
object by OnWeakNonIntrusiveDetach after Attach is finished. So it may
release newer attached object unfortunately.

So I would like to add a way to waiting for detach JNI object using
MozPromise.

Also, MozPromise.h includes Natives.h header (for GeckoResult support).
So I cannot modify inline method to use MozPromise due to recursive. So I
split implementation with NativesInlines.h as workaround.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1827386 - Part 2. Don't re-attach SessionAccessibility until Detach is completely finished. r=#geckoview-reviewers

Although GeckoViewSupport::Transfer will re-attach SessionAccessibility,
it doesn't wait for previous SessionAccessibility is disposed completely
since SessionAccessibility::OnWeakNonIntrusiveDetach makes tasks only.

When worst case,

1. Create a task for calling SessionAccessilibity::SetAttached(false) on Android UI thread.
2. Create a task for calling SessionAccessilibity::SetAttached(true) on Android UI thread.
3. Attach new JNI object to SeessionAccessibility
4. Run 1's task on Android UI thread, then create a task for disposing JNI object on Gecko thread.
5. Run 2's task on Android UI thread.
6. Run 4's task on Gecko thread. JNI object is detached incorrectly if JNI object is recycled.

If reattaching same JNI object, we detach new object unfortunately. Since I add
MozPromise By Part 1, we can wait for it to use this API.

Depends on D175335

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a41bdd815729
Part 1. NativeWeakPtr::Detach returns MozPromise to detect whether dispoer is finished. r=geckoview-reviewers,owlish
https://hg.mozilla.org/integration/autoland/rev/33cdbcb6cf0a
Part 2. Don't re-attach SessionAccessibility until Detach is completely finished. r=geckoview-reviewers,owlish

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a41bdd815729
https://hg.mozilla.org/mozilla-central/rev/33cdbcb6cf0a