9476 2006-06-16 18:58:17 -0700 REGRESSION: Reproducible crash after closing window after viewing css2.1/t0803-c5501-imrgn-t-00-b-ag.html 2006-06-17 04:15:50 -0700 1 1 1 Unclassified WebKit JavaScriptCore 420+ Mac OS X 10.4 RESOLVED FIXED HasReduction, Regression P1 Normal --- 1 ddkilzer webkit-unassigned adele ggaren mjs timothy oldest_to_newest 46009 0 ddkilzer 2006-06-16 18:58:17 -0700 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 46010 1 ddkilzer 2006-06-16 19:08:19 -0700 Trying a clean build. 46011 2 timothy 2006-06-16 19:32:23 -0700 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. 46012 3 8875 ggaren 2006-06-16 20:54:27 -0700 Created attachment 8875 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. 46013 4 8875 ggaren 2006-06-16 21:03:00 -0700 Comment on attachment 8875 Speculative Patch That'll teach me to debug without a build. *p = q->next fixes up the previous item's next pointer. 46015 5 ddkilzer 2006-06-16 21:47:49 -0700 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. 46016 6 8876 ddkilzer 2006-06-16 21:57:36 -0700 Created attachment 8876 Patch v1 Patch v1 does what I said to do in Comment #5. 46020 7 ddkilzer 2006-06-16 23:04:59 -0700 (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. 46021 8 8876 ggaren 2006-06-16 23:25:10 -0700 Comment on attachment 8876 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 46064 9 ddkilzer 2006-06-17 03:43:30 -0700 Committed revision 14896. 46065 10 ddkilzer 2006-06-17 03:44:04 -0700 (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. 46075 11 ddkilzer 2006-06-17 04:15:50 -0700 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. 8875 2006-06-16 20:54:27 -0700 2006-06-16 21:57:36 -0700 Speculative Patch patch.txt text/plain 1044 ggaren SW5kZXg6IGRlYnVnZ2VyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBkZWJ1Z2dlci5jcHAJKHJldmlzaW9u IDE0ODY2KQorKysgZGVidWdnZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02OCwxOSArNjgsMjMg QEAKIAogdm9pZCBEZWJ1Z2dlcjo6ZGV0YWNoKEludGVycHJldGVyKiBpbnRlcnApCiB7Ci0gIGlm IChpbnRlcnAgJiYgaW50ZXJwLT5kZWJ1Z2dlcigpID09IHRoaXMpCisgIGlmIChpbnRlcnApIHsK KyAgICBBU1NFUlQoaW50ZXJwLT5kZWJ1Z2dlcigpID09IHRoaXMpOwogICAgIGludGVycC0+c2V0 RGVidWdnZXIoMCk7CisgIH0KIAotICAvLyBpdGVyYXRlIHRoZSBhZGRyZXNzZXMgd2hlcmUgQXR0 YWNoZWRJbnRlcnByZXRlciBwb2ludGVycyBhcmUgc3RvcmVkCi0gIC8vIHNvIHdlIGNhbiB1bmxp bmsgaXRlbXMgZnJvbSB0aGUgbGlzdAotICBBdHRhY2hlZEludGVycHJldGVyICoqcCA9ICZyZXAt PmludGVycHM7Ci0gIEF0dGFjaGVkSW50ZXJwcmV0ZXIgKnE7Ci0gIHdoaWxlICgocSA9ICpwKSkg ewotICAgIGlmICghaW50ZXJwIHx8IHEtPmludGVycCA9PSBpbnRlcnApIHsKLSAgICAgICpwID0g cS0+bmV4dDsKLSAgICAgIGRlbGV0ZSBxOworICBBdHRhY2hlZEludGVycHJldGVyKiBjdXJyZW50 ID0gcmVwLT5pbnRlcnBzOworICBBdHRhY2hlZEludGVycHJldGVyKiBwcmV2ID0gMDsKKyAgd2hp bGUgKGN1cnJlbnQpIHsKKyAgICBpZiAoIWludGVycCB8fCBwLT5pbnRlcnAgPT0gaW50ZXJwKSB7 CisgICAgICBBdHRhY2hlZEludGVycHJldGVyKiB0bXAgPSBjdXJyZW50LT5uZXh0OworICAgICAg ZGVsZXRlIGN1cnJlbnQ7CisgICAgICBjdXJyZW50ID0gdG1wOworICAgICAgaWYgKHByZXYpCisg ICAgICAgICAgcHJldi0+bmV4dCA9IHRtcDsKICAgICB9IGVsc2UgewotICAgICAgcCA9ICZxLT5u ZXh0OworICAgICAgcHJldiA9IGN1cnJlbnQ7CisgICAgICBjdXJyZW50ID0gY3VycmVudC0+bmV4 dDsKICAgICB9CiAgIH0KIH0K 8876 2006-06-16 21:57:36 -0700 2006-06-16 23:25:10 -0700 Patch v1 bug-9476-v1.diff text/plain 1505 ddkilzer SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0ODk1KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMDYtMDYtMTYgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGtpbHplci5uZXQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgaHR0cDovL2J1Z3ppbGxhLm9wZW5kYXJ3aW4ub3JnL3Nob3df YnVnLmNnaT9pZD05NDc2CisgICAgICAgIFJFR1JFU1NJT046IFJlcHJvZHVjaWJsZSBjcmFzaCBh ZnRlciBjbG9zaW5nIHdpbmRvdyBhZnRlciB2aWV3aW5nCisgICAgICAgIGNzczIuMS90MDgwMy1j NTUwMS1pbXJnbi10LTAwLWItYWcuaHRtbAorCisgICAgICAgICoga2pzL2RlYnVnZ2VyLmNwcDoK KyAgICAgICAgKERlYnVnZ2VyOjpkZXRhY2gpOiBDYWxsIHNldERlYnVnZ2VyKDApIGZvciBhbGwg ZGVsZXRlZCBpbnRlcnByZXRlcnMuCisKIDIwMDYtMDYtMTcgIEFuZGVycyBDYXJsc3NvbiAgPGFj YXJsc3NvbkBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgTWFjaWVqIGFuZCBHZW9m Zi4KSW5kZXg6IEphdmFTY3JpcHRDb3JlL2tqcy9kZWJ1Z2dlci5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g SmF2YVNjcmlwdENvcmUva2pzL2RlYnVnZ2VyLmNwcAkocmV2aXNpb24gMTQ4OTUpCisrKyBKYXZh U2NyaXB0Q29yZS9ranMvZGVidWdnZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02OCw5ICs2OCw2 IEBAIHZvaWQgRGVidWdnZXI6OmF0dGFjaChJbnRlcnByZXRlciogaW50ZXIKIAogdm9pZCBEZWJ1 Z2dlcjo6ZGV0YWNoKEludGVycHJldGVyKiBpbnRlcnApCiB7Ci0gIGlmIChpbnRlcnAgJiYgaW50 ZXJwLT5kZWJ1Z2dlcigpID09IHRoaXMpCi0gICAgaW50ZXJwLT5zZXREZWJ1Z2dlcigwKTsKLQog ICAvLyBpdGVyYXRlIHRoZSBhZGRyZXNzZXMgd2hlcmUgQXR0YWNoZWRJbnRlcnByZXRlciBwb2lu dGVycyBhcmUgc3RvcmVkCiAgIC8vIHNvIHdlIGNhbiB1bmxpbmsgaXRlbXMgZnJvbSB0aGUgbGlz dAogICBBdHRhY2hlZEludGVycHJldGVyICoqcCA9ICZyZXAtPmludGVycHM7CkBAIC03OCwxMCAr NzUsMTAgQEAgdm9pZCBEZWJ1Z2dlcjo6ZGV0YWNoKEludGVycHJldGVyKiBpbnRlcgogICB3aGls ZSAoKHEgPSAqcCkpIHsKICAgICBpZiAoIWludGVycCB8fCBxLT5pbnRlcnAgPT0gaW50ZXJwKSB7 CiAgICAgICAqcCA9IHEtPm5leHQ7CisgICAgICBxLT5pbnRlcnAtPnNldERlYnVnZ2VyKDApOwog ICAgICAgZGVsZXRlIHE7Ci0gICAgfSBlbHNlIHsKKyAgICB9IGVsc2UKICAgICAgIHAgPSAmcS0+ bmV4dDsKLSAgICB9CiAgIH0KIH0KIAo=