Last Comment Bug 719494 - Closing tabs is too slow sometimes
: Closing tabs is too slow sometimes
Status: VERIFIED FIXED
: perf
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P1 normal with 1 vote (vote)
: Firefox 12
Assigned To: :Margaret Leibovic
:
: Sebastian Kaspari (:sebastian)
Mentors:
: 720205 (view as bug list)
Depends on:
Blocks: 720205
  Show dependency treegraph
 
Reported: 2012-01-19 10:39 PST by :Margaret Leibovic
Modified: 2016-07-29 14:21 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
verified
beta+
11+


Attachments
WIP (7.25 KB, patch)
2012-01-19 16:48 PST, :Margaret Leibovic
mbrubeck: feedback+
Details | Diff | Splinter Review
patch (8.62 KB, patch)
2012-01-20 12:32 PST, :Margaret Leibovic
mbrubeck: review+
Details | Diff | Splinter Review
additional patch for add-ons (5.75 KB, patch)
2012-01-20 14:35 PST, :Margaret Leibovic
mbrubeck: review-
Details | Diff | Splinter Review
follow-up cleanup patch (10.26 KB, patch)
2012-01-20 16:51 PST, :Margaret Leibovic
mbrubeck: review+
Details | Diff | Splinter Review
combined patch (12.15 KB, patch)
2012-01-22 22:30 PST, :Margaret Leibovic
mbrubeck: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2012-01-19 10:39:41 PST
I'd have to investigate more to see if there's a pattern to when this happens, but sometimes tapping the close button on the tabs tray will seem like it hasn't done anything because it takes so long for the tab to close. This is especially bad if I just think my tap hasn't registered, so I tap again and end up accidentally closing another tab.
Comment 1 Lucas Rocha (:lucasr) 2012-01-19 10:56:45 PST
The tabs UI is built around messages to/from gecko. So, when you clock on X button to close a tab, it sends a message to gecko and waits for a message from gecko confirming the operation. Only then the tab element is removed from the UI.

I've suggested before that we should build the tabs UI on top of a sort of message queue that allows us to update UI immediately while handling gecko state bits in parallel. So, in case of a close operation, we'd immediately switch to a another tab and remove the closed tab from the list. We shouldn't need to wait gecko to actually remove tab and send a message back to native UI to then update the UI.

We'd still have to fix cases where gecko is simply not responding to messages. But this is a major problem that has to be fixed anyway.
Comment 2 :Margaret Leibovic 2012-01-19 12:01:25 PST
(In reply to Lucas Rocha (:lucasr) from comment #1)
> I've suggested before that we should build the tabs UI on top of a sort of
> message queue that allows us to update UI immediately while handling gecko
> state bits in parallel. So, in case of a close operation, we'd immediately
> switch to a another tab and remove the closed tab from the list. We
> shouldn't need to wait gecko to actually remove tab and send a message back
> to native UI to then update the UI.

I think we either need something like this, or we need to change the way we're sending messages so that we're not relying on their response before updating the UI. The same issue is causing bug 718465.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-19 12:12:29 PST
Could we hide the tab in the UI, and then the message can remove it for real?
Comment 4 :Margaret Leibovic 2012-01-19 13:22:00 PST
(In reply to Mark Finkle (:mfinkle) from comment #3)
> Could we hide the tab in the UI, and then the message can remove it for real?

I'll look into trying this. I think we can be smarter about how we rely on message passing.
Comment 5 :Margaret Leibovic 2012-01-19 16:48:44 PST
Created attachment 590046 [details] [diff] [review]
WIP

Following the message path, I couldn't find a good reason to wait for the Tab:Closed event. It looks to me like BrowserApp.closeTab just does some cleaning up, and the only data it passes back is the tab id, which we already know in java. This patch make the UI way more responsive, and if you guys like this approach, I think we should look into other places where we're waiting on Gecko when we don't need to be.

Talking about this in the office, Mossop brought up the fact that we'd need to wait if we wanted to properly handle onbeforeunload events, but we're not supporting that now anyway (and I don't know if we want to support this, since it's a non-standard event, and it can be annoying for users).
Comment 6 Matt Brubeck (:mbrubeck) 2012-01-19 17:15:09 PST
Comment on attachment 590046 [details] [diff] [review]
WIP

This looks good to me.  I do wonder if there are any other paths that cause Tab.destroy to get called in JavaScript without going through the Java code, like maybe window.close() from content...
Comment 7 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-19 17:21:52 PST
I like this, but I think we do need to continue to handle Tab:Closed because of the case mbrubeck mentioned. In the common case (user closes a tab) we would just ignore Tab:Closed since the tab id wouldn't exist (presumably).
Comment 8 :Margaret Leibovic 2012-01-20 12:32:44 PST
Created attachment 590301 [details] [diff] [review]
patch

Discussed on IRC: Events like DOMWindowClose are handled in Java. We only ever call BrowserApp.closeTab (and then Tab.destroy), as the result of a message from Java. Since we're already doing this, we can just make Java in control of closing tabs, and BrowserApp._closeTab will only be responsible for updating BrowserApp's state to reflect what's going on in Java.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-20 13:40:05 PST
How would an add-on programmatically close a tab?
Comment 11 :Margaret Leibovic 2012-01-20 14:35:18 PST
Created attachment 590355 [details] [diff] [review]
additional patch for add-ons

This patch adds back the Tab:Closed message, but it will only be sent when the tab close is initiated from JS by BrowserApp.closeTab.
Comment 12 Matt Brubeck (:mbrubeck) 2012-01-20 15:50:56 PST
Comment on attachment 590355 [details] [diff] [review]
additional patch for add-ons

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+            } else if (event.equals("Tab:Closed")) {
>+                int tabId = message.getInt("tabID");
>+                handleCloseTab(tabId);

I *think* instead of handleCloseTab, we want to call Tabs.getInstance().closeTab here.  That will handle the logic of selecting the correct next tab.

Optional: Should we move handleCloseTab from GeckoApp into Tabs?

>+++ b/mobile/android/chrome/content/browser.js

>+  closeTab: function closeTab(aTab) {
>+    let message = {
>+      gecko: {
>+        type: "Tab:Closed",
>+        tabID: aTab.id
>+      }
>+    };
>+    sendMessageToJava(message);
>+
>+    this._closeTab(aTab);
>+  },

...and then we can remove the _closeTab call here because we'll get a Tab:Close message from Java.

I think we should also rename _closeTab to something like _handleTabClose, to emphasize that it only gets called in response to a Java method.

I'm also tempted to swap the names of the "Tab:Close" and "Tab:Closed" messsages.  Then "Tab:Close" would be a request to close a tab, and "Tab:Closed" would be sent in response.
Comment 13 :Margaret Leibovic 2012-01-20 16:51:15 PST
Created attachment 590390 [details] [diff] [review]
follow-up cleanup patch

Addressed the comments from comment 12. I found at least a few other things I want to do to clean up this code, but I will file other bugs to avoid scope creep (things like moving more tab-related logic from GeckoApp to Tabs, including the Tab:Foo listeners).

Also, when I tested this, closing the active tab resulted in an awkward moment where the tab closed but the next tab wasn't selected yet, but I think if we do a similar cleanup/restructuring for Tab:Select and Tab:Selected, we can fix that (bug 719493 was already filed about this slowness).
Comment 14 Ed Morley [:emorley] 2012-01-20 19:24:35 PST
Backed out of inbound for M1 crashes:
https://hg.mozilla.org/mozilla-central/rev/e46cca506613
Comment 15 :Margaret Leibovic 2012-01-22 22:30:08 PST
Created attachment 590631 [details] [diff] [review]
combined patch

Since we had to back out the first patch, I decided to just combine these two patches.

Philor said that the crash was:
java.lang.NullPointerException at org.mozilla.gecko.BrowserToolbar.setProgressVisibility(BrowserToolbar.java:257)

I suspect that what happened is that we exposed a race condition in the tab select code path, since we're still blocking on Gecko to select a tab when we call closeTab. As a solution to this, I wrote a patch on top of this one to make the corresponding un-blocking changes to the tab select code, and I'll upload it to bug 719493 shortly. I think that if we land those together, that should fix the problem.
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-25 07:25:26 PST
*** Bug 720205 has been marked as a duplicate of this bug. ***
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:30:22 PST
https://hg.mozilla.org/mozilla-central/rev/111b399b10e4
Comment 19 :Margaret Leibovic 2012-01-25 10:31:37 PST
Comment on attachment 590631 [details] [diff] [review]
combined patch

[Approval Request Comment]
Improves tab responsiveness. Landed on m-c this morning, so it will be in Nightly tomorrow.
Comment 20 Alex Keybl [:akeybl] 2012-01-25 17:07:16 PST
Comment on attachment 590631 [details] [diff] [review]
combined patch

[Triage Comment]
Mobile only - approved for Aurora.
Comment 22 Cristian Nicolae (:xti) 2012-03-02 08:26:50 PST
Verified fixed on:

Firefox 13.0a1 (2012-03-02)
20120302031112
http://hg.mozilla.org/mozilla-central/rev/3a7b9e61c263

--
Device: Samsung Galaxy S2
OS: Android 2.3.4

Note You need to log in before you can comment on or make changes to this bug.