Bug 53271 – Potentially Unsafe HashSet of RuntimeObject* in RootObject definition

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 80391 [details]
Patch to use WeakGCMap instead of HashSet in RootObject

[Michael Saboff]

Committed r76893: <http://trac.webkit.org/changeset/76893>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76893 might have broken GTK Linux 64-bit Debug

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Comment on attachment 80391 [details]
Patch to use WeakGCMap instead of HashSet in RootObject

remove r+ from landed patch

[Csaba Osztrogonác]

It was rolled out: http://trac.webkit.org/changeset/76925

[Geoffrey Garen]

+ 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.

[Michael Saboff]

Created attachment 80478 [details]
Patch that addresses the test failures

Made the change suggested by Geoff and successfully ran all tests in debug mode.

[Geoffrey Garen]

Comment on attachment 80478 [details]
Patch that addresses the test failures

r=me

[Michael Saboff]

Committed r76969: <http://trac.webkit.org/changeset/76969>

[Alejandro G. Castro]

(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.

[Michael Saboff]

*** Bug 53376 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

SUCCESS: Build 14472 (r77164) was the first to show failures: set([u'fast/events/tabindex-focus-blur-all.html'])

[Gavin Barraclough]

*** Bug 53411 has been marked as a duplicate of this bug. ***