343659 - Typing into text boxes performs very slowly

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Saler]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1a3) Gecko/20060705 BonEcho/2.0a3
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1a3) Gecko/20060705 BonEcho/2.0a3

With seven or eight tabs open at the time, I was typing up something in a large text box in the first tab. While the browser itself seemed to slow down a bit (perhaps not any more than I would have expected with that number of tabs open), typing into that text box became very, very slow. I attributed this to the automatic spell checker. After setting layoutspellcheckDefault to 0, I have not had a similar problem. While it would be nice to not sacrifice typing speed (to the point that I am waiting for what I have just typed to pop up on the screen a significant time later) when spell check is enabled, what would be preferable is an option in Preferences to disable spell check without having to use about:config or user.js.

Reproducible: Sometimes

Steps to Reproduce:
1. Make sure spell check is on.
2. Load about seven or eight, maybe more (but no more than 12 is really needed), tabs with pages in them. I used a couple Lexis-Nexis pages and a few Google searches, but any series of pages should work.
3. In one of those tabs, preferably the first, have a page open with a large text box to type into. Typing into that box should be considerably slower than normal.

Actual Results:  
Typing into the text box was very slow. After closing several tabs, it began to speed up a bit. When no other tabs were open I felt it was slower than normal, but still not so slow that I had to switch into another program to finish typing (what I ended up having to do by the end of it).

Expected Results:  
The speed with which the typed text appeared on the screen should not have been reduced by the fact that many tabs were open. Alternately, the software should have provided a global option within Preferences, easily accessible, to disable spell check.

Standard Firefox theme. No crash, so no Talkback crash ID. Computer configuration: Apple iBook G4 1.33 GHz 512 MB RAM. Three programs running (AdiumX, Thunderbird, Bon Echo) at the time.

Comment 1

[Oakwine]

I have seen this recently on my iMac (G5), and I believe that I also saw it today on my Windows XP box.  On my iMac, the characters were appearing at a rate of about 1 per second.  On my Windows box, a restart brought the typing rate back to normal.  I am not certain what the trigger is to start the problem.

Comment 2

[Tim Saler]

Okay, it's not just the spell check now. Once Bon Echo has been open for a considerable period of time (I don't have an exact number), it seems that typing into text boxes slows down dramatically. I have layoutspellcheckDefault set to 0, and the speed is about the same now as it was when I first detected the bug with spell check enabled.

Comment 3

[Nick Fagerlund]

I confirm this on a G3 iBook running 10.4.7, using today's Bon Echo build. This gets PROGRESSIVELY worse as you browse, rather than being an on-or-off sort of thing. 

The fastest way to induce this problem is to open enough tabs to trigger scrolling tab overflow, close enough tabs that they all fit on one screen again, open enough tabs to trigger scrolling overflow... rinse, spin, repeat. Even if you're not doing anything else interesting, that'll make text fields unusable sooner rather than later. 

Since this wasn't an issue until really really recently, I want to blame either bug 318168 or bug 254021. I think we could stand to test a bunch of old builds and try to figure out when it went sour. Tab bar scrolling hit the branch on the 28th of June; not sure when Undo Close Tab went live. 

(Also, here's something interesting: Did you notice that the arrow keys work at an acceptable speed no matter what?)

Comment 4

[Nick Fagerlund]

This regressed on the 1.8 branch between June 29th and June 30th. After June 30, I can always reproduce this bug by opening enough tabs to trigger scrolling tab overflow, closing one tab to make all tabs fit onscreen again, and then repeatedly opening and closing a single tab to turn overflow on and off. 

I am typing this from the 2006062906 Mac build right now, and I just spent like half an hour trying and failing to induce the behavior of this bug. On the 2006063011 Mac build, I was able to make text fields completely unusable in about a minute and a half by opening the appropriate number of tabs and buttonmashing Cmd-T / Cmd-W. 

June 29 was the first Bon Echo build to include the new scrolling tab bar overflow (bug 318168). It did not include bug 254021 (undo close tab). June 30 was the first Bon Echo build with Undo Close Tab. 

Aaaand that's the end of what I can do. We now need a code-savvy person to take a look at the 1.8-branch checkins between the 2006062906 and 2006063011 builds.

Tim, could you mark this as a regression? 

Comment 5

[Oakwine]

Should this be duped to Bug 343314 (100% cpu load while typing)?  They have the same regression date.

Comment 6

[Tim Saler]

I actually don't have the same situation with you as it pertains to the arrow keys working at full speed no matter how slow everything else is. My arrow keys also lag.

This is my first bug filing. How do I mark the bug as a regression?

Comment 7

[Martijn Wargers (dead)]

By adding the word 'regression' in the keywords section.
This is indeed probably a duplicate of bug 343314.

Comment 8

[(not reading, please use [e-mail] instead)]

based on comment #4 and #5, this sounds like either me or dietrich.

Comment 9

[Dietrich Ayala (:dietrich)]

Here's the SS code that gets called every time you type in the textarea/input:

http://lxr.mozilla.org/mozilla/source/browser/components/sessionstore/src/nsSessionStore.js#583

It fetches the text data here:

http://lxr.mozilla.org/mozilla/source/browser/components/sessionstore/src/nsSessionStore.js#931

I'll take this, as it sounds like it was not reproduce-able after bug 318168 landed, only after bug 254021 landed.

Comment 10

[Peter Kasting]

*** Bug 343314 has been marked as a duplicate of this bug. ***

Comment 11

[Peter Kasting]

Given bug 343314, I don't think this is mac-only; changing hardware/OS to all.  Requesting blocking; I think this needs to make B2 at the latest.  If there was a fix ready soonest I'd even push hard for drivers to consider this for B1.

Comment 12

[Nick Fagerlund]

Bless you for requesting blocking, Peter; I hate this bug.

Comment 13

[Peter Kasting]

This cannot be considered for beta 1 unless there's either a patch, or an excellent understanding of the bug's impact and what the fix looks like.

I'm really concerned about the possible impact this will have on testers, but it's not yet seen to be so severe that we can hang the beta on it indefinitely.

So, Dietrich, if you determine anything useful, please post.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: test disabling saving text data

I've experienced this a couple of times, however am not able to reproduce on either branch or trunk right now. I've tried pasting the testcase from bug 343314 into a textarea (and appending 5, 10 times), w/o experiencing either crashes or slowdowns while typing.

I'd thought that there might be a problem with the save timer, causing the session to be saved to disk each time we recieved the tabpanel's "input" event. However, that's not the case.

Maybe the collection of data on the "input" event was causing the slowdown, as it occurs on any change to the text, but I was able to add far more text than the testcase, without experiencing a slowdown. That said, possibly my hardware makes me less susceptible (2gb ram, 2ghz mactel).

In any case, attached is a patch to disable the collection of text data. Can someone who is able to reproduce the problem please try to do so with this change applied?

Comment 15

[Nick Fagerlund]

I don't have a build environment, and I don't think my G3 would thank me for setting one up. But if anyone wants to slap together a quick PPC or Universal test build, I would be more than happy to brutalize it for a while. 

Comment 16

[Nick Fagerlund]

I'm sorry, that was silly of me. Will patch and test.

Comment 17

[Nick Fagerlund]

2006070503 with attachment 228387 [details] [diff] [review] applied is completely protected against this bug, so you were probably right about that code. (I ran it once without the patch first, just to make sure everything was in order, and was able to render text fields unusable without a problem.)

Comment 18

[Dietrich Ayala (:dietrich)]

(In reply to comment #17)
> 2006070503 with attachment 228387 [details] [diff] [review] [edit] applied is completely protected against this
> bug, so you were probably right about that code. (I ran it once without the
> patch first, just to make sure everything was in order, and was able to render
> text fields unusable without a problem.)
> 

Thanks Nigel! I can reproduce this on-demand now. The key for me was your button mashing suggestion. Here's how I reproduce:

1. Open a single tab with a textarea.
2. Open a new tab and then close it (repeat 8 times or more)
3. Start typing in the textarea, and you'll notice it's very slow now.

In the session-restore code, we listen for the "input" event from the tabpanel. We should receive a single "input" event each time the contents of the textarea change. However, in this bug it appears that we're receiving input events for every tab that has ever been opened, including closed tabs.

Eg: After completing the reproduction steps above (ie, we now have a single tab open, that contains a textarea, and we've closed 8 tabs), for every character I type into the textarea, I should receive a single "input" event. However, I receive 9 "input" events.

Possibly I'm keeping references to closed tabs, thereby keeping them alive and sending events? But this doesn't seem to make sense because I shouldn't be receiving "input" events from tabs that had no input.

Looking further into the origin of that event now...

Comment 19

[Peter Kasting]

I'm changing the target milestone to B1.  I think even if Dietrich doesn't find the root cause of the bug by tomorrow, drivers should strongly consider taking attachment 228387 [details] [diff] [review] as a bandaid fix for B1, and then doing a "real" fix ASAP.

Comment 20

[Dietrich Ayala (:dietrich)]

Attachment: fix

Uses Mano's suggestion to implement nsIDomEventListener to allow proper removal of event listeners. This fixed the problem of receiving too many "input" events, and seems to alleviate the slowness of text input. It's possible that this patch will also affect bug 341893, which correlated leaks to the re-enabling of session-restore on the trunk.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 228403 [details] [diff] [review]
fix

Please also fix the tabbrowser listeners.

Comment 22

[Dietrich Ayala (:dietrich)]

Attachment: fixes tabbrowser listeners as well, changes listeners to non-capturing

Comment 23

[Asaf Romano (gone)]

Comment on attachment 228406 [details] [diff] [review]
fixes tabbrowser listeners as well, changes listeners to non-capturing

OK, this is all broken, probably since the change from DOMNodeInserted/Removed to TabOpen/Close landed.

1) For the first tab, you're getting the notificationbox instead of the tabpanel.
2) Starting from the second tab, you're adding the content events on the tabbrowser element instead of the tabpanel. This also means that you're adding the content events on the whoe tabbrowser element.

Here's what i think you should do
 1) use aEvent.originialTarget in the TabOpen/TabClose instead of aEvent.target. This gives you the <xul:tab> itself. You need to get the tabpanel from it (see tabbox.xml).
 2) Correct the initial call to onTabAdd to pass the tabpanel instead of the notification box.

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: adding capture back in, getting correct event target

This version fixes event targeting for the new tab events, re-adds capture.

Re: notificationbox vs tabpanel - I must be missing something here. The child nodes of mPanelContainer *are* notificationboxes, but with a panel ID. Are they one and the same?

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

To get this landed this morning we'd need an approved and baked patch, and it doesn't look like we have that. It's a nuisance, yes, but not one that I think we need to block the release for.

Mano, if you get the patch reviewed and it bakes nicely on trunk (or is really braindead safe enough that you think we should consider it after a short bake time) please ping a driver on IRC to see if the beta1 builds are still open for a last minute sneak-in.

Comment 26

[Asaf Romano (gone)]

Comment on attachment 228420 [details] [diff] [review]
adding capture back in, getting correct event target

>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================

>+        break;
>+      case "TabOpen":
>+      case "TabClose":
>+        var panelID = aEvent.originalTarget.linkedPanel;
>+        var tabpanels = aEvent.currentTarget.ownerDocument.defaultView.getBrowser().mPanelContainer;
>+        for (var i = 0; i < tabpanels.childNodes.length; i++) {
>+          if (panelID == tabpanels.childNodes[i].id)
>+            break;
>+        }

Just use originalTarget.ownerDocument.getElementById(panelID) here.

>+        switch (aEvent.type) {
>+          case "TabOpen":
>+            this.onTabAdd(aEvent.currentTarget.ownerDocument.defaultView, tabpanels.childNodes[i]);
>+            break;
>+          case "TabClose":
>+            this.onTabClose(aEvent.currentTarget.ownerDocument.defaultView, tabpanels.childNodes[i]);
>+            this.onTabRemove(aEvent.currentTarget.ownerDocument.defaultView, tabpanels.childNodes[i]);
>+            break;
>+        }
>+      case "TabSelect":
>+        this.onTabSelect(aEvent.currentTarget.ownerDocument.defaultView, aEvent.currentTarget);

TabSelect doesn't expect to the tabbrowser element either.

>-    tabbrowser.addEventListener("TabOpen", function(aEvent) {
>-      _this.onTabAdd(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
>-      }, false);
>-    tabbrowser.addEventListener("TabClose", function(aEvent) {
>-      _this.onTabClose(aEvent.currentTarget.ownerDocument.defaultView, aEvent.originalTarget);
>-      _this.onTabRemove(aEvent.currentTarget.ownerDocument.defaultView, aEvent.target);
>-      }, false);
>-    tabbrowser.addEventListener("TabSelect", function(aEvent) {
>-      _this.onTabSelect(aEvent.currentTarget.ownerDocument.defaultView, aEvent.currentTarget);
>-      }, false);
>+    tabbrowser.addEventListener("TabOpen", this, true);
>+    tabbrowser.addEventListener("TabClose", this, true);
>+    tabbrowser.addEventListener("TabSelect", this, true);

No need for capturing here, not really an issue though (considering the way these events are fired).


>   },
> 
> /* ........ QueryInterface .............. */
> 
>   QueryInterface: function(aIID) {
>     if (!aIID.equals(Ci.nsISupports) && !aIID.equals(Ci.nsIObserver) && 
>-      !aIID.equals(Ci.nsISupportsWeakReference) && 
>+      !aIID.equals(Ci.nsISupportsWeakReference) && !aIID.equals(Ci.nsIDOMEventListener) &&

nit: put this on a separate line.

Comment 27

[Asaf Romano (gone)]

Beltzner, the summary of this bug is misleading, this breaks session store in all sort of unexpected ways (e.g. text field store only works on the first tab). We also leak event listeners (and probaly also the tab elements) here whenever a tab is closed. FWIW, I don't think this patch needs to bake on trunk for long.

Chaning milestone back to b1.

Comment 28

[Asaf Romano (gone)]

Also, this is Dietrich's patch, not mine ;)

Comment 29

[Dietrich Ayala (:dietrich)]

Attachment: comments addressed

Comment 30

[Dietrich Ayala (:dietrich)]

Attachment: now with proper tabpanel passing

Comment 31

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 228447 [details] [diff] [review]
now with proper tabpanel passing

This change does not look like it is highly risky provided it has been tested with all of the events that this event listener handles (load, tab switches, opens etc).

Comment 32

[Asaf Romano (gone)]

Comment on attachment 228447 [details] [diff] [review]
now with proper tabpanel passing


>+      case "TabOpen":
>+      case "TabClose":
>+        var panelID = aEvent.originalTarget.linkedPanel;
>+        var tabpanel = aEvent.originalTarget.ownerDocument.getElementById(panelID);
>+        switch (aEvent.type) {
>+          case "TabOpen":
>+            this.onTabAdd(aEvent.currentTarget.ownerDocument.defaultView, tabpanel);
>+            break;
>+          case "TabClose":
>+            this.onTabClose(aEvent.currentTarget.ownerDocument.defaultView, tabpanel);
>+            this.onTabRemove(aEvent.currentTarget.ownerDocument.defaultView, tabpanel);
>+            break;
>+        }
>+        break;

make this sub-switch a if-else block.

 
>+      case "TabSelect":
>+        var tabpanels = aEvent.currentTarget.ownerDocument.defaultView.getBrowser().mPanelContainer;
>+        this.onTabSelect(aEvent.currentTarget.ownerDocument.defaultView, tabpanels);

You can use aEvent.currentTarget.mPanelContainer directly here.


>   QueryInterface: function(aIID) {
>     if (!aIID.equals(Ci.nsISupports) && !aIID.equals(Ci.nsIObserver) && 
>       !aIID.equals(Ci.nsISupportsWeakReference) && 
>+      !aIID.equals(Ci.nsIDOMEventListener) &&
>       !aIID.equals(Ci.nsISessionStore)) {
>       Components.returnCode = Cr.NS_ERROR_NO_INTERFACE;

nit: put nsIObserver on a separate line too.


r=mano with these corrected.

Comment 33

[Dietrich Ayala (:dietrich)]

Attachment: as going on trunk

Patch as it'll go on trunk. Looking into why balsa went orange before checking in.

Did the manual smoketest:

- set browser.sessionstore.resume_session=true
- open a tab, restart
- open a window, restart
- close a tab, restart
- close a window, restart
- change selected tab, restart
- type in a textarea, restart
- force-kill, restart, restore session
- force-kill, restart, don't restore session

Comment 34

[Darin Fisher]

I'm experiencing this bug (I think) while composing email messages in Gmail.  I could be mistaken, but I think the problem has gotten worse since I first started my browser.  I left the browser running overnight, re-opened Gmail in the morning, and it has been slow ever since.

Comment 35

[Dietrich Ayala (:dietrich)]

Attachment: followup to pass proper param to onTabClose()

Comment 36

[Asaf Romano (gone)]

Comment on attachment 228469 [details] [diff] [review]
followup to pass proper param to onTabClose()

r=mano

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 228463 [details] [diff] [review]
as going on trunk

tested against latest branch build, looks good.

Comment 38

[Dietrich Ayala (:dietrich)]

confirmed on a trunk release build.

Comment 39

[Tim Saler]

Thanks Dietrich & everyone else for your hard work. It is truly appreciated.

Comment 40

[Nick Fagerlund]

Haha, beat me to it. My thanks, too, DA.

Comment 41

[Worcester12345]

add perf key word?