353419 - [Mac] When trying to "Close Other Tabs" with >20 tabs open, tabs don't close and "too much recursion" error shows in JS Console

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

[Marcia Knous [:marcia]]

seen using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060918 Firefox/2.0.

STR:

1. Open 132 tabs (BBC RSS feed + open new tab and load various testing sites)
2. Highlight the first tab and select "Close Other Tabs"
3. I receive the Warning dialog and click "Close Tabs."

The tabs never close and I receive errors in the JS console. Attachment coming.

Comment 1

[Marcia Knous [:marcia]]

Attachment: Screenshot of Error console

Screenshot of Error Console. I am running on a G5 Dual 1.8 with 1.25 GB RAM

Comment 2

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

I'm able to reproduce with 120 tabs on my mac os x branch fresh build.

from venkman, I think the error is coming from:

@chrome://global/content/bindings/tabbrowser.xml:1300 removeAllTabsBut([object
XULElement])
@chrome://global/content/bindings/tabbrowser.xml:1315 oncommand([object
XULCommandEvent])
@chrome://browser/content/browser.xul:1 
@:0

note, line 1300 is the confirmEx() call, which has me confused.

Comment 3

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

interesting, quit (with 120 tabs) works, but not "close all tabs but"

Comment 4

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

taking, and I'll start looking into it by setting a breakpoint where that message is generated in the js intepreter and then getting a call stack

Comment 5

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

I also see this on the trunk.  what is interesting is the stack I get when breaking in JS_ReportErrorNumber() [when errorNumber is 26, or JSMSG_OVER_RECURSED]

my stack is mostly:

#1378 0x15910a39 in nsMacControl::WindowEventHandler ()
#1379 0x92f017eb in DispatchEventToHandlers ()
#1380 0x92f00e90 in SendEventToEventTargetInternal ()
#1381 0x92f43cdc in CallNextEventHandler ()

the aEvent is the same, but aUserData and aHandlerCallRef vary.

I'll attach the full stack.  

Comment 6

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

Attachment: full stack

Comment 7

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

on mac, I can hit the "too much recursion" if I just hold down command + t until I have about 120 untitled tabs and then I just switch tabs.

Comment 8

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

mark / josh:  is it possible that with this many tabs, we have so many (legit?) event handlers that when we go to execute some JS, we will fail the JS_CHECK_STACK_SIZE() (see http://lxr.mozilla.org/seamonkey/source/js/src/jsobj.c#4498) which is to prevent for out of control recursion?

Comment 9

[Mark Mentovai]

Seth, your stack from comment 5 is because of all of the scrollbars on the window.  Comment out this chunk and see if it makes a difference:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/mac/nsMacControl.cpp&rev=1.60.2.3&mark=649-654#640

Comment 10

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

> Seth, your stack from comment 5 is because of all of the scrollbars on the
> window.  Comment out this chunk and see if it makes a difference:

Mark, thanks for the quick response.  When I comment out that chunk of code and rebuild, I no longer have the problem.

Comment 11

[Tracy Walker [:tracy]]

Has there been a limit discovered as to how many tabs can be opened before this starts to happen? Perhaps it's resource dependent? I'm wondering if it can/should be release noted if not fixed by 2.0 final.

Comment 12

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

> Has there been a limit discovered as to how many tabs can be opened before this
> starts to happen? 

Not officially.  

mark, would it also vary if the pages loaded in the tabs had additional elements (like if there were frames or multiple line text areas, so we had more scroll bar.)

> Perhaps it's resource dependent? 

No, I don't think so.  In this case, it is mac specific, but the stack limit appears to be set here:

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcthreadcontext.cpp#408

> I'm wondering if it can/should be release noted if not fixed by 2.0 final.

Yes, we should do add a note to the release notes that the Mac version of Firefox has problems when a single window has many (?) tabs open.  Do you know the magic keywords / whiteboard summary for getting something into the release notes?

Note, the bugs fixed by Mark's checkin that introduced this regression are:

bug 54488 Scrollbars in inactive windows should also be inactive
bug 339370 Scrollbars in popup windows appear inactive
bug 339482 Bad app behavior caused by removing event handlers based on random stack garbage.

Comment 13

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

giving this back to nobody, as I'm not working on it.

but I think it is a valid mac bug (in mozilla/widget/src/mac/nsMacControl.cpp).

Josh / mark, this issue also affects trunk, but will this be an issue on the trunk, when we enable cocoa widgets?

Comment 14

[Tracy Walker [:tracy]]

keyword: relnote

Comment 15

[Mike Schroepfer]

Not going to block FF2 on this..  Would take in a 2.0.1

Comment 16

[Adam Guthrie]

For what it's worth, I think people are blowing this way out of proportion. 

I consider myself a "power user" and I probably only have about 30-40 tabs open in extreme cases. Sure, if people were seeing this error with anywhere up to around < 50 tabs, I'd be concerned, but it's with > 100 tabs!

We specifically added a preference for Firefox 2 to /stop/ people from getting into this situation (the "warn if opening lots of tabs pref). Also, I really question the probability of someone who has that many tabs open wanting to close all other tabs (I want all my tabs!), but that's debatable.

Off hand, I can think of a number of other things that could be added to the relnotes instead of a minor issue that not many people are going to hit like this one.

Comment 17

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

> We specifically added a preference for Firefox 2 to /stop/ people from getting
> into this situation (the "warn if opening lots of tabs pref).

to clarify, that promtp and pref were added to warn people that when opening up several tabs at once, the browser might become sluggish, and not to warn them that having lots of tabs could cause problems.

> Also, I really
> question the probability of someone who has that many tabs open wanting to
> close all other tabs (I want all my tabs!), but that's debatable.

keep in mind that once you hit this problem, other actions will fail (not just close other tabs), because the call stack is so big (due to all the event handlers.)

> Off hand, I can think of a number of other things that could be added to the
> relnotes instead of a minor issue that not many people are going to hit like
> this one.

do they all have the relnote keyword?  beltzner's query for bugs to relnote items is something like (ff2blocking- or mozilla181-) and keyword == relnote

Comment 18

[Mark Mentovai]

Maybe we should just turn off the code that gives scrollbars a gray appearance when their windows are deactivated.  The fix that caused this regression was visual-only.

Comment 19

[Marcia Knous [:marcia]]

I would be in favor of doing whatever makes the experience better for Mac users. FWIW, even opening the abcnews.com feed with 52 tabs causes issues on my PPC mac running 10.3.9. As Seth notes in Comment 17, things get really wonky when this happens, even if you try to stop loading all of the tabs.  Given that lots of users out there open news feeds, I don't think it is unreasonable to think that people on the Mac will hit this bug at one point or another, and it can be frustrating.

Comment 20

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

> Maybe we should just turn off the code that gives scrollbars a gray appearance
> when their windows are deactivated.  The fix that caused this regression was
> visual-only.

FWIW, I'd prefer that.

How safe would it be to disable the chunk of code (from your comment #9)?  If so, could this be a ride along bug for Fx2, if we have another RC?

Comment 21

[Mark Mentovai]

> How safe would it be to disable the chunk of code (from your comment #9)?

Completely and totally safe.

Comment 22

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

mark, how about we turn off this code on the trunk (with a #ifdef SEE_BUG_353419 or something) and the add all the magic flags and status whiteboard so that if there is another RC, it can make it (or get on the list for 2.0.0.1?)

if you are ok with this plan, I'll attach (and test) a patch for the trunk.

Comment 23

[Mark Mentovai]

Trunk builds use cocoa widgets now.  Bypassing the code there won't have any effect, except on builds that still explicitly use carbon widgets.

Comment 24

[Mark Mentovai]

(In reply to comment #22)
> for 2.0.0.1?)

Please tell me you're kidding.

Comment 25

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

Attachment: workaround, per mark's previous comments

Comment 26

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

Comment on attachment 241722 [details] [diff] [review]
workaround, per mark's previous comments

hoping for review and approval for 1.8.1.1 (which is the first dot release after firefox 2.0, I think?)

Comment 27

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

Comment on attachment 241722 [details] [diff] [review]
workaround, per mark's previous comments

will see what drivers think about this for Fx 2.0 RC3

Comment 28

[Brendan Eich [:brendan]]

Mark: is the stack depth bounded, and our 512K is just not enough?  Or could the stack grow linearly with number of tabs and no bound is enough given a large enough number of tabs?

If the former, we could just boost to 1MB.  The OS is happy to allocate stack to whatever ulimit -s defaults to (8MB on my MBP).

/be

Comment 29

[Mark Mentovai]

> Or could the stack grow linearly with number of tabs and no bound is enough
> given a large enough number of tabs?

Yup, that's it.  It just so happens that the 512kB limit just about matches the stack depth during window activation/deactivation at 100 tabs.

Comment 30

[Mark Mentovai]

Comment on attachment 241722 [details] [diff] [review]
workaround, per mark's previous comments

I changed my mind, we also need to fix this:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/mac/nsMacControl.cpp&rev=1.60.2.3&mark=369#369

+#ifdef SEE_BUG_353419
   if (mEnabled && (isPopup || ::IsWindowActive(mWindowPtr)))
+#else
+  if (mEnabled)
+#endif

Otherwise, scrollbars created when a window is not active will always have the grayed-out appearance.

Comment 31

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

> Otherwise, scrollbars created when a window is not active will always have the
> grayed-out appearance.

mark, I'm having a hard time reproducing this problem (with just the first fix).

can you suggest some steps to reproduce?  That way, I can verify your supplimental fix.

Comment 32

[Mark Mentovai]

Configure things to open new windows in tabs, and set a js timer to open a document large enough to have visible scrollbars.  Once the timer's running, switch out of the app, or activate a different window.  When the timer fires, go to the freshly-opened tab and check the quality of its scrollbars.

Comment 33

[Mike Schroepfer]

Hey folks - do we thing > 100 tabs is really a common enough problem that we'd touch *anything* in an RC to try and remedy?  I'm not sure I've ever in my life had > 30-40 tabs open...

Comment 34

[Adam Guthrie]

On the side of the opposition, I'd also like to point out that this bug does not occur by simply /opening/ a large number of tabs. It doesn't even occur when /quiting/ with a large number of tabs. The user must explicitly choose "Close Other Tabs" to see the bug.

Comment 35

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

> Hey folks - do we thing > 100 tabs is really a common enough problem that we'd
> touch *anything* in an RC to try and remedy?

Probably not very common.  According to http://wiki.mozilla.org/Tabbed_Browsing, a "Hardcore tabbed browsing user (10-20 tabs at once, overflows tab bar)".  I'm not sure where that number comes from, but it seems reasonable.

Adam's comment #34 is correct, but keep in mind that other actions (once you are in this state) can result in in the "too much recursion" error.

Also on the side of the opposition is that we've been running and testing with the existing code since 2006-05-30, and this is the only problem we know about, whereas turning off the code has some risk.

I think we can ship Fx 2.0 with this bug.  Mark, if you agree, I'll clear my approval request.

Comment 36

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

Comment on attachment 241722 [details] [diff] [review]
workaround, per mark's previous comments

(clearing nomination)

Comment 37

[Marcia Knous [:marcia]]

In Comment #19 I note that this happens with 50 tabs, which is the abcnews.com feed (and possibly less). And I think if someone had that many tabs open, they should be able to use the Close Other Tabs selection and not have their system hosed. I agree we can ship with this bug, but I would like to see it addressed in one of the follow up releases.

(In reply to comment #33)
> Hey folks - do we thing > 100 tabs is really a common enough problem that we'd
> touch *anything* in an RC to try and remedy?  I'm not sure I've ever in my life
> had > 30-40 tabs open...
> 

Comment 38

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

> I note that this happens with 50 tabs

I'm still not seeing that.  To be exact, when I have 100 tabs open (to about:blank), I hit this bug (when I attempt to close 99).

If I have 99 tabs open, I will not hit this bug when I attempt to close 98 tabs.

perhaps having content (like abc?) could expose the bug, as there could be scroll bars withing the abc pages?

note:  as far as other errors go, you will get the "too much recursion" problem when the window with > 100 tabs gets (or loses) focus.

Comment 39

[Mark Mentovai]

> note:  as far as other errors go, you will get the "too much recursion"
> problem when the window with > 100 tabs gets (or loses) focus.

I thought that activation/deactivation was a key to this bug all along.  If that's correct, and Bad Things begin happening when you try to activate or deactivate a window with some large number of tabs, then I don't think this should be passed over so easily for 2.0.  Otherwise, I'm OK waiting for 2.0.1.

Comment 40

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

from irc with mark...

Q: if a page were to have multiple scroll bars in the content, could that cause us to run out of room (on the stack) earlier?

A: of course.  extra frames, too, because each frame has a couple of scrollbars just for itself, even if they're hidden
	

I've been using about:blank, which may not be a good way to figure out the lower limit.  I'm going to try with gmail (which uses frames) to see what the lower limit might be.

Comment 41

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

shoot.

I am able to reproduce this error with as few as 20 tabs open, if they are open to gmail.com, which seems to be a reasonable thing.

I'm going to try out Mark's suggestion in comment #30 and attach a new patch.

Comment 42

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

wait, having problems reproducing the problem with only 20 tabs, working on a reproducable test case...

Comment 43

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

This won't block the Firefox 2 release unless we start getting lots of reports of it in the wild. We think that "Close All Other Tabs" isn't that frequently used. Nominating for 1.8.1.1

Comment 44

[Daniel Veditz [:dveditz]]

Situation hasn't really changed since comment 43, minusing for 1.8.1.1

Comment 45

[Tony Chung [:tchung]]

Fix verified on 2.0.0.3

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

Comment 46

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

tony, I don't think any fix was checked in for this bug.  (note, this was not always a consistent bug to reproduce.)

Comment 47

[Tony Chung [:tchung]]

okay, it looks like i wasnt able to repro the bug this time around.  Tried it on a vista machine also for 2.0.0.3.  I'll just leave this as a note.

Comment 48

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

> Tried it on a vista machine also for 2.0.0.3

tony, this was a mac only bug.

Comment 49

[AndrewM]

Is this wanted for Firefox 3? Target Milestone still says Firefox 2.

Comment 50

[Jesse Ruderman]

This was in the Firefox 2 release notes, so it might be good to fix it for Firefox 3...

Comment 51

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

I bet since bug 54488 has regressed, this might even be fixed on trunk!

Regardless, it doesn't block. Not a primary use case.

Comment 52

[Markus Stange [:mstange]]

We don't use native scrollbars any more. I couldn't reproduce the bug, so I'm closing this WORKSFORME, although FIXED might be valid as well.