[webkit-reviews] review denied: [Bug 183604] When the WebContent process is blocked from accessing the WindowServer, the call CVDisplayLinkCreateWithCGDisplay will fail. : [Attachment 335736] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Per Arne Vollan
<pvollan at apple.com>'s request for review:
Bug 183604: When the WebContent process is blocked from accessing the
WindowServer, the call CVDisplayLinkCreateWithCGDisplay will fail.
https://bugs.webkit.org/show_bug.cgi?id=183604

Attachment 335736: Patch

https://bugs.webkit.org/attachment.cgi?id=335736&action=review




--- Comment #12 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 335736
  --> https://bugs.webkit.org/attachment.cgi?id=335736
Patch

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

I think this look good, but the mutex use in the new WebProcess code is
unnecessary. Please remove them and just ASSERT that they are always being
called on the MainThread.

>> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:79
>> +	LockHolder lock(mutex());
> 
> I'm not sure these locks are necessary in the WebContent process, since these
calls are being triggered by a message sent from the UIProcess. I suspect you
needed this previously because the displayLink could fire on some OS interval
outside the band of the main thread, but now we hit these on the main message
thread.
> 
> If we have performance problems, maybe we will need a dedicated (higher
priority) message queue, so that these get processed as soon after the display
update is requested as possible.

Yeah -- I just rebuilt with your patch, and we always hit this on the main
thread in response to messaging from the UIProcess.

So please add an ASSERT(isMainThread()); here, and remove the lock.

>> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:87
>> +	LockHolder lock(mutex());
> 
> I did some debugging, and I'm pretty sure we don't want these, since this is
all on a message thread in the WebContent process.
> 
> We needed the mutex here when the WindowServer was driving calls to this
method; but now this is happening in response to a message from the UIProcess,
so I do not believe multiple threads are hitting this code.

Again -- I just rebuilt with your patch, and we always hit this on the main
thread in response to messaging from the UIProcess.

So please add an ASSERT(isMainThread()); here, and remove the lock.