155838 - Infinite loop caused by strange code

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[timeless]

tail of callstack:
jsds_NotifyPendingDeadScripts(JSContext * 0x042fd840) line 480
jsds_GCCallbackProc(JSContext * 0x042fd840, JSGCStatus JSGC_END) line 506 + 9 
bytes
XPCJSRuntime::GCCallback(JSContext * 0x042fd840, JSGCStatus JSGC_END) line 560 
+ 21 bytes
js_GC(JSContext * 0x042fd840, unsigned int 0) line 1370 + 6 bytes
js_ForceGC(JSContext * 0x042fd840, unsigned int 0) line 980 + 19 bytes
JS_GC(JSContext * 0x042fd840) line 1656 + 8 bytes
JS_MaybeGC(JSContext * 0x0207f3e0) line 1675 + 6 bytes
nsJSContext::ScriptExecuted(nsJSContext * const 0x0207f3e4) line 1588
DoPostScriptEvaluated(JSContext * 0x0207f3e4, JSExceptionState * 0x07570710) 
line 98
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x04bb3278, 
nsXPCWrappedJS * 0x04bb3278, unsigned short 57720, const nsXPTMethodInfo * 
0x029fb8f8, nsXPTCMiniVariant * 0x80570021) line 1571 + 11 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x041c9b38, unsigned short 3, 
const nsXPTMethodInfo * 0x029fb8f8, nsXPTCMiniVariant * 0x0012ec34) line 430
PrepareAndDispatch(nsXPTCStubBase * 0x00000000, unsigned int 3, unsigned int * 
0x0012ecec, unsigned int * 0x0012ecdc) line 115 + 18 bytes
SharedStub() line 139
jsds_ExecutionHookProc(JSDContext * 0x02232738, JSDThreadState * 0x00000000, 
unsigned int 4, void * 0x00000000, long * 0x0012eea8) line 669
jsd_CallExecutionHook(JSDContext * 0x00afa518, JSContext * 0x042fd840, unsigned 
int 4, unsigned int (JSDContext *, JSDThreadState *, unsigned int, void *, long 
*)* 0x01125b0d jsds_ExecutionHookProc(JSDContext *, JSDThreadState *, unsigned 
int, void *, long *), void * 0x00000000, long * 0x0012eea8) line 168
jsd_ThrowHandler(JSContext * 0x042fd840, JSScript * 0x0217dd30, unsigned char * 
0x0217ddc6, long * 0x0012eea8, void * 0x04f5c358) line 149 + 18 bytes
js_Interpret(JSContext * 0x042fd840, long * 0x0012ef34) line 3889 + 38 bytes
js_Invoke(JSContext * 0x00000001, unsigned int 0, unsigned int 0) line 805 + 10 
bytes
js_Interpret(JSContext * 0x042fd840, long * 0x0012f10c) line 2744
js_Invoke(JSContext * 0x00000001, unsigned int 1, unsigned int 2) line 805 + 10 
bytes
js_InternalInvoke(JSContext * 0x042fd868, JSObject * 0x02091f00, long 80818656, 
unsigned int 0, unsigned int 1, long * 0x0012f2f0, long * 0x0012f224) line 880 
+ 13 bytes
JS_CallFunctionValue(JSContext * 0x042fd840, JSObject * 0x02091f00, long 
80818656, unsigned int 1, long * 0x0012f2f0, long * 0x0012f224) line 3428 + 26 
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x00b4f968, void * 
0x02091f00, void * 0x04d131e0, unsigned int 1, void * 0x0012f2f0, int * 
0x0012f2d4, int 0) line 1042 + 25 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x042fd840, 
nsIDOMEvent * 0x06927de0) line 182 + 30 bytes
nsEventListenerManager::HandleEventSubType(nsEventListenerManager * const 
0x00000000, nsListenerStruct * 0x05d9ca68, nsIDOMEvent * 0x06927de0, 
nsIDOMEventTarget * 0x044ed1a0, unsigned int 110263784, unsigned int 0) line 
1221 + 14 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x05d9c428, 
nsIPresContext * 0x00000000, nsEvent * 0x0012f684, nsIDOMEvent * * 0x00000000, 
nsIDOMEventTarget * 0x044ed1a0, unsigned int 7, nsEventStatus * 0x0012f6bc) 
line 1901 + 16 bytes
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x044ed190, 
nsIPresContext * 0x05d2a008, nsEvent * 0x0012f684, nsIDOMEvent * * 0x0012f650, 
unsigned int 1, nsEventStatus * 0x0012f6bc) line 745
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x044ed190, 
unsigned int 0) line 1522 + 35 bytes
nsDocShell::EndPageLoad(nsDocShell * const 0x00000000, nsIWebProgress * 
0x02c088dc, nsIChannel * 0x04957278, unsigned int 0) line 3995
nsWebShell::EndPageLoad(nsWebShell * const 0x00000000, nsIWebProgress * 
0x02c088dc, nsIChannel * 0x04957278, unsigned int 0) line 731
nsDocShell::OnStateChange(nsDocShell * const 0x02c088dc, nsIWebProgress * 
0x02c088dc, nsIRequest * 0x04957278, unsigned int 76903032, unsigned int 0) 
line 3909
nsDocLoaderImpl::FireOnStateChange(nsDocLoaderImpl * const 0x00000000, 
nsIWebProgress * 0x02c088dc, nsIRequest * 0x04957278, int 131088, unsigned int 
0) line 1183
nsDocLoaderImpl::doStopDocumentLoad(nsDocLoaderImpl * const 0x00000000, 
nsIRequest * 0x04957278, unsigned int 0) line 834
nsDocLoaderImpl::DocLoaderIsEmpty(nsDocLoaderImpl * const 0x00000000) line 732
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x068c96e0, nsIRequest * 
0x00000000, nsISupports * 0x00000000, unsigned int 0) line 663
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x00000000, nsIRequest * 
0x02c088cc, nsISupports * 0x00000000, unsigned int 0) line 532 + 13 bytes
nsJARChannel::OnStopRequest(nsJARChannel * const 0x00000000, nsIRequest * 
0x068fe564, nsISupports * 0x00000000, unsigned int 0) line 612
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x00000000) line 
213
PL_HandleEvent(PLEvent * 0x06907dac) line 597
PL_ProcessPendingEvents(PLEventQueue * 0x1003344a) line 526 + 6 bytes
_md_EventReceiverProc(HWND__ * 0x00bb1778, unsigned int 4202612, unsigned int 
2540968, long 2013534688) line 1078
nsAppShellService::Run(nsAppShellService * const 0x0026c5a8) line 458
main1(int 3, char * * 0x00262ed8, nsISupports * 0x00262f10) line 1456 + 9 bytes
main(int 3, char * * 0x00262ed8) line 1805 + 26 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x00133454, 
HINSTANCE__ * 0x00400000) line 1825 + 23 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e8d326()

infiniteloop:
    do {
        ds = gDeadScripts;
        
        if (hook)
        {
            /* tell the user this script has been destroyed */
#ifdef CAUTIOUS_SCRIPTHOOK
            JS_DISABLE_GC(rt);
#endif
            hook->OnScriptDestroyed (ds->script);
#ifdef CAUTIOUS_SCRIPTHOOK
            JS_ENABLE_GC(rt);
#endif
        }
        /* get next deleted script */
        gDeadScripts = NS_REINTERPRET_CAST(DeadScript *,
                                           PR_NEXT_LINK(&ds->links));
        /* take ourselves out of the circular list */
        PR_REMOVE_LINK(&ds->links);
        /* addref came from the FromPtr call in jsds_ScriptHookProc */
        NS_RELEASE(ds->script);
        /* free the struct! */
        PR_Free(ds);
    } while (&gDeadScripts->links != &ds->links);
    /* keep going until we catch up with our tail */
    gJsds->UnPause(nsnull);
    gDeadScripts = 0;
}

I set a breakpoint at     gJsds->UnPause(nsnull);
I haven't reached it.

This seems reasonable, because 
        PR_Free(ds);
    } while (&gDeadScripts->links != &ds->links);
is *very* strange.

Comment 1

[Robert Ginda]

>        PR_Free(ds);
>    } while (&gDeadScripts->links != &ds->links);
> is *very* strange.

It might look strange, but I don't understand why it would cause this problem. 
&ds->links is an address, which doesn't change when the allocation it happens to
point to is freed.

Maybe you could add something useful, like steps to reproduce?

This is probably a dupe of bug 155937.

Comment 2

[Robert Ginda]

Attachment: restate the loop to eliminate "strange code"

Comment 3

[Robert Ginda]

Attachment: no need to null out gDeadScripts at the bottom of the function anymore

now that we null out gDeadScripts inside the loop, we don't need to do it
before leaving the function.

I wonder if there was a case where we appended to gDeadScrips while it pointed
to garbage.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 90859 [details] [diff] [review]
no need to null out gDeadScripts at the bottom of the function anymore

Not sure what other code (on another thread?) might be looking at gDeadScripts
while it points to the PR_Free'ed memory that ds points at, but this seems like
an improvement anyway.

r/sr=[e-mail]

/be

Comment 5

[Robert Ginda]

peterv, can you r=?

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 90859 [details] [diff] [review]
no need to null out gDeadScripts at the bottom of the function anymore

r=peterv. Regarding brendan's comment, isn't (after the patch) gDeadScripts
always either null or pointing to ds->links so it never points to the
PR_Free'ed memory that ds points at?

Comment 7

[Brendan Eich [:brendan]]

peterv: yes, I was noting that.  My comment about "Note sure what other code..."
was about the old version of the code, prior to this patch (i.e., the top of
trunk version).  Now that I've rubbed my eyes, I see a big hazard in the old
code: it would call gJsds->UnPause(nsnull) before nulling gDeadScripts.  If
UnPause can call code that uses gDeadScripts, then blammo.

/be

Comment 8

[Robert Ginda]

UnPause doesn't do anything except decrement another global and set some hooks.
 OnScriptHookDestroyed calls out to xpconnect, but the only way that should be
able to modify gDeadScripts is if GC were enabled (which is isn't, because
CAUTIONS_SCRIPT_HOOK is always defined.)

The only time we should modify gDeadScripts is when a JSScript* is destroyed
while GC is running.  (See jsds_ScriptHook in jsd_xpc.cpp)

Comment 9

[Brendan Eich [:brendan]]

Weird.  Then I have no idea why the crashes occurred, but it still isn't good
form to leave a global dangling while calling out, even if you "know" that the
control flow won't reach a data dependency on that global before it is nulled.

/be

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 90859 [details] [diff] [review]
no need to null out gDeadScripts at the bottom of the function anymore

a=roc+moz for TRUNK

Comment 11

[Robert Ginda]

Checked in to trunk.  I'm going to hold off marking fixed for now.

Comment 12

[Robert Ginda]

no more reports, must be fixed :)