Bug 55402 - loaderRunLoop() should use synchronization instead of while loop
Summary: loaderRunLoop() should use synchronization instead of while loop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-28 13:20 PST by Pratik Solanki
Modified: 2012-10-24 15:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-10-18 16:53 PDT, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2012-10-18 18:33 PDT, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (3.26 KB, patch)
2012-10-19 01:23 PDT, Jae Hyun Park
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2012-10-23 03:47 PDT, Jae Hyun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-02-28 13:20:39 PST
loaderRunLoop() creates a thread and calls sleep in a loop until the thread has started and set the loaderRunLoopObject static variable. We should use a ThreadCondition for signaling instead of a Sleep(10)
Comment 1 Jae Hyun Park 2012-10-18 16:53:08 PDT
Created attachment 169508 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-18 16:57:05 PDT
Attachment 169508 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:52:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Pratik Solanki 2012-10-18 17:36:28 PDT
Comment on attachment 169508 [details]
Patch

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

Thanks for working on this bug. I don't think its quite right though. Have you tested on a platform that uses this file i.e. Windows?

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:73
> -        while (!loaderRunLoopObject) {
> -            // FIXME: <http://webkit.org/b/55402> - loaderRunLoop() should use synchronization instead of while loop
> -#if PLATFORM(WIN)
> -            Sleep(10);
> -#else
> -            struct timespec sleepTime = { 0, 10 * 1000 * 1000 };
> -            nanosleep(&sleepTime, 0);
> -#endif
> -        }
> +        m_threadCondition.wait();

Does this even compile? ThreadCondition::wait() takes a Mutex& as an argument.
Comment 4 Jae Hyun Park 2012-10-18 18:33:13 PDT
Created attachment 169528 [details]
Patch
Comment 5 Jae Hyun Park 2012-10-18 18:36:00 PDT
(In reply to comment #3)
Thanks for the review.

> (From update of attachment 169508 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169508&action=review
> 
> Thanks for working on this bug. I don't think its quite right though. Have you tested on a platform that uses this file i.e. Windows?
No, I haven't. But is there a difference in using threading primitives according to the platforms?
> 
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:73
> > -        while (!loaderRunLoopObject) {
> > -            // FIXME: <http://webkit.org/b/55402> - loaderRunLoop() should use synchronization instead of while loop
> > -#if PLATFORM(WIN)
> > -            Sleep(10);
> > -#else
> > -            struct timespec sleepTime = { 0, 10 * 1000 * 1000 };
> > -            nanosleep(&sleepTime, 0);
> > -#endif
> > -        }
> > +        m_threadCondition.wait();
> 
> Does this even compile? ThreadCondition::wait() takes a Mutex& as an argument.
Fixed it.
Comment 6 WebKit Review Bot 2012-10-18 18:36:18 PDT
Attachment 169528 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:52:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Pratik Solanki 2012-10-18 19:58:53 PDT
(In reply to comment #5)
> (In reply to comment #3)
> Thanks for the review.
> 
> > (From update of attachment 169508 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169508&action=review
> > 
> > Thanks for working on this bug. I don't think its quite right though. Have you tested on a platform that uses this file i.e. Windows?
> No, I haven't. But is there a difference in using threading primitives according to the platforms?

I only ask because this file is not compiled on all platforms. It's only compiled on Windows and iOS. So I would have more confidence in the patch if the code compiled and ran on Windows.
Comment 8 Pratik Solanki 2012-10-18 21:17:52 PDT
Comment on attachment 169528 [details]
Patch

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

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.h:39
> +Mutex m_mutex;
> +ThreadCondition m_threadCondition;
> +

I think you can just make these static variables in LoaderRunLoopCF.cpp.
Comment 9 Jae Hyun Park 2012-10-19 01:23:10 PDT
Created attachment 169572 [details]
Patch
Comment 10 Jae Hyun Park 2012-10-19 01:23:49 PDT
(In reply to comment #8)
Thank you for your review.
> (From update of attachment 169528 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169528&action=review
> 
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.h:39
> > +Mutex m_mutex;
> > +ThreadCondition m_threadCondition;
> > +
> 
> I think you can just make these static variables in LoaderRunLoopCF.cpp.
Fixed it.
Comment 11 WebKit Review Bot 2012-10-19 01:25:18 PDT
Attachment 169572 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:54:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Alexey Proskuryakov 2012-10-19 08:42:06 PDT
Comment on attachment 169572 [details]
Patch

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

> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:41
> +static Mutex mutex;
> +static ThreadCondition threadCondition;

These have constructors and destructors, and would break Mac build if compiled it. I think that ports that use LoaderRunLoopCF.cpp also don't want that.
Comment 13 Pratik Solanki 2012-10-19 11:21:10 PDT
(In reply to comment #12)
> (From update of attachment 169572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169572&action=review
> 
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:41
> > +static Mutex mutex;
> > +static ThreadCondition threadCondition;
> 
> These have constructors and destructors, and would break Mac build if compiled it. I think that ports that use LoaderRunLoopCF.cpp also don't want that.

So is the solution to have static methods that return Mutex/ThreadCondition ? i.e. something like

static Mutex& loaderRunLoopMutex()
{
    static Mutex mutex;
    return mutex;
}
Comment 14 Pratik Solanki 2012-10-19 11:22:26 PDT
(In reply to comment #11)
> Attachment 169572 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:54:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I know this is in code that just moved around but you might as well fix this style error to keep the bot happy.
Comment 15 Jae Hyun Park 2012-10-23 03:47:14 PDT
Created attachment 170109 [details]
Patch
Comment 16 Jae Hyun Park 2012-10-23 03:47:53 PDT
(In reply to comment #12)
> (From update of attachment 169572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169572&action=review
> 
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:41
> > +static Mutex mutex;
> > +static ThreadCondition threadCondition;
> 
> These have constructors and destructors, and would break Mac build if compiled it. I think that ports that use LoaderRunLoopCF.cpp also don't want that.

Thanks for the review. I uploaded a new patch. Would this prevent the build break?
Comment 17 Jae Hyun Park 2012-10-23 03:48:17 PDT
(In reply to comment #14)
> (In reply to comment #11)
> > Attachment 169572 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> > Source/WebCore/platform/network/cf/LoaderRunLoopCF.cpp:54:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
> > Total errors found: 1 in 2 files
> > 
> > 
> > If any of these errors are false positives, please file a bug against check-webkit-style.
> 
> I know this is in code that just moved around but you might as well fix this style error to keep the bot happy.

Thanks for the review. I fixed it in the new patch.
Comment 18 Pratik Solanki 2012-10-24 13:39:36 PDT
Comment on attachment 170109 [details]
Patch

This looks good to me, though I am not a reviewer so I can't r+.
Comment 19 WebKit Review Bot 2012-10-24 15:05:40 PDT
Comment on attachment 170109 [details]
Patch

Rejecting attachment 170109 [details] from commit-queue.

jae.park@company100.net does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 20 WebKit Review Bot 2012-10-24 15:40:41 PDT
Comment on attachment 170109 [details]
Patch

Clearing flags on attachment: 170109

Committed r132412: <http://trac.webkit.org/changeset/132412>
Comment 21 WebKit Review Bot 2012-10-24 15:40:46 PDT
All reviewed patches have been landed.  Closing bug.