Summary: Use guaranteed RunLoop instead of RunLoop::current for dispatching inspector message Crashes seen under InspectorHeapAgent::didGarbageCollect attempting to dispatch RunLoop::Timer: * Crash Details Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000008 Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [0] Triggered by Thread: 12 Thread 12 name: WTF::AutomaticThread Thread 12 Crashed: 0 CFRunLoopAddTimer + 96 1 WTF::RunLoop::TimerBase::start(double, bool) + 188 2 WTF::RunLoop::TimerBase::start(double, bool) + 188 3 Inspector::InspectorHeapAgent::didGarbageCollect(JSC::CollectionScope) + 348 4 JSC::Heap::didFinishCollection(double) + 396 5 JSC::Heap::collectInThread() + 1276 6 JSC::Heap::Thread::work() + 16 In this case the RunLoop::current existed but then went away. Lets just use the RunLoop::main, this is for dispatching a very small JSON message.
<rdar://problem/30167791>
Created attachment 300296 [details] [PATCH] Proposed Fix
Comment on attachment 300296 [details] [PATCH] Proposed Fix Clearing flags on attachment: 300296 Committed r211486: <http://trac.webkit.org/changeset/211486>
All reviewed patches have been landed. Closing bug.
Rolled this change out in: https://trac.webkit.org/changeset/211666 This caused some follow-up issues. It doesn't make sense to call RunLoop::initializeMainRunLoop here. JSC's initialize threading can be called on non-main thread on Mac.
So it seems we can't be guaranteed that RunLoop::main exists in JavaScriptCore. Having a guaranteed RunLoop is only needed by the JSContext inspector path. So currently that only affects Mac, but I've seen patches bringing it up on other ports. A hacky solution would be adding RunLoop::initializeMainFromAnyThread for this path, but I really don't want to do that. Perhaps a better solution to this problem is to move off of RunLoops entirely, since we have no guaranteed run loop and probably don't want to keep one around. We are just trying to send garbage collection messages to the frontend at some convenient future time.
> Perhaps a better solution to this problem is to move off of RunLoops > entirely, since we have no guaranteed run loop and probably don't want to > keep one around. We are just trying to send garbage collection messages to > the frontend at some convenient future time. Ultimately I think this is the right approach. The idea of "send the collection messages a little bit later" is itself working around a case where sending a message immediately was had issues with the Sweeper. If that is the case, we just need to find the right time where it is safe to send the message, instead of the hacky Timer.
I'm unable to reproduce the original issue that caused us to need to delay the event anyways. I'll do a little more testing, but it seems we can just eliminate all of this complexity! I'm guessing this is much better with the concurrent GC.
Created attachment 300641 [details] [PATCH] Proposed Fix
Comment on attachment 300641 [details] [PATCH] Proposed Fix Attachment 300641 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3005754 Number of test failures exceeded the failure limit.
Created attachment 300645 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300641 [details] [PATCH] Proposed Fix Attachment 300641 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3005708 Number of test failures exceeded the failure limit.
Created attachment 300648 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 300641 [details] [PATCH] Proposed Fix Oh, that makes sense. WebKit1 will need some kind of "wait a beat".
Created attachment 300758 [details] [PATCH] Proposed Fix
Attachment 300758 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/WebHeapAgent.h:28: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300768 [details] [PATCH] Proposed Fix
Comment on attachment 300768 [details] [PATCH] Proposed Fix r=me
Comment on attachment 300768 [details] [PATCH] Proposed Fix Clearing flags on attachment: 300768 Committed r211771: <http://trac.webkit.org/changeset/211771>
(In reply to comment #19) > Comment on attachment 300768 [details] > [PATCH] Proposed Fix > > Clearing flags on attachment: 300768 > > Committed r211771: <http://trac.webkit.org/changeset/211771> This change appears to have caused two API tests to assert on ios-simulator: https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3257
(In reply to comment #21) > (In reply to comment #19) > > Comment on attachment 300768 [details] > > [PATCH] Proposed Fix > > > > Clearing flags on attachment: 300768 > > > > Committed r211771: <http://trac.webkit.org/changeset/211771> > > This change appears to have caused two API tests to assert on ios-simulator: > https://build.webkit.org/builders/ > Apple%20iOS%2010%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3257 These tests now have to initialize the main run loop. I'll update the tests.
Actually, I think its better here to update iOS WebKit initialization. Since that would solve all tests that use WebKitLegacy APIs. I'm actually surprised this wasn't done previously. See bug 167953