Bug 38604 - workers-gc2 crashing on Qt
Summary: workers-gc2 crashing on Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-05 13:30 PDT by Gavin Barraclough
Modified: 2010-05-05 23:18 PDT (History)
2 users (show)

See Also:


Attachments
The patch (9.49 KB, patch)
2010-05-05 13:40 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-05-05 13:30:22 PDT
This appears to be due to a couple of issues.
(1) When the atomic string table is deleted it does not clear the 'inTable' bit on AtomicStrings – it implicitly assumes that all AtomicStrings have already been deleted at this point (otherwise they will crash in their destructor when they try to remove themselves from the atomic string table).
(2) We don't fix the ordering in which WTF::WTFThreadData and WebCore::ThreadGlobalData are destructed.

We should make sure that ThreadGlobalData is cleaned up before worker threads terminate and WTF::WTFThreadData is destroyed, and we should clear the inTable bit of members on atomic string table destruction.
Comment 1 Gavin Barraclough 2010-05-05 13:40:08 PDT
Created attachment 55150 [details]
The patch
Comment 2 Darin Adler 2010-05-05 14:13:59 PDT
Comment on attachment 55150 [details]
The patch

> +        on AtomicStrings â it implicitly assumes that all AtomicStrings have already

You should stick to ASCII for ChangeLog.

> +    void setIsAtomic(bool isIdentifier)
> +    {
> +        ASSERT(!isStatic());
> +        if (isIdentifier)
> +            m_refCountAndFlags |= s_refCountFlagIsAtomic;
> +        else
> +            m_refCountAndFlags &= s_refCountFlagIsAtomic;
> +    }

Maybe a set/clear pair of functions instead of one that takes a bool would be better.
Comment 3 Darin Adler 2010-05-05 14:14:20 PDT
(In reply to comment #2)
> Maybe a set/clear pair of functions instead of one that takes a bool would be
> better.

Just to be clear, I’m not asking you to change this right away, just kind of “musing” about it.
Comment 4 Gavin Barraclough 2010-05-05 14:16:18 PDT
(In reply to comment #3)
> Just to be clear, I’m not asking you to change this right away, just kind of
> “musing” about it.

I agree - I did it this way for now to match the setIsIdentifier method.  I may switch to set/clear methods for both as a separate patch.
Comment 5 Gavin Barraclough 2010-05-05 18:12:22 PDT
This should be fixed in r58851.