Bug 9476 – REGRESSION: Reproducible crash after closing window after viewing css2.1/t0803-c5501-imrgn-t-00-b-ag.html

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

I believe this may be a regression from Bug 7080.

Steps to Reproduce:

1. Open WebKit+Safari.
2. Open LayoutTests/css2.1/t0803-c5501-imrgn-t-00-b-ag.html
3. Close the browser window.
4. WebKit+Safari crashes.

Relevant part of stack trace:

Date/Time:      2006-06-16 20:51:52.614 -0500
OS Version:     10.4.6 (Build 8I127)
Report Version: 4

Command: Safari
Path:    /Applications/Safari.app/Contents/MacOS/Safari
Parent:  bash [263]

Version:        2.0.3 (417.9.3)
Build Version:  2
Project Name:   WebBrowser
Source Version: 4170903

PID:    25147
Thread: 0

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x6f6e546f

Thread 0 Crashed:
0   com.apple.JavaScriptCore       	0x0101578c KJS::Debugger::detach(KJS::Interpreter*) + 140 (debugger.cpp:79)
1   com.apple.JavaScriptCore       	0x01023c40 KJS::Interpreter::~Interpreter [not-in-charge]() + 240 (interpreter.cpp:264)
2   com.apple.WebCore              	0x01a6b694 KJS::ScriptInterpreter::~ScriptInterpreter [in-charge deleting]() + 64 (kjs_binding.cpp:75)
3   com.apple.WebCore              	0x01a894a4 WebCore::KJSProxy::~KJSProxy [in-charge]() + 92 (kjs_proxy.cpp:48)
4   com.apple.WebCore              	0x01c02644 WebCore::FramePrivate::~FramePrivate [in-charge]() + 116 (FramePrivate.h:112)
5   com.apple.WebCore              	0x01869e0c WebCore::Frame::~Frame [not-in-charge]() + 1076 (Frame.cpp:221)
6   com.apple.WebCore              	0x0187f120 WebCore::FrameMac::~FrameMac [in-charge deleting]() + 248 (FrameMac.mm:173)
7   com.apple.WebCore              	0x01bae92c WebCore::Shared<WebCore::Frame>::deref() + 144 (Shared.h:32)
8   com.apple.WebCore              	0x01bae980 WTF::RefPtr<WebCore::Frame>::~RefPtr [in-charge]() + 64 (RefPtr.h:41)
9   com.apple.WebCore              	0x0198ecc4 WebCore::Page::~Page [in-charge]() + 340 (Page.cpp:63)
10  com.apple.WebCore              	0x018af8f0 -[WebCorePageBridge dealloc] + 64 (WebCorePageBridge.mm:83)
11  com.apple.WebKit               	0x0039811c -[WebView(WebPrivate) _close] + 224 (WebView.m:575)
12  com.apple.Safari               	0x00047d2c 0x1000 + 290092
13  com.apple.Foundation           	0x9297d5e8 -[NSArray makeObjectsPerformSelector:withObject:] + 264
14  com.apple.Safari               	0x0005c608 0x1000 + 374280
15  com.apple.Safari               	0x0005a9c8 0x1000 + 367048
16  com.apple.Foundation           	0x92975ad8 _nsnote_callback + 180
17  com.apple.CoreFoundation       	0x9080b010 __CFXNotificationPost + 368
18  com.apple.CoreFoundation       	0x908030ec _CFXNotificationPostNotification + 684
19  com.apple.Foundation           	0x9295fee0 -[NSNotificationCenter postNotificationName:object:userInfo:] + 92
20  com.apple.AppKit               	0x937c1820 -[NSWindow _close] + 100
21  com.apple.AppKit               	0x937c1784 -[NSWindow close] + 36
22  com.apple.Safari               	0x0005a96c 0x1000 + 366956
23  com.apple.Safari               	0x0005c498 0x1000 + 373912
24  com.apple.AppKit               	0x937c0ff0 -[NSApplication sendAction:to:from:] + 108
25  com.apple.Safari               	0x00029adc 0x1000 + 166620
26  com.apple.AppKit               	0x937c0f24 -[NSControl sendAction:to:] + 96
27  com.apple.AppKit               	0x937c0e04 -[NSCell _sendActionFrom:] + 156
28  com.apple.AppKit               	0x937c08e4 -[NSButtonCell performClick:] + 472
29  com.apple.AppKit               	0x937c0ff0 -[NSApplication sendAction:to:from:] + 108
30  com.apple.Safari               	0x00029adc 0x1000 + 166620
31  com.apple.AppKit               	0x9381b838 -[NSMenu performActionForItemAtIndex:] + 392
32  com.apple.AppKit               	0x9381b5bc -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:] + 104
33  com.apple.AppKit               	0x9381b064 -[NSMenu performKeyEquivalent:] + 272
34  com.apple.AppKit               	0x9381acb0 -[NSApplication _handleKeyEquivalent:] + 328
35  com.apple.AppKit               	0x937247a8 -[NSApplication sendEvent:] + 2944
36  com.apple.Safari               	0x000217a8 0x1000 + 133032
37  com.apple.AppKit               	0x9371c0b0 -[NSApplication run] + 508
38  com.apple.AppKit               	0x9380cbfc NSApplicationMain + 452
39  com.apple.Safari               	0x0005cb98 0x1000 + 375704
40  com.apple.Safari               	0x0005ca40 0x1000 + 375360

[David Kilzer (:ddkilzer)]

Trying a clean build.

[Timothy Hatcher]

This code is now being exercised by my debugger change in r14890. Debug builds attach a debugger that can be accessed from another process if needed.

[Geoffrey Garen]

Created attachment 8875 [details]
Speculative Patch

We think the problem is that the code deletes an item in a linked list without fixing up the previous item's next pointer. Speculative patch attached.

[Geoffrey Garen]

Comment on attachment 8875 [details]
Speculative Patch

That'll teach me to debug without a build. *p = q->next fixes up the previous item's next pointer.

[David Kilzer (:ddkilzer)]

For my simple test case in Comment #0, what's happening is that KJS::Debugger::~Debugger() gets called *first* which calls detach(0), then KJS::Interpreter::~Interpreter() gets called *second* which calls detach(this), which by that time has already had its debugger detached (and thus bad pointers are dereferenced).

The while() loop in detach() doesn't call setDebugger(0) on every Debugger it destroys, so some Interpreters will think they still need to destroy theirs.

The solution is to call setDebugger(0) on every Interpreter that is destroyed regardless of whether it's "ours" or not.  The initial code in detach() is therefore no longer needed.

[David Kilzer (:ddkilzer)]

Created attachment 8876 [details]
Patch v1

Patch v1 does what I said to do in Comment #5.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Patch v1 does what I said to do in Comment #5.

All tests pass except as noted in Bug 9477 and Bug 9478, which are different causes.

[Geoffrey Garen]

Comment on attachment 8876 [details]
Patch v1

The word "interpreter" is a little confused here. We're not deleting interpreters. Rather, we're deleting nodes in a linked list of interpreters, leaving the interpreters themselves around.

A better description might be, "Call setDebugger(0) for all interpreters removed from the 'attached to a debugger' list."

Anyway, patch=good, r=me

[David Kilzer (:ddkilzer)]

Committed revision 14896.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> A better description might be, "Call setDebugger(0) for all interpreters
> removed from the 'attached to a debugger' list."

FWIW, I fixed up the comment in the ChangeLog to the above before I committed.

[David Kilzer (:ddkilzer)]

Something was bothering me about KJS::Debugger::detach()--if the code always updated the linked list (whose head was 'rep->interps') correctly, then why did the second call to detach fail with a bad pointer dereference?

The answer is that KJS::Debugger::~Debugger() deletes 'rep' itself after calling detach(0), thus '&rep->interps' will point to something invalid the next time detach() is called.