loaderRunLoop() should use synchronization instead of while loop

• Component: Platform
• Status: RESOLVED FIXED

[Pratik Solanki]

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)

[Jae Hyun Park]

Created attachment 169508 [details]
Patch

[WebKit Review Bot]

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.

[Pratik Solanki]

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.

[Jae Hyun Park]

Created attachment 169528 [details]
Patch

[Jae Hyun Park]

(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.

[WebKit Review Bot]

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.

[Pratik Solanki]

(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.

[Pratik Solanki]

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.

[Jae Hyun Park]

Created attachment 169572 [details]
Patch

[Jae Hyun Park]

(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.

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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.

[Pratik Solanki]

(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;
}

[Pratik Solanki]

(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.

[Jae Hyun Park]

Created attachment 170109 [details]
Patch

[Jae Hyun Park]

(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?

[Jae Hyun Park]

(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.

[Pratik Solanki]

Comment on attachment 170109 [details]
Patch

This looks good to me, though I am not a reviewer so I can't r+.

[WebKit Review Bot]

Comment on attachment 170109 [details]
Patch

Rejecting attachment 170109 [details] from commit-queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 170109 [details]
Patch

Clearing flags on attachment: 170109

Committed r132412: <http://trac.webkit.org/changeset/132412>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.