Bug 101015

Summary: [EFL][WK2] Use MutexLocker instead of lock()/unlock().
Product: WebKit Reporter: Byungwoo Lee <bw80.lee>
Component: WebKit EFLAssignee: Byungwoo Lee <bw80.lee>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, levin+threading, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98978    
Attachments:
Description Flags
Patch
none
Patch none

Byungwoo Lee
Reported 2012-11-01 22:29:44 PDT
Using MutexLocker will be better than using lock()/unlock() explicitly.
Attachments
Patch (4.12 KB, patch)
2012-11-02 00:06 PDT, Byungwoo Lee 		no flags
DetailsFormatted DiffDiff
Patch (3.81 KB, patch)
2012-11-02 00:33 PDT, Byungwoo Lee 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Byungwoo Lee
Comment 1 2012-11-02 00:06:18 PDT
Created attachment 171990 [details] Patch
Byungwoo Lee
Comment 2 2012-11-02 00:18:37 PDT
Comment on attachment 171990 [details] Patch Only performWork() and performTimerWork() should be changed. The locking scope of dispatchAfterDelay is discussing in Bug 98978.
Byungwoo Lee
Comment 3 2012-11-02 00:33:36 PDT
Created attachment 171997 [details] Patch
Kenneth Rohde Christiansen
Comment 4 2012-11-02 02:57:17 PDT
Comment on attachment 171997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171997&action=review > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:80 > + { > + MutexLocker locker(m_workItemQueueLock); > + if (m_workItemQueue.isEmpty()) > + return; > + > + m_workItemQueue.swap(workItemQueue); > + } Why do you need to lock if the queue is empty?
Byungwoo Lee
Comment 5 2012-11-02 03:02:38 PDT
(In reply to comment #4) > (From update of attachment 171997 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171997&action=review > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:80 > > + { > > + MutexLocker locker(m_workItemQueueLock); > > + if (m_workItemQueue.isEmpty()) > > + return; > > + > > + m_workItemQueue.swap(workItemQueue); > > + } > > Why do you need to lock if the queue is empty? Because m_workItemQueue is the shared resource that can be accessed on multiple thread at one time. I think protecting all the access to the shared resource will be safe.
Byungwoo Lee
Comment 6 2012-11-02 03:03:34 PDT
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 171997 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=171997&action=review > > > > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:80 > > > + { > > > + MutexLocker locker(m_workItemQueueLock); > > > + if (m_workItemQueue.isEmpty()) > > > + return; > > > + > > > + m_workItemQueue.swap(workItemQueue); > > > + } > > > > Why do you need to lock if the queue is empty? > > Because m_workItemQueue is the shared resource that can be accessed on multiple thread at one time. > > I think protecting all the access to the shared resource will be safe. (And the lock will be released when the function returns.)
WebKit Review Bot
Comment 7 2012-11-02 06:01:05 PDT
Comment on attachment 171997 [details] Patch Clearing flags on attachment: 171997 Committed r133289: <http://trac.webkit.org/changeset/133289>
WebKit Review Bot
Comment 8 2012-11-02 06:01:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.