Bug 167683 – Web Inspector: Do not use RunLoop when dispatching inspector GC event

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<rdar://problem/30167791>

[Joseph Pecoraro]

Created attachment 300296 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 300296 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 300296

Committed r211486: <http://trac.webkit.org/changeset/211486>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 300641 [details]
[PATCH] Proposed Fix

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Joseph Pecoraro]

Comment on attachment 300641 [details]
[PATCH] Proposed Fix

Oh, that makes sense. WebKit1 will need some kind of "wait a beat".

[Joseph Pecoraro]

Created attachment 300758 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

Created attachment 300768 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 300768 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 300768 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 300768

Committed r211771: <http://trac.webkit.org/changeset/211771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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