Last Comment Bug 346968 - Problematic branch callback calls from the last ditch GC
: Problematic branch callback calls from the last ditch GC
Status: RESOLVED FIXED
[sg:critical][need testcase]
: fixed1.8.0.7, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-02 02:01 PDT by Igor Bukanov
Modified: 2006-10-10 03:02 PDT (History)
5 users (show)
mconnor: blocking1.8.1+
dveditz: blocking1.8.0.7+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (8.71 KB, patch)
2006-08-02 23:19 PDT, Igor Bukanov
brendan: review+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
Fix for 1.8.0 (1.07 KB, patch)
2006-08-08 12:28 PDT, Igor Bukanov
dveditz: approval1.8.0.7+
Details | Diff | Splinter Review

Description Igor Bukanov 2006-08-02 02:01:38 PDT
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.
Comment 1 Brendan Eich [:brendan] 2006-08-02 08:44:24 PDT
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
Comment 2 Brendan Eich [:brendan] 2006-08-02 08:45:48 PDT
Should be fixed on branches too.

/be
Comment 3 Igor Bukanov 2006-08-02 23:19:51 PDT
Created attachment 231895 [details] [diff] [review]
Fix
Comment 4 Brendan Eich [:brendan] 2006-08-03 10:15:47 PDT
Comment on attachment 231895 [details] [diff] [review]
Fix

Nice -- nominating.  The 1.8.0 branch will need a different patch.

/be
Comment 5 Mike Schroepfer 2006-08-04 10:11:34 PDT
Comment on attachment 231895 [details] [diff] [review]
Fix

a=schrep for drivers.
Comment 6 Igor Bukanov 2006-08-06 02:39:23 PDT
I committed to the trunk the patch from comment 3.
Comment 7 Igor Bukanov 2006-08-06 12:40:32 PDT
I committed the patch from comment 3 to MOZILLA_1_8_BRANCH
Comment 8 Igor Bukanov 2006-08-08 12:28:49 PDT
Created attachment 232787 [details] [diff] [review]
Fix for 1.8.0

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.
Comment 9 Igor Bukanov 2006-08-08 23:27:25 PDT
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 10 Daniel Veditz [:dveditz] 2006-08-09 14:42:46 PDT
Comment on attachment 232787 [details] [diff] [review]
Fix for 1.8.0

approved for 1.8.0 branch, a=dveditz for drivers
Comment 11 Igor Bukanov 2006-08-10 09:28:05 PDT
I committed the patch from comment 8 to MOZILLA_1_8_0_BRANCH
Comment 12 Alexander Sack 2006-09-12 03:38:42 PDT
... so this is a 1.8 regression, right?
Comment 13 Igor Bukanov 2006-09-12 04:34:07 PDT
(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.
Comment 14 Alexander Sack 2006-09-12 05:27:03 PDT
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?
Comment 15 Igor Bukanov 2006-09-12 06:14:11 PDT
(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.*.

Comment 16 Brendan Eich [:brendan] 2006-09-12 10:57:19 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.