Bug 34728 - REGRESSION (before r54472): Various tests in fast/workers are crashing on the buildbot.
Summary: REGRESSION (before r54472): Various tests in fast/workers are crashing on the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: David Levin
URL:
Keywords:
: 34731 (view as bug list)
Depends on: 33383
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-08 15:26 PST by David Levin
Modified: 2010-02-12 07:14 PST (History)
7 users (show)

See Also:


Attachments
the crash dump. (30.17 KB, text/plain)
2010-02-08 18:15 PST, David Levin
no flags Details
Patch (5.87 KB, patch)
2010-02-08 18:37 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2010-02-08 18:39 PST, David Levin
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2010-02-08 15:26:19 PST
Specifically, it usually seems to be
  fast/workers/*worker-replace-global-constructor.html 
but there have been others recently.

This is the bug for investigation/patches (cc'ed interested parties, but feel free to remove yourselves as well).
Comment 1 David Levin 2010-02-08 15:27:11 PST
Forgot to include this.

Current smallest repro steps (on my machine):
   run-webkit-tests LayoutTests/fast/transforms/ LayoutTests/fast/workers/
Comment 2 Eric Seidel (no email) 2010-02-08 16:41:37 PST
*** Bug 34731 has been marked as a duplicate of this bug. ***
Comment 3 Eric Seidel (no email) 2010-02-08 16:46:43 PST
As noted in the duplicate, we're hitting an ASSERT in debug builds:
ASSERTION FAILED: m_refCount > 0
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/RefCounted.h:68
bool WTF::RefCountedBase::derefBase())
Comment 4 Andrew Wilson 2010-02-08 16:52:28 PST
It looks like it started happening around 54472 (note the crash in shared-worker-redirect), which doesn't make any sense since it was an unrelated change.

And yet, it was green before that:

http://build.webkit.org/waterfall?last_time=1265580000
Comment 5 Eric Seidel (no email) 2010-02-08 16:54:01 PST
I looked through the /waterfall history and:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20(Tests)/builds/10226
from r54472 is the earliest crash I could find.
Comment 6 David Levin 2010-02-08 18:15:37 PST
Created attachment 48379 [details]
the crash dump.

Fix coming soon-ish.
Comment 7 David Levin 2010-02-08 18:37:58 PST
Created attachment 48380 [details]
Patch
Comment 8 Eric Seidel (no email) 2010-02-08 18:39:12 PST
Comment on attachment 48380 [details]
Patch

When did this actually start crashing?  do we know what change caused this?
Comment 9 David Levin 2010-02-08 18:39:55 PST
Created attachment 48381 [details]
Patch
Comment 10 David Levin 2010-02-08 19:07:58 PST
(In reply to comment #8)
> (From update of attachment 48380 [details])
> When did this actually start crashing?  do we know what change caused this?

My fix was based on looking at the crash and realizing that the object being deref'ed was just pretending to be ref counted, so I fixed that after talking to Gavin about it.

I didn't narrow don't the exact change that exposed this underlying issue (but I was guessing that it was http://trac.webkit.org/changeset/54400 -- I didn't confirm that in any way but I could attempt another build without just this one change to see if my guess is right).
Comment 11 Gavin Barraclough 2010-02-08 20:18:03 PST
Comment on attachment 48381 [details]
Patch

Looks awesome, thank you!
Comment 12 David Levin 2010-02-08 21:24:26 PST
Committed as http://trac.webkit.org/changeset/54525
Comment 13 David Levin 2010-02-08 21:45:59 PST
(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 48380 [details] [details])
> > When did this actually start crashing?  do we know what change caused this?
>
> I was guessing that it was http://trac.webkit.org/changeset/54400 

I started with a git branch that had the crash repro'ing. Then I rolled out r54400 locally. Did the build.
Ran the tests and they passed.

So the revision that started the issue for workers is confirmed if that is of help to folks.
Comment 14 Dmitry Titov 2010-02-08 23:07:16 PST
I wonder if we want to have an ASSERT like this:

    ~RefCountedBase()
    {
       ASSERT(m_deletionHasBegun);
    }

That would prevent deleting of the RefCounted objects other then via deref().  We do have cases of that, for example StyleBase is RefCounted but StyleList that derives from is deleted by 'delete'.