719494 - Closing tabs is too slow sometimes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Margaret Leibovic]

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)]

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]

(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?)]

Could we hide the tab in the UI, and then the message can remove it for real?

Comment 4

[:Margaret Leibovic]

(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]

Attachment: 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)]

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) ([e-mail]) (he/him)]

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]

Attachment: 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 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ad9ccfcd15

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

How would an add-on programmatically close a tab?

Comment 11

[:Margaret Leibovic]

Attachment: 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)]

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]

Attachment: 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]]

Backed out of inbound for M1 crashes:
https://hg.mozilla.org/mozilla-central/rev/e46cca506613

Comment 15

[:Margaret Leibovic]

Attachment: 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 16

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/111b399b10e4

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/111b399b10e4

Comment 19

[:Margaret Leibovic]

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]]

Comment on attachment 590631 [details] [diff] [review]
combined patch

[Triage Comment]
Mobile only - approved for Aurora.

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e02a829225d1

Comment 22

[Cristian Nicolae (:xti)]

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