Closed Bug 1442250 Opened 6 years ago Closed 6 years ago

Fix various GeckoSession lifecycle bugs uncovered by tests

Categories

(GeckoView :: General, enhancement, P1)

Product:
GeckoView
Component:
General
Platform:
All
Android
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(5 files)

Bug 1442250 - 1. Add SessionLifecycleTest;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1442250 - 2. Unregister then re-register all listeners on close;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1442250 - 3. Track EventDispatcher attach/detach;
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1442250 - 4. Fix crash when child preloading fails;
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1442250 - 5. Reset native queue early when transferring;
59 bytes, text/x-review-board-request
esawin
: review+
Details 
1. Listeners are not re-registered after closeWindow then openWindow
2. EventDispatcher attach/detach can race
3. Child process preloading can cause NPE
Comment on attachment 8956704 [details]
Bug 1442250 - 1. Add SessionLifecycleTest;

https://reviewboard.mozilla.org/r/225664/#review231636

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt:49
(Diff revision 1)
> +        }
> +        sessionRule.session.reload()
> +        sessionRule.session.waitForPageStop()
> +    }
> +
> +    @Test fun openWindow_allowCallsWhileClosed() {

I was thinking abou this situation the other day and question whether queueing is the right behavior. What happens when we eventually have synchronous calls (for A11y, etc)? Not clear what their behavior should be if we're queuing calls (block forever? throw? return null?).

I think we should consider throwing IllegalStateException or similar if you try to mutate a closed session.
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231640

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:49
(Diff revision 1)
>  public class GeckoSession extends LayerSession
>                            implements Parcelable {
>      private static final String LOGTAG = "GeckoSession";
>      private static final boolean DEBUG = false;
>  
> -    /* package */ enum State implements NativeQueue.State {
> +    private static final int WINDOW_CHANGE_IN_PROGRESS = 0x10;

The state tracking in here is getting difficult to follow. Need some comments on these to describe what's going on.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:423
(Diff revision 1)
> +                if (mNativeQueue == null) {
> +                    // Already closed elsewhere.
> +                    return;
> +                }
>                  mNativeQueue.reset(State.INITIAL);
> +                mNativeQueue = null;

In what circumstances do we now have a null mNativeQueue?
Depends on: 1441971
Comment on attachment 8956704 [details]
Bug 1442250 - 1. Add SessionLifecycleTest;

https://reviewboard.mozilla.org/r/225664/#review231636

> I was thinking abou this situation the other day and question whether queueing is the right behavior. What happens when we eventually have synchronous calls (for A11y, etc)? Not clear what their behavior should be if we're queuing calls (block forever? throw? return null?).
> 
> I think we should consider throwing IllegalStateException or similar if you try to mutate a closed session.

I think in general "setters" should be queued (e.g. delegate setters, loadUri, etc.) and "getters" should return "empty" values (e.g. a11y should return a view without nodes, `session.navigation.location` should return null or empty string, etc).

The reason for queuing setters is I think a lot of consumers will use the pattern

    GeckoSession session = new GeckoSession();
    session.setContentDelegate(new GeckoSession.ContentDelegate() { ... });
    
or

    GeckoSession session = new GeckoSession();
    session.loadUri(mHomePageUri);

and delay opening the session until later (perhaps during `GeckoView.onAttachedToWindow`).
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231640

> In what circumstances do we now have a null mNativeQueue?

When the Window object has been closed and is no longer usable.
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231640

> When the Window object has been closed and is no longer usable.

How is that different from when it's newly created, and we do have a queue?
Comment on attachment 8956704 [details]
Bug 1442250 - 1. Add SessionLifecycleTest;

https://reviewboard.mozilla.org/r/225664/#review231848

Tests!
Attachment #8956704 - Flags: review?(esawin) → review+
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231852

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:317
(Diff revision 2)
>              }
>          };
>  
> +    /* package */ int handlersCount;
> +
> +    private final GeckoSessionHandler<?>[] mAllHandlers = new GeckoSessionHandler<?>[] {

"All" is redundant, maybe mSessionHandlers?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:435
(Diff revision 2)
> +                if (mNativeQueue == null) {
> +                    // Already closed elsewhere.
> +                    return;
> +                }
>                  mNativeQueue.reset(State.INITIAL);
> +                mNativeQueue = null;

Why is setting it to null necessary? I feel like we're abusing it as an additional state (already closed) and forcing null checks everywhere.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:711
(Diff revision 2)
> -    private void onWindowChanged() {
> -        if (mWindow != null) {
> +    private void onWindowChanged(int change) {
> +        if (change == WINDOW_OPEN || change == WINDOW_TRANSFER_IN) {
>              mTextInput.onWindowChanged(mWindow);
>          }
> +
> +        if ((change & ~WINDOW_CHANGE_IN_PROGRESS) == WINDOW_CLOSE) {

I think using bit-wise flags for internal interfaces is painful and error-prone, I vote for enums.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionHandler.java:60
(Diff revision 2)
>          final EventDispatcher eventDispatcher = session.getEventDispatcher();
>          if (mDelegate == delegate) {
>              return;
>          }
>  
> -        if (!mAlwaysListen && mDelegate != null) {
> +        final boolean needUnregister = !mAlwaysListen && mDelegate != null && delegate == null;

Can you please use (a) new(s) line for improved readability?

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionHandler.java:61
(Diff revision 2)
>          if (mDelegate == delegate) {
>              return;
>          }
>  
> -        if (!mAlwaysListen && mDelegate != null) {
> +        final boolean needUnregister = !mAlwaysListen && mDelegate != null && delegate == null;
> +        final boolean needRegister = !mAlwaysListen && mDelegate == null && delegate != null;

Dito.
*Ditto
Comment on attachment 8956706 [details]
Bug 1442250 - 3. Track EventDispatcher attach/detach;

https://reviewboard.mozilla.org/r/225668/#review231860
Attachment #8956706 - Flags: review?(esawin) → review+
Comment on attachment 8956708 [details]
Bug 1442250 - 5. Reset native queue early when transferring;

https://reviewboard.mozilla.org/r/225672/#review231862

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:453
(Diff revision 2)
>          }
>  
>          @WrapForJNI(dispatchTo = "proxy", stubName = "Close")
>          private native void nativeClose();
>  
> -        @WrapForJNI(dispatchTo = "proxy")
> +        public synchronized void transfer(final NativeQueue queue,

Please add a method description (ideally for the related methods, too).

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:494
(Diff revision 2)
>          @WrapForJNI(calledFrom = "gecko")
> -        private synchronized void onReady() {
> -            if (mNativeQueue != null &&
> -                    mNativeQueue.checkAndSetState(State.INITIAL, State.READY)) {
> +        private synchronized void onReady(final @Nullable NativeQueue queue) {
> +            // queue is null the first time onReady is called. queue is non-null
> +            // later when onReady is called again during a transfer.
> +            if ((queue == null && mNativeQueue == null) ||
> +                    (queue != null && mNativeQueue != queue)) {

Wrong indentation.
Attachment #8956708 - Flags: review?(esawin) → review+
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231640

> How is that different from when it's newly created, and we do have a queue?

The Window object is considered open as soon as it's created, because we call Window.open as soon as we create a new Window object. It's after calling Window.close that we consider it closed, and no longer usable, since there's no call to reopen an existing Window object.
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review231852

> Why is setting it to null necessary? I feel like we're abusing it as an additional state (already closed) and forcing null checks everywhere.

There are situations where two sessions can point to the same Window object (e.g. session1.writeToParcel then session2.readFromParcel). In that case, it's possible for Window.close to be called twice, and setting `mNativeQueue` to null signals that the Window object has already been closed.

> I think using bit-wise flags for internal interfaces is painful and error-prone, I vote for enums.

You know I don't like enums :)  I don't think this situation warrants any additional functionality provided by enums (custom methods, etc.). I suspect using enums would result in similar amount of code (so no less painful), and I'm confident that we're good enough programmers to be able to handle one bit-wise flag :)  Now if we have 20 flags, maybe enum would be the answer, but not currently IMO.
(In reply to Jim Chen [:jchen] [:darchons] from comment #22)
> > Why is setting it to null necessary? I feel like we're abusing it as an additional state (already closed) and forcing null checks everywhere.
> 
> There are situations where two sessions can point to the same Window object
> (e.g. session1.writeToParcel then session2.readFromParcel). In that case,
> it's possible for Window.close to be called twice, and setting
> `mNativeQueue` to null signals that the Window object has already been
> closed.

That sounds like an additional state, I think we shouldn't abuse null references to track state but I think adding another explicit state would complicate things needlessly in this case (without enums).

> > I think using bit-wise flags for internal interfaces is painful and error-prone, I vote for enums.
> 
> You know I don't like enums :)  I don't think this situation warrants any
> additional functionality provided by enums (custom methods, etc.). I suspect
> using enums would result in similar amount of code (so no less painful), and
> I'm confident that we're good enough programmers to be able to handle one
> bit-wise flag :)  Now if we have 20 flags, maybe enum would be the answer,
> but not currently IMO.

Personal preferences should not prevent us from writing code that is easy to read and understand. Good code should not require a high level of expertise to understand for something trivial as state management.

> if ((change & ~WINDOW_CHANGE_IN_PROGRESS) == WINDOW_CLOSE)

Without knowing the value of WINDOW_CHANGE_IN_PROGRESS, I find this is not an intuitive condition.

If you refuse to use enums for state keeping, we can at least add an additional boolean to monitor state-in-progress instead of combining it with the actual state.
(In reply to Eugen Sawin [:esawin] from comment #28)
>
> > > I think using bit-wise flags for internal interfaces is painful and error-prone, I vote for enums.
> > 
> > You know I don't like enums :)  I don't think this situation warrants any
> > additional functionality provided by enums (custom methods, etc.). I suspect
> > using enums would result in similar amount of code (so no less painful), and
> > I'm confident that we're good enough programmers to be able to handle one
> > bit-wise flag :)  Now if we have 20 flags, maybe enum would be the answer,
> > but not currently IMO.
> 
> Personal preferences should not prevent us from writing code that is easy to
> read and understand. Good code should not require a high level of expertise
> to understand for something trivial as state management.

I was just joking about not liking enums :)  I do think enums can be useful in situations that warrant the additional functionalities enums provide. For example, members in GeckoSession.State have custom methods like isAtLeast(), so I think that's an acceptable use of enum. However, for use cases where enums and integral constants provide similar functionality, I believe we should prefer constants over enums. That's the idea presented in this link [1] (of course some will disagree with it).

There's also the IntDef annotation [2] that could be a good compromise between enums and constants. It apparently has good IDE support, so I think we should definitely explore that option.

[1] https://developer.android.com/topic/performance/memory.html#Abstractions
[2] https://developer.android.com/reference/android/support/annotation/IntDef.html

> > if ((change & ~WINDOW_CHANGE_IN_PROGRESS) == WINDOW_CLOSE)
> 
> Without knowing the value of WINDOW_CHANGE_IN_PROGRESS, I find this is not
> an intuitive condition.
> 
> If you refuse to use enums for state keeping, we can at least add an
> additional boolean to monitor state-in-progress instead of combining it with
> the actual state.

That's fine with me.
(In reply to Jim Chen [:jchen] [:darchons] from comment #29)
> There's also the IntDef annotation [2] that could be a good compromise
> between enums and constants. It apparently has good IDE support, so I think
> we should definitely explore that option.
> 
> [1] https://developer.android.com/topic/performance/memory.html#Abstractions
> [2]
> https://developer.android.com/reference/android/support/annotation/IntDef.
> html

IntDef looks interesting, combining simplicity of int constants with some type safety.
Comment on attachment 8956705 [details]
Bug 1442250 - 2. Unregister then re-register all listeners on close;

https://reviewboard.mozilla.org/r/225666/#review232402
Attachment #8956705 - Flags: review?(esawin) → review+
Comment on attachment 8956707 [details]
Bug 1442250 - 4. Fix crash when child preloading fails;

https://reviewboard.mozilla.org/r/225670/#review232434
Attachment #8956707 - Flags: review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c11a0b091095
1. Add SessionLifecycleTest; r=esawin
https://hg.mozilla.org/integration/autoland/rev/e534002a0483
2. Unregister then re-register all listeners on close; r=esawin
https://hg.mozilla.org/integration/autoland/rev/f06bf979916a
3. Track EventDispatcher attach/detach; r=esawin
https://hg.mozilla.org/integration/autoland/rev/beefe251970f
4. Fix crash when child preloading fails; r=jchen
https://hg.mozilla.org/integration/autoland/rev/a49ec516aafd
5. Reset native queue early when transferring; r=esawin
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: