Bug 1208041 - Fix race condition when coalescing viewport events; r=snorp

We have a pretty messy system of coalescing viewport events that
introduced a race condition during the recent JNI refactoring. This
patch makes that code simpler and fixes the race condition. Instead of
keeping track of a previous viewport event, we now scan the event queue
for previous viewport events. This shouldn't be a perf concern because
we only scan the queue for viewport and native callback events, and stop
scanning as soon as we find another kind of event.

widget/android/nsAppShell.cpp

@@ -183,7 +183,6 @@ public:
 };
 
 nsAppShell::nsAppShell()
-    : mQueuedViewportEvent(nullptr)
 {
     gAppShell = this;
 
@@ -544,11 +543,6 @@ nsAppShell::LegacyGeckoEvent::Run()
 
     case AndroidGeckoEvent::VIEWPORT:
     case AndroidGeckoEvent::BROADCAST: {
-        {
-            MonitorAutoLock lock(nsAppShell::gAppShell->mEventQueue.mMonitor);
-            nsAppShell::gAppShell->mQueuedViewportEvent = nullptr;
-        }
-
         if (curEvent->Characters().Length() == 0)
             break;
 
@@ -826,11 +820,6 @@ void
 nsAppShell::LegacyGeckoEvent::PostTo(mozilla::LinkedList<Event>& queue)
 {
     {
-        // set this to true when inserting events that we can coalesce
-        // viewport events across. this is effectively maintaining a whitelist
-        // of events that are unaffected by viewport changes.
-        bool allowCoalescingNextViewport = false;
-
         EVLOG("nsAppShell::PostEvent %p %d", ae, ae->Type());
         switch (ae->Type()) {
         case AndroidGeckoEvent::COMPOSITOR_CREATE:
@@ -858,16 +847,26 @@ nsAppShell::LegacyGeckoEvent::PostTo(mozilla::LinkedList<Event>& queue)
             break;
 
         case AndroidGeckoEvent::VIEWPORT:
-            if (nsAppShell::gAppShell->mQueuedViewportEvent) {
-                // drop the previous viewport event now that we have a new one
-                EVLOG("nsAppShell: Dropping old viewport event at %p in favour of new VIEWPORT event %p",
-                      nsAppShell::gAppShell->mQueuedViewportEvent, ae);
-                // Delete the event and remove from list.
-                delete nsAppShell::gAppShell->mQueuedViewportEvent;
+            // Coalesce a previous viewport event with this one, while
+            // allowing coalescing to happen across native callback events.
+            for (Event* event = queue.getLast(); event;
+                    event = event->getPrevious())
+            {
+                if (event->HasSameTypeAs(this) &&
+                        static_cast<LegacyGeckoEvent*>(event)->ae->Type()
+                            == AndroidGeckoEvent::VIEWPORT) {
+                    // Found a previous viewport event; remove it.
+                    delete event;
+                    break;
+                }
+                NativeCallbackEvent callbackEvent(nullptr);
+                if (event->HasSameTypeAs(&callbackEvent)) {
+                    // Allow coalescing viewport events across callback events.
+                    continue;
+                }
+                // End of search for viewport events to coalesce.
+                break;
             }
-            nsAppShell::gAppShell->mQueuedViewportEvent = this;
-            allowCoalescingNextViewport = true;
-
             queue.insertBack(this);
             break;
 
@@ -887,21 +886,10 @@ nsAppShell::LegacyGeckoEvent::PostTo(mozilla::LinkedList<Event>& queue)
             queue.insertBack(this);
             break;
 
-        case AndroidGeckoEvent::NATIVE_POKE:
-            allowCoalescingNextViewport = true;
-            // fall through
-
         default:
             queue.insertBack(this);
             break;
         }
-
-        // if the event wasn't on our whitelist then reset mQueuedViewportEvent
-        // so that we don't coalesce future viewport events into the last viewport
-        // event we added
-        if (!allowCoalescingNextViewport) {
-            nsAppShell::gAppShell->mQueuedViewportEvent = nullptr;
-        }
     }
 }
 

widget/android/nsAppShell.h

@@ -115,22 +115,26 @@ protected:
 
     nsresult AddObserver(const nsAString &aObserverKey, nsIObserver *aObserver);
 
-    void ScheduleNativeEventCallback() override
+    class NativeCallbackEvent : public Event
     {
         // Capturing the nsAppShell instance is safe because if the app
         // shell is detroyed, this lambda will not be called either.
-        PostEvent([this] {
-            NativeEventCallback();
-        });
+        nsAppShell* const appShell;
+
+    public:
+        NativeCallbackEvent(nsAppShell* as) : appShell(as) {}
+        void Run() override { appShell->NativeEventCallback(); }
+    };
+
+    void ScheduleNativeEventCallback() override
+    {
+        mEventQueue.Post(mozilla::MakeUnique<NativeCallbackEvent>(this));
     }
 
     class Queue
     {
-    public:
-        // XXX need to be public for the mQueuedViewportEvent ugliness.
-        mozilla::Monitor mMonitor;
-
     private:
+        mozilla::Monitor mMonitor;
         mozilla::LinkedList<Event> mQueue;
 
     public:
@@ -169,7 +173,6 @@ protected:
 
     } mEventQueue;
 
-    Event* mQueuedViewportEvent;
     bool mAllowCoalescingTouches;
 
     nsCOMPtr<nsIAndroidBrowserApp> mBrowserApp;