Bug 159075 - Web Inspector: CRASH in backend at Inspector::HeapFrontendDispatcher::garbageCollected + 552 when closing frontend/inspected page
Summary: Web Inspector: CRASH in backend at Inspector::HeapFrontendDispatcher::garbage...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on: 159105
Blocks:
  Show dependency tree / graph
 
Reported: 2016-06-23 16:06 PDT by BJ Burg
Modified: 2016-06-27 17:18 PDT (History)
11 users (show)

See Also:

Attachments
Proposed Fix (1.68 KB, patch)
2016-06-23 16:12 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix (6.96 KB, patch)
2016-06-24 11:06 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Proposed Fix v3 (7.56 KB, patch)
2016-06-24 16:57 PDT, BJ Burg 		no flags Details | Formatted Diff | Diff
Followup fix (1.81 KB, patch)
2016-06-27 13:46 PDT, BJ Burg 		fpizlo: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-06-23 16:06:02 PDT 
Very persistent-but-infrequent crashes ever since this was added. which makes sense.
Comment 1 BJ Burg 2016-06-23 16:06:16 PDT 
<rdar://problem/26094341>
Comment 2 BJ Burg 2016-06-23 16:12:09 PDT 
Created attachment 281938 [details]
Proposed Fix
Comment 3 Joseph Pecoraro 2016-06-23 19:27:25 PDT 
Comment on attachment 281938 [details]
Proposed Fix

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

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:297
> +        // The frontend could have gone away after the activity was scheduled.
> +        if (m_frontendDispatcher)

Is `this` valid? Could it be stale?
Comment 4 BJ Burg 2016-06-24 10:43:53 PDT 
Comment on attachment 281938 [details]
Proposed Fix

Yeah, you are right. I guess it needs to go the direction of CSSAgent's ChangeRegionOversetTask... blah.
Comment 5 BJ Burg 2016-06-24 11:06:16 PDT 
Created attachment 282001 [details]
Proposed Fix
Comment 6 Joseph Pecoraro 2016-06-24 13:05:56 PDT 
Comment on attachment 282001 [details]
Proposed Fix

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

r=me!

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:52
> +    RunLoop::Timer<SendGarbageCollectionEventsTask> m_timer;

I did not know there was a RunLoop::Timer! This is excellent.
Comment 7 WebKit Commit Bot 2016-06-24 13:30:19 PDT 
Comment on attachment 282001 [details]
Proposed Fix

Clearing flags on attachment: 282001

Committed r202443: <http://trac.webkit.org/changeset/202443>
Comment 8 WebKit Commit Bot 2016-06-24 13:30:23 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 9 Alexey Proskuryakov 2016-06-24 15:44:19 PDT 
This has introduced a number of memory corruption crashes, rolling out.

Will follow up with details in e-mail.
Comment 10 WebKit Commit Bot 2016-06-24 15:46:59 PDT 
Re-opened since this is blocked by bug 159105
Comment 11 BJ Burg 2016-06-24 16:57:43 PDT 
Created attachment 282033 [details]
Proposed Fix v3
Comment 12 BJ Burg 2016-06-27 10:47:25 PDT 
Comment on attachment 282033 [details]
Proposed Fix v3

Let's give this another try.
Comment 13 WebKit Commit Bot 2016-06-27 10:50:36 PDT 
Comment on attachment 282033 [details]
Proposed Fix v3

Clearing flags on attachment: 282033

Committed r202492: <http://trac.webkit.org/changeset/202492>
Comment 14 WebKit Commit Bot 2016-06-27 10:50:40 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 BJ Burg 2016-06-27 13:43:15 PDT 
Causing JSC stress tests to fail due to an (apparently not harmless) change introduced in the last patch. Will post a fix.
Comment 16 BJ Burg 2016-06-27 13:46:52 PDT 
Reopening to attach new patch.
Comment 17 BJ Burg 2016-06-27 13:46:54 PDT 
Created attachment 282165 [details]
Followup fix
Comment 18 BJ Burg 2016-06-27 14:52:26 PDT 
Landed followup in <https://trac.webkit.org/r202515>.
Comment 19 Joseph Pecoraro 2016-06-27 17:18:09 PDT 
Comment on attachment 282033 [details]
Proposed Fix v3

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

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:71
> +        std::lock_guard<Lock> lock(m_mutex);

Maybe we should use LockHolder instead of std::lock_guard.