Closed Bug 353419 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

PowerPC
macOS
defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: marcia, Unassigned)

References

Details

(Keywords: regression, relnote)

Attachments

(3 files)

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.
Screenshot of Error Console. I am running on a G5 Dual 1.8 with 1.25 GB RAM
Summary: Unable to close 132 tabs → Unable to close 132 tabs, getting "Error: too much recursion" in the js console
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.
interesting, quit (with 120 tabs) works, but not "close all tabs but"
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
Assignee: nobody → sspitzer
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.  
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.
Summary: Unable to close 132 tabs, getting "Error: too much recursion" in the js console → [mac os X] problems after opening up many (>100 tabs), getting "Error: too much recursion" in the js console
Version: 2.0 Branch → Trunk
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?
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
> 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.
Blocks: 54488
Keywords: regression
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.
> 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.
Summary: [mac os X] problems after opening up many (>100 tabs), getting "Error: too much recursion" in the js console → [mac os X] problems after opening up many (>100 tabs?), getting "Error: too much recursion" in the js console
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?

Assignee: sspitzer → nobody
keyword: relnote
Flags: blocking-firefox2?
Keywords: relnote
Not going to block FF2 on this..  Would take in a 2.0.1
Flags: blocking-firefox2? → blocking-firefox2-
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.
Summary: [mac os X] problems after opening up many (>100 tabs?), getting "Error: too much recursion" in the js console → [Mac] When trying to "Close Other Tabs" with > 100 tabs open, tabs don't close and "too much recursion" error shows in JS Console
> 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
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.
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.
> 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?
> How safe would it be to disable the chunk of code (from your comment #9)?

Completely and totally safe.
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.
Target Milestone: --- → Firefox 2
Trunk builds use cocoa widgets now.  Bypassing the code there won't have any effect, except on builds that still explicitly use carbon widgets.
(In reply to comment #22)
> for 2.0.0.1?)

Please tell me you're kidding.
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?)
Attachment #241722 - Flags: review?(mark)
Attachment #241722 - Flags: review?
Attachment #241722 - Flags: approval1.8.1.1?
Attachment #241722 - Flags: review?(mark) → review+
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
Attachment #241722 - Flags: approval1.8.1.1? → approval1.8.1?
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
> 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 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.
Attachment #241722 - Flags: review+ → review-
> 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.
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.
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...
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.
> 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 on attachment 241722 [details] [diff] [review]
workaround, per mark's previous comments

(clearing nomination)
Attachment #241722 - Flags: approval1.8.1?
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...
> 

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

Flags: blocking-firefox2- → blocking-firefox2?
Summary: [Mac] When trying to "Close Other Tabs" with > 100 tabs open, tabs don't close and "too much recursion" error shows in JS Console → [Mac] When trying to "Close Other Tabs" with >20 tabs open, tabs don't close and "too much recursion" error shows in JS Console
wait, having problems reproducing the problem with only 20 tabs, working on a reproducable test case...
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
Flags: blocking1.8.1.1?
Flags: blocking-firefox2?
Flags: blocking-firefox2-
Situation hasn't really changed since comment 43, minusing for 1.8.1.1
Flags: blocking1.8.1.1? → blocking1.8.1.1-
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
tony, I don't think any fix was checked in for this bug.  (note, this was not always a consistent bug to reproduce.)
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.
> Tried it on a vista machine also for 2.0.0.3

tony, this was a mac only bug.
Is this wanted for Firefox 3? Target Milestone still says Firefox 2.
This was in the Firefox 2 release notes, so it might be good to fix it for Firefox 3...
Flags: blocking-firefox3?
Target Milestone: Firefox 2 → ---
I bet since bug 54488 has regressed, this might even be fixed on trunk!

Regardless, it doesn't block. Not a primary use case.
Flags: blocking-firefox3? → blocking-firefox3-
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.