Bug 9787 – fast/frames tests failing (bad pointer to owner element) under MallocScribble

• Component: Frames
• Status: RESOLVED FIXED

[Darin Adler]

Since the recent frame changes, I've seen an intermittent crash that I've been able to reproduce with 1 or 2 runs when running the fast/frames tests.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x555555cd
0x013997cc in WebCore::RenderPart::setFrame (this=0x55555555, frame=0x0) at /Safari/OpenSource/WebCore/rendering/RenderPart.cpp:66
66          if (frame == m_frame)
(gdb) bt
#0  0x013997cc in WebCore::RenderPart::setFrame (this=0x55555555, frame=0x0) at /Safari/OpenSource/WebCore/rendering/RenderPart.cpp:66
#1  0x01128f04 in WebCore::Frame::~Frame (this=0x18926660) at /Safari/OpenSource/WebCore/page/Frame.cpp:210
#2  0x0113e728 in WebCore::FrameMac::~FrameMac (this=0x18926660) at /Safari/OpenSource/WebCore/bridge/mac/FrameMac.mm:175
#3  0x0148dcec in WebCore::Shared<WebCore::Frame>::deref (this=0x18926664) at /Safari/OpenSource/WebCore/platform/Shared.h:32
#4  0x01127dbc in WebCore::Frame::lifeSupportTimerFired (this=0x18926660) at /Safari/OpenSource/WebCore/page/Frame.cpp:2510
#5  0x014e3070 in WebCore::Timer<WebCore::Frame>::fired (this=0x1498e7d0) at /Safari/OpenSource/WebCore/platform/Timer.h:94
#6  0x012d8020 in WebCore::TimerBase::fireTimers (fireTime=1152371018.6822951, firingTimers=@0xbfffe620) at /Safari/OpenSource/WebCore/platform/Timer.cpp:335
#7  0x012d80ec in WebCore::TimerBase::sharedTimerFired () at /Safari/OpenSource/WebCore/platform/Timer.cpp:352
#8  0x012d7498 in WebCore::timerFired () at /Safari/OpenSource/WebCore/platform/mac/SharedTimerMac.cpp:46
#9  0x907ef550 in __CFRunLoopDoTimer ()
#10 0x907dbec8 in __CFRunLoopRun ()
#11 0x907db47c in CFRunLoopRunSpecific ()
#12 0x92953164 in -[NSRunLoop runMode:beforeDate:] ()
#13 0x0000ae08 in dumpRenderTree (pathOrURL=0xbfffeeb8 "empty-cols-attribute.html") at /Safari/OpenSource/WebKitTools/DumpRenderTree/DumpRenderTree.m:756
#14 0x00007ed8 in main (argc=2, argv=0xbffff7e4) at /Safari/OpenSource/WebKitTools/DumpRenderTree/DumpRenderTree.m:321

To reproduce, I created a file with the names of the tests:

    cd LayoutTests/fast/frames
    ls -1 *.html > tests.txt
    gdb <build dir>/Debug/DumpRenderTree
    set env MallocScribble 1
    run - < tests.txt
    <then run and run over again until you see the crash>

The problem is that a Frame ends up with a pointer to an element that has been deallocated. It's critical that we fix this -- this is the kind of thing that really kills us in WebKit releases.

We need to clean up the object ownership rules between the owner element and the frame. The concept is that the frame holds a pointer to the owner element that's guaranteed to get cleared if the element goes away before the frame does, and it seems this invariant has been broken.

More importantly, this code is getting too loosely connected and fragile with the responsibilities for lifetime being split between the frame element classes, the frame class, and the renderer for the part.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

There may be some relevant crash logs in Bug 9784.

[Anders Carlsson]

Created attachment 9277 [details]
Patch

I reworked things a bit so that RenderPart doesn't have to know about the frame. Instead, the elements reset the owner element in ::detach.

With this patch I can't reproduce the crash.

[Darin Adler]

Comment on attachment 9277 [details]
Patch

The line of code in Frame::~Frame that says d->m_ownerElement = 0 doesn't seem necessary. We're destroying the object and we don't need to nil out fields in it.

The code should not use renderer->element() -- it should use the more efficient and more sensibly named node() function.

Are you certain there's no code path where the frame element gets destroyed without a close call? For example, what about when a whole page and frame tree goes away at once. Same question about the other elements.

It seems a little dangerous to me that the frame has a pointer to the owner element, and it's not ref'd. And in the other direction, the owner element has only an indirect memory of the frame -- a frame name. Can these two ever get out of sync? Because if they do, then the Frame has a bad pointer in it.

The patch looks fine to me, but I'm still concerned that the object ownership relationship is too fragile.

I'm going to say review+ because this is definitely an improvement.

[Maciej Stachowiak]

I made the two specific tweaks Darin suggested and landed this, to get the buildbot happy again. I agree that it would be good (in a follow-on patch) to clear up the ownership policy.

[Timothy Hatcher]

<rdar://problem/4618213>