Summary: | REGRESSION: Reproducible crash after closing window after viewing css2.1/t0803-c5501-imrgn-t-00-b-ag.html | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | adele, ggaren, mjs, timothy | ||||||
Priority: | P1 | Keywords: | HasReduction, Regression | ||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2006-06-16 18:58:17 PDT
Trying a clean build. 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. 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.
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.
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. Created attachment 8876 [details] Patch v1 Patch v1 does what I said to do in Comment #5. (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. 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
Committed revision 14896. (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. 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. |