Due to current startup mechanics, calling GeckoView.loadUri before attaching to a window results in a crash and dispatching events before the listeners have registered would discard those events. We need to introduce queuing mechanics in the event dispatcher and translate some critical paths based on native calls (GeckoView.loadUri) to use generic events to avoid startup race conditions.

With this we introduce GeckoView state, initially on INITIAL and set to READY when "chrome-document-loaded" is observed. EventDispatcher starts in "paused" state, in which it will queue all events. Resuming the dispatcher will dispatched all previously queued events. I've tried to combine EventDispatcher.mIsDispatching with .mAttachedToGecko to avoid additional state, but it seems like we might introduce other races that way. I also don't like having the additional CDLO class in nsWindow.cpp, I'm sure there should be a better way to observe the event without making nsWindow itself an nsIObserver.

This is how loadUri can work using the EventDispatcher. We have to keep the native call-based version around for Fennec.

Comment on attachment 8842549 [details] [diff] [review] 0001-Bug-1343613-1.0-Add-event-queuing-in-EventDispatcher.patch Review of attachment 8842549 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly ok to me, with some nits, but I think some cleanup around the decision to queue could help. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java @@ +56,5 @@ > private final Map<String, List<BundleEventListener>> mBackgroundThreadListeners = > new HashMap<String, List<BundleEventListener>>(DEFAULT_BACKGROUND_EVENTS_COUNT); > > + private boolean mAttachedToGecko = false; > + private boolean mIsDispatching = false; You don't need to explicitly initialize to false, it's understood they default to false. @@ +63,5 @@ > + mIsDispatching = true; > + for (DispatchArgs args: mEventQueue) { > + dispatch(args.mType, args.mMessage, args.mCallback); > + } > + mEventQueue.clear(); Problem I see here is that dispatch() is not guaranteed to actually deliver the message. It could end up queueing it again if mAttachedToGecko is false, and then you'd clear all of the queued messages here. Possibly solution would be to add a 'boolean shouldQueueMessages()' which compares both mAttachedToGecko and mIsDispatching, as we do in dispatch(). In resumeDispatching(), we only flush the queue if shouldQueueMessages() results in the affirmative. We would need to call resumeDispatching() from setAttachedToGecko() as well. Most of this seems academic, though, since we currently would never call resumeDispatching() while not attached to gecko. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java @@ +69,5 @@ > + if (!mState.isAtLeast(State.READY) && > + newState.isAtLeast(State.READY)) { > + eventDispatcher.resumeDispatching(); > + } else if (mState.isAtLeast(State.READY) && > + !newState.isAtLeast(State.READY)) { We don't really expect this to occur, right? I think it would be better to log/throw/assert/something if we go back to the initial state.

I've reverted some of the dispatch logic to dispatch to threads instead of queuing and returning when not attached. I've also found that by having EventDispatcher paused by default initially, we would break some use cases (Fennec depends on those). So I've made it optional at construction. I will address the rest of the comments after Jim's review.

Comment on attachment 8842602 [details] [diff] [review] 0001-Bug-1343613-1.1-Add-event-queuing-in-EventDispatcher.patch Review of attachment 8842602 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java @@ +67,5 @@ > + } > + mEventQueue.clear(); > + } > + > + public synchronized void pauseDispatching() { Do we need this? When would we pause dispatching? @@ +71,5 @@ > + public synchronized void pauseDispatching() { > + mIsDispatching = false; > + } > + > + private class DispatchArgs { Should be static class. @@ +72,5 @@ > + mIsDispatching = false; > + } > + > + private class DispatchArgs { > + public final String mType; No prefix for public members. @@ +84,5 @@ > + this.mCallback = callback; > + } > + } > + > + private final ArrayList<DispatchArgs> mEventQueue = new ArrayList<>(); I still think we should reuse the existing queue in GeckoThread, and teach GeckoThread to check the GeckoView state. That way loadUri is covered as well. We are likely to have native calls in the future that need queuing, e.g. if we add a method for getting a thumbnail. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java @@ +41,4 @@ > private static final String DEFAULT_SHARED_PREFERENCES_FILE = "GeckoView"; > private static final String LOGTAG = "GeckoView"; > > + private static final boolean DEBUG = true; Should be false @@ +64,5 @@ > + > + private volatile State mState = State.INITIAL; > + > + @WrapForJNI(calledFrom = "gecko") > + private synchronized void setState(final State newState) { Don't need 'synchronized' ::: widget/android/nsWindow.cpp @@ +293,4 @@ > nsWindow* operator->() const { return operator nsWindow*(); } > }; > > +class ChromeDocumentLoadedObserver final : public nsIObserver Can we reuse the observer in nsAppShell? Seems pointless to have a separate observer for each nsWindow. @@ +322,5 @@ > + NS_IMETHODIMP > + Observe(nsISupports* aSubject, const char* aTopic, > + const char16_t* aData) override > + { > + if (!strcmp(aTopic, kEventTopic.get())) { We should check that the new window is the one we care about. If two windows are created at the same time, both nsWindows will get the "chrome-document-loaded" notification for whichever window is loaded first. In that case both nsWindows will be set to ready state, even though only the first window is actually ready. @@ +336,5 @@ > + } > +}; > + > +const nsLiteralCString ChromeDocumentLoadedObserver::kEventTopic > + = NS_LITERAL_CSTRING("chrome-document-loaded"); Use string literal (i.e. const char kEventTopic[] = "...";) instead of nsLiteralCString.

What do you think about something like this? With this, every class can provide its own state by implementing IState and queue events based on that. (Will address nsWindow comments later)

Comment on attachment 8842978 [details] [diff] [review] 0001-Bug-1343613-1.2-Add-event-GeckoView-queuing-in-Event.patch Review of attachment 8842978 [details] [diff] [review]: ----------------------------------------------------------------- I'm okay with this approach once `stateObj` is sorted out. Also, I think you want to split all the queuing stuff into a NativeQueue class or something, which will have State and StateHolder/StateGetter as subinterfaces. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java @@ +237,5 @@ > + final boolean isViewReady = > + mGeckoView == null || > + mGeckoView.getState().isAtLeast(GeckoView.State.READY); > + > + if (isViewReady && mAttachedToGecko && hasGeckoListener(type)) { The `mAttachedToGecko` check shouldn't be necessary with the view ready check. @@ +287,5 @@ > return true; > } > > + if (mGeckoView != null && > + !mGeckoView.getState().isAtLeast(GeckoView.State.READY)) { Merge this into the next if block ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java @@ +386,5 @@ > + * @param methodName Name of the instance method. > + * @param args Args to call the instance method with; to specify a parameter type, > + * pass in a Class instance first, followed by the value. > + */ > + public static void queueNativeCallUntil(IState stateObj, final IState state, final Object obj, Using a `stateObj` here to hold the current state is racy, because the real state can change after `stateObj` is set but before we had a chance to compare it in `queueNativeCallLocked`. I think you need to pass in a "state getter" or "state holder" object instead. @@ +413,4 @@ > // We already handled the call. > continue; > } > + if (state.getClass() != call.state.getClass() || I think this should be part of the is/isAtLeast implementation.

I've split the native queuing refactoring out into patch 1 and will handle the EventDispatcher queuing in patch 2. I don't think we need a StateHolder/StateGetter class to prevent a race since queuing and flushing is synchronized - we shouldn't miss any state transitions.

Pass IStateHolder instead of State for current state check.

(In reply to Jim Chen [:jchen] [:darchons] from comment #7) > The `mAttachedToGecko` check shouldn't be necessary with the view ready > check. EventDispatcher.mGeckoView is optional, with it being null (Fennec) isViewReady defaults to true, so I think we still need that check. > Merge this into the next if block Merging the two blocks would require to conditionally set (and therefore expose in GeckoThread) the state holder and state. We can do that, but in the end the result was more confusing than the two somewhat redundant blocks. > Can we reuse the observer in nsAppShell? Seems pointless to have a separate > observer for each nsWindow. I agree, although I don't know we could (easily) do that. Do you have an idea?

(In reply to Eugen Sawin [:esawin] from comment #10) > Created attachment 8844227 [details] [diff] [review] > 0002-Bug-1343613-2.1-Add-GeckoView-event-queuing-in-Event.patch > > (In reply to Jim Chen [:jchen] [:darchons] from comment #7) > > The `mAttachedToGecko` check shouldn't be necessary with the view ready > > check. > > EventDispatcher.mGeckoView is optional, with it being null (Fennec) > isViewReady defaults to true, so I think we still need that check. So instead of a GeckoView, EventDispatcher can now keep an IStateHolder, which is GeckoThread's state holder for the global dispatcher, and GeckoView for the per-window dispatcher. Then here you can use the state holder to check for readiness instead of mAttachedToGecko. Same functionality but I think that makes a better design, because there's less GeckoView-specific stuff in EventDispatcher. > > Merge this into the next if block > > Merging the two blocks would require to conditionally set (and therefore > expose in GeckoThread) the state holder and state. We can do that, but in > the end the result was more confusing than the two somewhat redundant blocks. Having a state holder in EventDispatcher also lets you merge the two if blocks (granted you have to keep another "ready state" member in EventDispatcher). Your call. > > Can we reuse the observer in nsAppShell? Seems pointless to have a separate > > observer for each nsWindow. > > I agree, although I don't know we could (easily) do that. Do you have an > idea? Similar to what you did in ChromeDocumentLoadedObserver::Observe, basically QI aSubject to nsIDocument, then call WidgetUtils::DOMWindowToWidget(doc->GetWindow()) to get the widget. Finally cast the widget to an nsWindow. Make sure nsWindow has an mGeckoViewSupport, and then set the GeckoView state from there. I think you'll have to add a GeckoView global ref in GeckoViewSupport.

Addressed comments. Currently, we allow re-setting a GeckoView state (e.g., READY->READY) which translates to no action, we might want to guard against it in future (checkAndSetState).

Addressed comments.

Comment on attachment 8844553 [details] [diff] [review] 0001-Bug-1343613-1.5-Refactor-native-call-queuing-out-of-.patch Review of attachment 8844553 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java @@ +71,5 @@ > } > > + @Override > + public boolean isAtLeast(final NativeQueue.IState other) { > + if (other.getClass() != State.class) { Use instanceof @@ +81,1 @@ > public boolean isAtLeast(final State other) { Remove this and just go with isAtLeast(NativeQueue.IState) @@ +92,4 @@ > } > } > > + public static class StateHolder implements NativeQueue.IStateHolder { private static class @@ +104,5 @@ > } > } > > + public static StateHolder getStateHolder() { > + return new StateHolder(); Please use a single StateHolder instance. @@ +110,5 @@ > + > + public static final State MIN_STATE = State.INITIAL; > + public static final State MAX_STATE = State.EXITED; > + > + private static volatile State sState = State.INITIAL; /* package */ @@ +508,5 @@ > @WrapForJNI(calledFrom = "gecko") > private static void setState(final State newState) { > ThreadUtils.assertOnGeckoThread(); > + NativeQueue.flushQueued(newState); > + sState = newState; We should set new states inside NativeQueue's lock. Otherwise we might queue a call after flushing but before setting the new state, and end up missing the state transition. I think you can add StateHolder.setState() and have flushQueued call it inside the lock. Or you can add NativeQueue.getLock(), and synchronize on that when setting new states. @@ +588,5 @@ > + * Queue a call to the given static method until Gecko is in the RUNNING state. > + */ > + public static void queueNativeCall(final Class<?> cls, final String methodName, > + final Object... args) { > + NativeQueue.queueUntil(new StateHolder(), State.RUNNING, cls, methodName, args); Use getStateHolder() ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java @@ +59,5 @@ > + } > + > + @Override > + public boolean isAtLeast(final NativeQueue.IState other) { > + if (other.getClass() != State.class) { Use instanceof @@ +65,5 @@ > + } > + return isAtLeast((State) other); > + } > + > + public boolean isAtLeast(final State other) { Remove this and go with isAtLeast(NativeQueue.IState) @@ +75,5 @@ > + } > + > + } > + > + public class StateHolder implements NativeQueue.IStateHolder { private class @@ +92,5 @@ > + > + @WrapForJNI(calledFrom = "gecko") > + private void setState(final State newState) { > + if (!mState.isAtLeast(State.READY) && > + newState.isAtLeast(State.READY)) { Drop these checks, and set new state inside NativeQueue's lock. @@ +106,5 @@ > + return mState; > + } > + > + /* package */ StateHolder getStateHolder() { > + return new StateHolder(); Use single StateHolder instance ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/NativeQueue.java @@ +12,5 @@ > +import java.lang.reflect.Modifier; > +import java.util.ArrayList; > + > +public class NativeQueue { > + private static final String LOGTAG = "NativeQueue"; LOGTAG = "GeckoNativeQueue" @@ +14,5 @@ > + > +public class NativeQueue { > + private static final String LOGTAG = "NativeQueue"; > + > + public interface IState { Consider naming it State, Java convention doesn't use I prefix for interfaces. @@ +19,5 @@ > + boolean is(final IState other); > + boolean isAtLeast(final IState other); > + } > + > + public interface IStateHolder { Ditto @@ +21,5 @@ > + } > + > + public interface IStateHolder { > + IState getState(); > + IState getDispatcherReadyEvent(); "getReadyState" @@ +100,5 @@ > + // but there is no automatic mechanism for non-native methods. > + throw new UnsupportedOperationException("Not allowed to queue non-native methods"); > + } > + > + if (stateHolder != null && stateHolder.getState().isAtLeast(state)) { Don't need to check null.

Comment on attachment 8844555 [details] [diff] [review] 0002-Bug-1343613-2.2-Add-GeckoView-event-queuing-in-Event.patch Review of attachment 8844555 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java @@ +72,5 @@ > + /* package */ EventDispatcher(final NativeQueue.IStateHolder stateHolder) { > + mStateHolder = stateHolder; > + } > + > + private boolean isReadyForDispatching() { isReadyForDispatchingToGecko ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java @@ +257,5 @@ > > private void init(Context context) { > Log.d(LOGTAG, "init"); > + > + eventDispatcher = new EventDispatcher(getStateHolder()); Why move initialization to init()? ::: widget/android/nsWindow.cpp @@ +3445,4 @@ > } > > void > +nsWindow::GeckoViewSupport::EnableEventDispatcher() Group this with the other GeckoViewSupport functions.

Comment on attachment 8844553 [details] [diff] [review] 0001-Bug-1343613-1.5-Refactor-native-call-queuing-out-of-.patch Review of attachment 8844553 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java @@ +92,4 @@ > } > } > > + public static class StateHolder implements NativeQueue.IStateHolder { Actually you don't even need a separate StateHolder class. Just let GeckoThread implement NativeQueue.IStateHolder, and make sState a non-static field (mState).

(In reply to Jim Chen [:jchen] [:darchons] from comment #15) > ::: widget/android/nsWindow.cpp > @@ +3445,4 @@ > > } > > > > void > > +nsWindow::GeckoViewSupport::EnableEventDispatcher() > > Group this with the other GeckoViewSupport functions. That's right below the last GeckoViewSupport function, same position as in the declaration.

(In reply to Jim Chen [:jchen] [:darchons] from comment #16) > Comment on attachment 8844553 [details] [diff] [review] > 0001-Bug-1343613-1.5-Refactor-native-call-queuing-out-of-.patch > > Review of attachment 8844553 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java > @@ +92,4 @@ > > } > > } > > > > + public static class StateHolder implements NativeQueue.IStateHolder { > > Actually you don't even need a separate StateHolder class. Just let > GeckoThread implement NativeQueue.IStateHolder, and make sState a non-static > field (mState). I've already rewritten it so that StateHolder is the actual holder of state now. This way we only need to implement the State interface for each NativeQueue "client".

Comment on attachment 8844630 [details] [diff] [review] 0001-Bug-1343613-1.6-Refactor-native-call-queuing-out-of-.patch Review of attachment 8844630 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with nits. ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java @@ +67,5 @@ > } > > + @Override > + public boolean is(final NativeQueue.State other) { > + return this.rank == ((State) other).rank; Keep the current behavior of comparing reference (i.e. return this == other;) @@ +467,5 @@ > * @return True if the current Gecko thread state matches > */ > public static boolean isStateAtMost(final State state) { > + final State actual = (State) sStateHolder.getState(); > + return actual.is(state) || !actual.isAtLeast(state); You can just `return state.isAtLeast(sStateHolder.getState());` @@ +480,4 @@ > * @return True if the current Gecko thread state matches > */ > public static boolean isStateBetween(final State minState, final State maxState) { > + return isStateAtLeast(minState) && isStateAtMost(maxState); > final NativeQueue.State actual = sStateHolder.getState(); > return actual.isAtLeast(minState) && maxState.isAtleast(actual); ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java @@ +55,5 @@ > + } > + > + @Override > + public boolean is(final NativeQueue.State other) { > + return this.rank == ((State) other).rank; return this == other; @@ +75,5 @@ > + private void setState(final State newState) { > + mStateHolder.setState(newState); > + } > + > + /* package */ StateHolder getStateHolder() { Not used? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/NativeQueue.java @@ +37,5 @@ > + return mReadyState; > + } > + > + public State getState() { > + synchronized (NativeQueue.sQueue) { Don't need synchronized for getting state because mState is already volatile. @@ +49,5 @@ > + > + public boolean checkAndSetState(final State expectedState, > + final State newState) { > + synchronized (NativeQueue.sQueue) { > + if (expectedState != null && mState != expectedState) { I guess `!mState.is(expectedState)` @@ +75,5 @@ > + } > + } > + > + private static final int QUEUED_CALLS_COUNT = 16; > + private static final ArrayList<QueuedCall> sQueue = new ArrayList<>(QUEUED_CALLS_COUNT); /* package */

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa287ae1cec [1.7] Refactor native call queuing out of GeckoThread. r=jchen https://hg.mozilla.org/integration/mozilla-inbound/rev/f9632a8f4b14 [2.3] Add GeckoView event queuing in EventDispatcher. r=jchen,snorp

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ec96c614b196b071f0961cde2cc5af1c6e380788 for Android crashes @nsWindow::GeckoViewSupport::EnableEventDispatcher, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=82290089&repo=mozilla-inbound

Oh yeah, you only want to do the EnableEventDispatcher stuff in the parent process.

Right, and we should only call it on the top level window.

Comment on attachment 8844979 [details] [diff] [review] 0003-Bug-1343613-3.0-Enable-window-event-dispatcher-only-.patch Review of attachment 8844979 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsAppShell.cpp @@ +574,4 @@ > java::GeckoThread::State::RUNNING()); > } > > + if (XRE_IsParentProcess()) { I think we should just not register "chrome-document-loaded" in child processes. The `GeckoThread::CheckAndSetState` call above doesn't apply to child processes either.

Register "chrome-document-loaded" observer only in the parent process. Carrying over r+.

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/33133003053c [1.7] Refactor native call queuing out of GeckoThread. r=jchen https://hg.mozilla.org/integration/mozilla-inbound/rev/12033f4800c8 [2.3] Add GeckoView event queuing in EventDispatcher. r=jchen,snorp https://hg.mozilla.org/integration/mozilla-inbound/rev/f859107b689b [3.1] Enable window event dispatcher only on top level window. r=jchen

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1673d1fc6164 [1.7.1] Add missing file. r=me

https://hg.mozilla.org/mozilla-central/rev/33133003053c https://hg.mozilla.org/mozilla-central/rev/12033f4800c8 https://hg.mozilla.org/mozilla-central/rev/f859107b689b https://hg.mozilla.org/mozilla-central/rev/1673d1fc6164