Bug 195322 - REGRESSION(r236862): early frame decoupling leaves JSC ArrayBuffer objects lingering
Summary: REGRESSION(r236862): early frame decoupling leaves JSC ArrayBuffer objects li...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 01:52 PST by Zan Dobersek
Modified: 2019-03-18 14:28 PDT (History)
11 users (show)

See Also:


Attachments
Heap logging patch (1.90 KB, patch)
2019-03-05 01:57 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
EWS run (1.23 KB, patch)
2019-03-15 07:50 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (2.10 KB, patch)
2019-03-18 13:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2019-03-05 01:52:55 PST
With frame detachment changes made in r236862, it seems JSC resources can persist beyond frame destruction.
https://trac.webkit.org/changeset/236862/webkit

The observed case is the one of ArrayBuffer resources that are kept alive during the JetStream benchmark execution. After r236862, at the end of a JetStream run there can be about 25 ArrayBuffer objects still referenced in the JSC heap and thus deferred for allocation through the GCIncomingRefCountedSet mechanism. This totals to over 700MB of extra memory at the end of a JetStream run. Before r236862 these ArrayBuffer objects were properly cleaned up during the run with only one or two objects remaining alive at the end of the run due to being used in the last few test cases, totalling ~150MB in size.

In r236862 specifically, the problem is a side effect of the early decoupling from the Frame object in DOMWindow::willDetachDocumentFromFrame(). This was further reorganized in later changes, but the problem remains present and can be observed on both Mac and Linux ports.
Comment 1 Zan Dobersek 2019-03-05 01:57:20 PST
Created attachment 363624 [details]
Heap logging patch

This patch adds more detailed dumps of extra memory statistics during GC, and provides a rundown of to-be-swept ArrayBuffer objects contained in GCIncomingRefCountedSet<ArrayBuffer>.

Here's the output on the last GC for a JetStream run:

> GCIncomingRefCountedSet::sweep(): m_vector[24]
>   object 0x3f08a8780, deferred 1, refCount 0, 16 incoming references
>   object 0x3fc45a180, deferred 1, refCount 0, 16 incoming references
>   object 0x3f1c40140, deferred 1, refCount 0, 11 incoming references
>   object 0x3fb415b40, deferred 1, refCount 0, 12 incoming references
>   object 0x3fa48b900, deferred 1, refCount 0, 16 incoming references
>   object 0x3fb8cbdc0, deferred 1, refCount 0, 12 incoming references
>   object 0x3f2d489c0, deferred 1, refCount 0, 11 incoming references
>   object 0x3fbf72100, deferred 1, refCount 0, 11 incoming references
>   object 0x3f7278c00, deferred 1, refCount 0, 16 incoming references
>   object 0x4073a84c0, deferred 1, refCount 0, 16 incoming references
>   object 0x3f0da24c0, deferred 1, refCount 0, 11 incoming references
>   object 0x3f2696540, deferred 1, refCount 0, 12 incoming references
>   object 0x3fc663280, deferred 1, refCount 0, 16 incoming references
>   object 0x3f2d86c80, deferred 1, refCount 0, 12 incoming references
>   object 0x3f2bf1100, deferred 1, refCount 0, 11 incoming references
>   object 0x3f5dfca00, deferred 1, refCount 0, 11 incoming references
>   object 0x3f6c6a940, deferred 1, refCount 0, 16 incoming references
>   object 0x3f53a2340, deferred 1, refCount 0, 16 incoming references
>   object 0x415615980, deferred 1, refCount 0, 13 incoming references
>   object 0x3f8508ec0, deferred 1, refCount 0, 14 incoming references
>   object 0x3ed269140, deferred 1, refCount 0, 44 incoming references
>   object 0x3f6255700, deferred 1, refCount 0, 16 incoming references
>   object 0x40170e2c0, deferred 1, refCount 0, 13 incoming references
>   object 0x411f0f280, deferred 1, refCount 0, 13 incoming references
>
> bytesAllocatedThisCycle = 313249
> totalBytesVisited = 13801072, currentHeapSize = 13801072
> extraMemorySize() = 759166003, currentHeapSize = 772967075
>   m_extraMemorySize = 4189747, m_deprecatedExtraMemorySize = 0, m_arrayBuffers.size() = 754976256
> Full: maxHeapSize = 1545934150
> Full: maxEdenSize = 772967075
> Full: sizeAfterLastFullCollect = 772967075
> Full: bytesAbandonedSinceLastFullCollect = 0
> sizeAfterLastCollect = 772967075
Comment 2 Ryosuke Niwa 2019-03-09 19:37:34 PST
Hm... it's hard to believe that we'd only leak ArrayBuffer with that change. Perhaps we're leaking the entire document / frame?
Comment 3 Chris Dumez 2019-03-09 20:23:01 PST
We may need to call JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(this); in DOMWindow::willDetachDocumentFromFrame().
Comment 4 Chris Dumez 2019-03-10 15:23:41 PDT
(In reply to Chris Dumez from comment #3)
> We may need to call
> JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(this); in
> DOMWindow::willDetachDocumentFromFrame().

It does not seem to help anything unfortunately.
Comment 5 Chris Dumez 2019-03-10 15:35:16 PDT
(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > We may need to call
> > JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(this); in
> > DOMWindow::willDetachDocumentFromFrame().
> 
> It does not seem to help anything unfortunately.

That said, I do not see any difference either if I comment out the code in Document::frameWasDisconnectedFromOwner() which I believe would restore pre-r236862 behavior.
Comment 6 Zan Dobersek 2019-03-14 13:39:30 PDT
The problem seems to be a lack of call to InspectorInstrumentation::frameWindowDiscarded(). Before r236862 this was called from Frame::willDetachPage(), through the FrameDestructionObserver interface.
http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/DOMWindow.cpp?rev=236862#L484

Starting in r236862 this was not being called anymore because DOMWindow was already decoupled from Frame when Frame::willDetachPage() was invoked and subsequently the m_destructionObservers set was empty.

The situation in trunk is similar, only difference is that after further refactoring the InspectorInstrumentation::frameWindowDiscarded() is now placed in Document::willDetachPage().
Comment 7 Chris Dumez 2019-03-14 13:40:52 PDT
(In reply to Zan Dobersek from comment #6)
> The problem seems to be a lack of call to
> InspectorInstrumentation::frameWindowDiscarded(). Before r236862 this was
> called from Frame::willDetachPage(), through the FrameDestructionObserver
> interface.
> http://trac.webkit.org/browser/webkit/trunk/Source/WebCore/page/DOMWindow.
> cpp?rev=236862#L484
> 
> Starting in r236862 this was not being called anymore because DOMWindow was
> already decoupled from Frame when Frame::willDetachPage() was invoked and
> subsequently the m_destructionObservers set was empty.
> 
> The situation in trunk is similar, only difference is that after further
> refactoring the InspectorInstrumentation::frameWindowDiscarded() is now
> placed in Document::willDetachPage().

Then we probably need a call to InspectorInstrumentation::frameWindowDiscarded() in DOMWindow::willDetachDocumentFromFrame().
Comment 8 Zan Dobersek 2019-03-15 07:50:06 PDT
Created attachment 364796 [details]
EWS run
Comment 9 Build Bot 2019-03-15 07:54:06 PDT
Attachment 364796 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Chris Dumez 2019-03-18 13:41:54 PDT
Created attachment 365054 [details]
Patch
Comment 11 WebKit Commit Bot 2019-03-18 14:27:14 PDT
Comment on attachment 365054 [details]
Patch

Clearing flags on attachment: 365054

Committed r243104: <https://trac.webkit.org/changeset/243104>
Comment 12 WebKit Commit Bot 2019-03-18 14:27:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-03-18 14:28:25 PDT
<rdar://problem/48993555>