| Summary: | Initialize the main RunLoop in iOS WebKitLegacy | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
| Component: | WebCore Misc. | Assignee: | Joseph Pecoraro <joepeck> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, bburg, commit-queue, joepeck | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Joseph Pecoraro
2017-02-07 13:19:47 PST
While this is under WebKitInitialize it is not inside it. Retitling. Created attachment 300843 [details]
[PATCH] Proposed Fix
Comment on attachment 300843 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=300843&action=review > Source/WebCore/platform/ios/wak/WebCoreThread.mm:655 > + RunLoop::initializeMainRunLoop(); Will RunLoop be confused because its initialization happens on the web thread, not the main one? Comment on attachment 300843 [details]
[PATCH] Proposed Fix
I believe you are correct. What concerns me is that there is an ASSERT(pthread_main_np()) that I added that doesn't fire, so now I'm rather confused.
I found my problem. I added the ASSERT in WTF and built WTF and WebCore but didn't build JavaScriptCore. So my assertion didn't actually materialize. Thanks for the careful review Created attachment 300851 [details]
[PATCH] Proposed Fix
Comment on attachment 300851 [details]
[PATCH] Proposed Fix
r=me, however I have another thought about this. Since apparently nothing uses the runloop with iOS WK1, wouldn't it be better to leave it uninitialized for a clear failure to occur if someone tries to use it?
As we initialize it to the actual main thread, it may become more likely that WebCore code will schedule something to run on this thread, which is a bad bad idea.
I suggest double checking with Anders about what he thinks. (In reply to comment #7) > Comment on attachment 300851 [details] > [PATCH] Proposed Fix > > r=me, however I have another thought about this. Since apparently nothing > uses the runloop with iOS WK1, wouldn't it be better to leave it > uninitialized for a clear failure to occur if someone tries to use it? > > As we initialize it to the actual main thread, it may become more likely > that WebCore code will schedule something to run on this thread, which is a > bad bad idea. There are already a few places in WebCore that use RunLoop::main, but as you point out in iOS WebKit1 they may want to be using the WebThread. Maybe we should assert when trying to use an uninitialized RunLoop instead. > There are already a few places in WebCore that use RunLoop::main, but as you > point out in iOS WebKit1 they may want to be using the WebThread. Filed Bug 168008. (In reply to comment #10) > Maybe we should assert when trying to use an uninitialized RunLoop instead. That is the assert that is already firing in a few TestAPI Tests in iso-simulator already. Comment on attachment 300851 [details] [PATCH] Proposed Fix Clearing flags on attachment: 300851 Committed r211890: <http://trac.webkit.org/changeset/211890> All reviewed patches have been landed. Closing bug. > > Maybe we should assert when trying to use an uninitialized RunLoop instead. > That is the assert that is already firing in a few TestAPI Tests in iso-simulator already. I'm confused. Does this landed change mean that it won't fire any more? (In reply to comment #15) > > > Maybe we should assert when trying to use an uninitialized RunLoop instead. > > > That is the assert that is already firing in a few TestAPI Tests in iso-simulator already. > > I'm confused. Does this landed change mean that it won't fire any more? Correct this fixes those assertions by ensuring we initialize on iOS. |