346968 - Problematic branch callback calls from the last ditch GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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

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

Should be fixed on branches too.

/be

Comment 3

[Igor Bukanov]

Attachment: Fix

Comment 4

[Brendan Eich [:brendan]]

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]

Comment on attachment 231895 [details] [diff] [review]
Fix

a=schrep for drivers.

Comment 6

[Igor Bukanov]

I committed to the trunk the patch from comment 3.

Comment 7

[Igor Bukanov]

I committed the patch from comment 3 to MOZILLA_1_8_BRANCH

Comment 8

[Igor Bukanov]

Attachment: 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]

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

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]

I committed the patch from comment 8 to MOZILLA_1_8_0_BRANCH

Comment 12

[Alexander Sack]

... so this is a 1.8 regression, right?

Comment 13

[Igor Bukanov]

(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]

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]

(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]]

(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