Bug 170391 – Implement PromiseDeferredTimer for non CF based ports

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 306055 [details]
Patch

[Michael Catanzaro]

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

[Yusuke Suzuki]

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.

[Michael Catanzaro]

Comment on attachment 306055 [details]
Patch

OK then, please rewrite to get rid of CFRunLoop.

[Yusuke Suzuki]

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

[Carlos Garcia Campos]

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

[Yusuke Suzuki]

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

[Carlos Garcia Campos]

Committed r214936: <http://trac.webkit.org/changeset/214936>