Dedicated worker crash caused by global DFG worklists + GC

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r163786%20(2787)/fast/workers/dedicated-worker-lifecycle-crash-log.txt

[Filip Pizlo]

This allegedly began with:

http://trac.webkit.org/log/?verbose=on&rev=163691&stop_rev=163691

But the assertion failure is:

ASSERTION FAILED: m_totalBytesCopied <= m_storageSpace.size()

This implies a long-standing GC copying bug.

[Filip Pizlo]

Skipped in r163791

[Alexey Proskuryakov]

<rdar://problem/16045493>

[Mark Hahnenberg]

(In reply to comment #1)
> This allegedly began with:
> 
> http://trac.webkit.org/log/?verbose=on&rev=163691&stop_rev=163691
> 
> But the assertion failure is:
> 
> ASSERTION FAILED: m_totalBytesCopied <= m_storageSpace.size()
> 
> This implies a long-standing GC copying bug.

After some investigation, it appears that the process-global DFG worklists are causing objects to participate in the garbage collections of VMs other than the one they were allocated in. This started manifesting in the worker tests because they're one of the few WebKit tests that do multithreaded JS.

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #1)
> > This allegedly began with:
> > 
> > http://trac.webkit.org/log/?verbose=on&rev=163691&stop_rev=163691
> > 
> > But the assertion failure is:
> > 
> > ASSERTION FAILED: m_totalBytesCopied <= m_storageSpace.size()
> > 
> > This implies a long-standing GC copying bug.
> 
> After some investigation, it appears that the process-global DFG worklists are causing objects to participate in the garbage collections of VMs other than the one they were allocated in. This started manifesting in the worker tests because they're one of the few WebKit tests that do multithreaded JS.

Whoa!  That's pretty awesome.  So, the fix is just for the following methods:

    bool isActive() const { return !!m_plans.size(); }
    
    void visitChildren(SlotVisitor&, CodeBlockSet&); // Only called on the main thread after suspending all threads.

to filter based on VM.  That implies passing the VM& in each of them.

That also means that:

    void suspendAllThreads();
    void resumeAllThreads();

need to be smart enough to not allow "double suspension" of threads.  Or to handle it gracefully.  You could accomplish that super easily by having an internal Mutex that suspendAllThreads() locks before doing anything else, and resumeAllThreads() unlocks after.

[Mark Hahnenberg]

Created attachment 224574 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 224574 [details]
Patch

Clearing flags on attachment: 224574

Committed r164371: <http://trac.webkit.org/changeset/164371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Hahnenberg]

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