| Summary: | Potentially Unsafe HashSet of RuntimeObject* in RootObject definition | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||
| Component: | WebCore JavaScript | Assignee: | Michael Saboff <msaboff> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, alex, eric, ggaren, kevin, ossy, rhodes.unci, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Bug Depends on: | 53287, 53418 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
Created attachment 80391 [details]
Patch to use WeakGCMap instead of HashSet in RootObject
Committed r76893: <http://trac.webkit.org/changeset/76893> http://trac.webkit.org/changeset/76893 might have broken GTK Linux 64-bit Debug It made 460 tests crash on Qt debug bot: http://webkit.sed.hu/buildbot/results/x86-32%20Linux%20Qt%20Debug/r76893%20%2812399%29/results.html ASSERTION FAILED: m_runtimeObjects.get(object) (../../../Source/WebCore/bridge/runtime_root.cpp:189 void JSC::Bindings::RootObject::removeRuntimeObject(JSC::Bindings::RuntimeObject*)) Segmentation fault Comment on attachment 80391 [details]
Patch to use WeakGCMap instead of HashSet in RootObject
remove r+ from landed patch
It was rolled out: http://trac.webkit.org/changeset/76925 + ASSERT(m_runtimeObjects.get(object)); I think this needs to be "ASSERT(m_runtimeObjects.uncheckedGet(object))". By the time removeRuntimeObject is called, object is unmarked, so get will return 0. Created attachment 80478 [details]
Patch that addresses the test failures
Made the change suggested by Geoff and successfully ran all tests in debug mode.
Comment on attachment 80478 [details]
Patch that addresses the test failures
r=me
Committed r76969: <http://trac.webkit.org/changeset/76969> (In reply to comment #10) > Committed r76969: <http://trac.webkit.org/changeset/76969> Apparently this could be still causing issues at least in GTK and Leopard Intel Debug, first crash in the compilation after that commit: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r76969%20(26725)/results.html In Gtk+ we have the same assertions but they are flaky: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r77114%20(18774)/fast/frames/sandboxed-iframe-scripting-stderr.txt Unfourtunately I can not reproduce it locally in gtk+. I'm going to rollout until you have time to check it. *** Bug 53376 has been marked as a duplicate of this bug. *** I'm seeing crashes relating to this on the Snow Leopard Debug bot: http://build.webkit.org/results/SnowLeopard%20Intel%20Leaks/r77400%20(14522)/fast/events/tabindex-focus-blur-all-stderr.txt SUCCESS: Build 14472 (r77164) was the first to show failures: set([u'fast/events/tabindex-focus-blur-all.html']) *** Bug 53411 has been marked as a duplicate of this bug. *** |
As part of investigating another defect, I came across the definition of HashSet<RuntimeObject*> m_runtimeObjects; in RootOBject definition. Since RuntimeObject's are GC'ed, there is the possibility that a RuntimeObject could be GC'ed but not removed from this HashSet. Although I haven't determined a case where this is actually happening, it makes sense to make a preventative fix.