Bug 170391 - Implement PromiseDeferredTimer for non CF based ports
Summary: Implement PromiseDeferredTimer for non CF based ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 170390
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-02 10:04 PDT by Carlos Garcia Campos
Modified: 2017-04-05 00:58 PDT (History)
13 users (show)

See Also:


Attachments
Patch (3.44 KB, patch)
2017-04-02 10:13 PDT, Carlos Garcia Campos
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-04-02 10:04:35 PDT
RunLoop handling is only implemented for CF causing several wasm tests to fail for other ports:

wasm.yaml/wasm/js-api/Module-compile.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-compile-parallel.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-instantiate.js.default-wasm: ERROR: Unexpected exit code: 3
wasm.yaml/wasm/js-api/web-assembly-instantiate-parallel.js.default-wasm: ERROR: Unexpected exit code: 3
Comment 1 Carlos Garcia Campos 2017-04-02 10:13:45 PDT
Created attachment 306055 [details]
Patch
Comment 2 Michael Catanzaro 2017-04-02 11:05:58 PDT
Comment on attachment 306055 [details]
Patch

It looks OK to me, but I wonder if the separate implementation for CF is really necessary. If it really is, then I'd expect RunLoop to be implemented in terms of CFRunLoop....
Comment 3 Yusuke Suzuki 2017-04-02 17:40:30 PDT
Comment on attachment 306055 [details]
Patch

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

> Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp:-85
> +#if USE(CF)
>          CFRunLoopRun();
> -}

It's unnecessary since we already have the assertion that CFRunLoopGetCurrent() == m_runLoop.get(). So, RunLoop::run() drives current thread RunLoop and it is OK for all the ports.
Comment 4 Michael Catanzaro 2017-04-02 18:16:57 PDT
Comment on attachment 306055 [details]
Patch

OK then, please rewrite to get rid of CFRunLoop.
Comment 5 Yusuke Suzuki 2017-04-02 18:20:49 PDT
(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 306055 [details]
> Patch
> 
> OK then, please rewrite to get rid of CFRunLoop.

A bit worried thing is that, current RunLoop & RunLoop::Timer do not have the full features of CFRunLoop. For example, RunLoop is thread-safe, functions can be called from the other thread. But RunLoop::Timer is only available from the created thread. And current Heap's timers are touched from the other thread.

So, I think we still need to use CFRunLoop right now. But at least, in runRunLoop, we can just use RunLoop::run(). But for doWork function, we still need CFRunLoop thing.
Comment 6 Carlos Garcia Campos 2017-04-02 22:34:47 PDT
(In reply to Yusuke Suzuki from comment #5)
> (In reply to Michael Catanzaro from comment #4)
> > Comment on attachment 306055 [details]
> > Patch
> > 
> > OK then, please rewrite to get rid of CFRunLoop.
> 
> A bit worried thing is that, current RunLoop & RunLoop::Timer do not have
> the full features of CFRunLoop. For example, RunLoop is thread-safe,
> functions can be called from the other thread. But RunLoop::Timer is only
> available from the created thread. And current Heap's timers are touched
> from the other thread.
> 
> So, I think we still need to use CFRunLoop right now. But at least, in
> runRunLoop, we can just use RunLoop::run(). But for doWork function, we
> still need CFRunLoop thing.

Also note that iOS calls setRunLoop. Since CF timers already have a m_runLoop member I thought it was safer to use the member. I agree we could use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I think it's less confusing to have ifdefs for the run too.
Comment 7 Yusuke Suzuki 2017-04-02 22:38:56 PDT
(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Yusuke Suzuki from comment #5)
> > (In reply to Michael Catanzaro from comment #4)
> > > Comment on attachment 306055 [details]
> > > Patch
> > > 
> > > OK then, please rewrite to get rid of CFRunLoop.
> > 
> > A bit worried thing is that, current RunLoop & RunLoop::Timer do not have
> > the full features of CFRunLoop. For example, RunLoop is thread-safe,
> > functions can be called from the other thread. But RunLoop::Timer is only
> > available from the created thread. And current Heap's timers are touched
> > from the other thread.
> > 
> > So, I think we still need to use CFRunLoop right now. But at least, in
> > runRunLoop, we can just use RunLoop::run(). But for doWork function, we
> > still need CFRunLoop thing.
> 
> Also note that iOS calls setRunLoop. Since CF timers already have a
> m_runLoop member I thought it was safer to use the member. I agree we could
> use RunLoop::run(), but since we need to keep the CF ifdefs for the stop, I
> think it's less confusing to have ifdefs for the run too.

OK, we should change & drop all the CF things when we use RunLoop & RunLoop::Timer thing exclusively.
Eventually, we should change current JSRunLoopTimer to accept RunLoop instead of CFRunLoopRef thing.
Comment 8 Carlos Garcia Campos 2017-04-05 00:58:15 PDT
Committed r214936: <http://trac.webkit.org/changeset/214936>