Closed Bug 346968 Opened 14 years ago Closed 14 years ago
Problematic branch callback calls from the last ditch GC
The last ditch GC must preserve cx->newborn array and keep all atoms. But as far as I can see calling branch callback can break this since there is very little restrictions on what the callback can do. In particular, it can call and JS_MaybeGC() or even JS_GC() wiping out all newborn arrays. In particular, the branch callback in the browser, http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#654 , calls JS_MaybeGC and potentially install the debugger. The former would GC all newborns with WAY_TOO_MUCH_GC defined (when JS_MaybeGC is just JS_GC) and the later AFAICS can allocate GC things (this is the reason I mark the bug as a security problem). This has to be addressed somehow. The simplest and IMO the best option would be just to disable the callback on the last ditch GC. Instead explicit calls to the callback should be issued from other points where it is safe to call the callback.
Yes, this is just a big bad bug, courtesy of my big block head. The bug I sought to fix was a kind of D.O.S testcase where there weren't enough branches in scripted code for the old DOM branch callback monitor, which counted branches, to catch the expensive native branches or other native operations. See bug 203278 comment 27. Of course, we've changed the DOM branch callback to monitor real time so this is all unmotivated now. Let's turn it off, quickly. If it helps to clean up control flow, rip out anything that distorted GC-locking in order to "rotate" the branch callback down from the allocator to the end of the collector. Sorry for the mess. /be
Should be fixed on branches too. /be
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #231895 - Flags: review?(brendan)
Comment on attachment 231895 [details] [diff] [review] Fix Nice -- nominating. The 1.8.0 branch will need a different patch. /be
Comment on attachment 231895 [details] [diff] [review] Fix a=schrep for drivers.
Attachment #231895 - Flags: approval1.8.1? → approval1.8.1+
I committed to the trunk the patch from comment 3.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
A version for 1.8.0 is much simpler compared with trunk/1.8.1 cases. On 1.8.0 branch the code to call the callback was not moved outside GC lock just to be removed completely. So this patch just erases the callback checking/calling in js_NewGCThing.
Attachment #232787 - Flags: approval18.104.22.168?
Note that on the trunk/1.8.1 branch the problem was much more serious due to close hooks that can be triggered to run from the branch callback invoked under js_NewGCThing. In fact that was the reason for the rval changes in array_extra in the patch for bug 322135. There rval2 could be GC-ed through the branch callback when IndexToId creates a string for a large index. Since the patch was touching the same code, I tried to make rval2 rooted even without the changes for this bug committed. But I should reported it as a separated bug and not wait for orange tinderboxes with a mistake in the implementation hidden among other changes.
Comment on attachment 232787 [details] [diff] [review] Fix for 1.8.0 approved for 1.8.0 branch, a=dveditz for drivers
Attachment #232787 - Flags: approval22.214.171.124? → approval126.96.36.199+
Whiteboard: [need testcase] → [sg:critical][need testcase]
... so this is a 1.8 regression, right?
(In reply to comment #12) > ... so this is a 1.8 regression, right? No. It is just that earlier branches require more "cooperation" with the engine to expose the bug, like, for example, periodically calling JS_GC() in the branch callback.
hmmm ... the code *after* applying this patch on 1.8.0 branch looks equal to the existing code in 1.0.x ... I don't see any branchCallback invocation there. What is the equivalent on 1.0.x? Any hints?
(In reply to comment #14) > hmmm ... the code *after* applying this patch on 1.8.0 branch looks equal to > the existing code in 1.0.x ... I don't see any branchCallback invocation there. > What is the equivalent on 1.0.x? Any hints? Aha, that was indeed a regression that happens in >= 1.8*. I misread that 1.8 as 1.8.1. So no bug in Firefox 1.0.*.
(In reply to comment #15) > Aha, that was indeed a regression that happens in >= 1.8*. I misread that 1.8 > as 1.8.1. So no bug in Firefox 1.0.*. Or in Firefox 1.5.0.x (based on 1.8.0.x). /be
You need to log in before you can comment on or make changes to this bug.