Bug 160160 – [Threaded Compositor] Crashes and deadlocks in single web process mode

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Every WebPage has its own threaded compositor that runs its own compositing thread. That means that when there's more than one WebPage in the same process, we are running OpenGL stuff in different secondary threads. That's causing crashes and deadlocks in X and graphics drivers. We should ensure there's a single compositing thread per process. This is what is causing unit test WebKit2.WKPageGetScaleFactorNotZero to time out since we switched to the threaded compositor. That test is creating two pages in the same web process, and most of the times the web process crashes or deadlocks causing the test to never finish and time out.

[Carlos Garcia Campos]

Created attachment 284470 [details]
Patch

[Carlos Garcia Campos]

Created attachment 284472 [details]
Updated patch

I forgot to detach the scene on invalidate.

[Carlos Garcia Campos]

Created attachment 284476 [details]
New patch

I've realized of an issue with the previous patch, the sync tasks should be sync for the threaded compositor, not for the compositing run loop. After moving the performSync method to the ThreadedCompositor class the CompositingRunLoop class became a very simple RunLoop wrapper with similar code to what a WorkQueue is. so this patch removes CompositingRunLoop in favor of using a global WorkQueue.

[WebKit Commit Bot]

Attachment 284476 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Any additional information on why this is occurring in the first place? 

I don't think CompositingRunLoop should be removed, instead it should be made a singleton for now, and later it could be turned into a thread pool.

[Carlos Garcia Campos]

(In reply to comment #5)
> Any additional information on why this is occurring in the first place? 

I don't think we can use X and most of the graphics drivers from different processes.

> I don't think CompositingRunLoop should be removed, instead it should be
> made a singleton for now, and later it could be turned into a thread pool.

That's what my previous patch did, converted it into a singleton, but after moving the performSync to the threaded compositor the class became just a wrapper around RunLoop in a separate thread which is what WorkQueue does in the end.

[Carlos Garcia Campos]

I would do it the other way around, let's bring it back if it's really possible to use multiple threads at some point, because I don't think that's going to happen.

[Zan Dobersek]

(In reply to comment #7)
> I would do it the other way around, let's bring it back if it's really
> possible to use multiple threads at some point, because I don't think that's
> going to happen.

It's possible with non-X11 setups.

[Zan Dobersek]

(In reply to comment #6)
> (In reply to comment #5)
> > Any additional information on why this is occurring in the first place? 
> 
> I don't think we can use X and most of the graphics drivers from different
> processes.
> 

I don't understand this.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Any additional information on why this is occurring in the first place? 
> > 
> > I don't think we can use X and most of the graphics drivers from different
> > processes.
> > 
> 
> I don't understand this.

That I think we will never be able to use multiple threads, because X and graphics drivers don't really support it. But if you say it's supported by non X platforms, I'll go back to previous approach keeping the CompositingRunLoop because I need this fixed.

[Carlos Garcia Campos]

Created attachment 284483 [details]
New patch

Bring back the compositing run loop class, making it a thread pool with a limit of 1 thread by default.

[WebKit Commit Bot]

Attachment 284483 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 284569 [details]
Another approach

Updated the patch after talking to Zan using a slightly different approach.

[WebKit Commit Bot]

Attachment 284569 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:49:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 284569 [details]
Another approach

View in context: https://bugs.webkit.org/attachment.cgi?id=284569&action=review

Looks good, thanks for reiterating on this.

> Source/WebKit2/Shared/CoordinatedGraphics/threadedcompositor/CompositingRunLoop.cpp:94
> +    WorkQueue& getOrCreateWorkQueueForContext(void* context)
> +    {
> +        auto addResult = m_workQueueMap.add(context, nullptr);
> +        if (addResult.isNewEntry) {
> +            if (m_threadCount >= m_threadCountLimit) {
> +                RELEASE_ASSERT(m_sharedWorkQueue);
> +                addResult.iterator->value = m_sharedWorkQueue;
> +            } else {
> +                addResult.iterator->value = WorkQueue::create("org.webkit.ThreadedCompositorWorkQueue");
> +                if (!m_threadCount)
> +                    m_sharedWorkQueue = addResult.iterator->value;
> +                m_threadCount++;
> +            }
> +        }

This is OK for now, and it works for a single-thread limit. But for configurations where more (but not unlimited) threads could be used, one option would be to use a HashSet here and disperse the contexts across the available threads. But that's for the future.

[Carlos Garcia Campos]

Committed r203718: <http://trac.webkit.org/changeset/203718>