REGRESSION(r236862): early frame decoupling leaves JSC ArrayBuffer objects lingering

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

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.

[Zan Dobersek]

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

[Ryosuke Niwa]

Hm... it's hard to believe that we'd only leak ArrayBuffer with that change. Perhaps we're leaking the entire document / frame?

[Chris Dumez]

We may need to call JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(this); in DOMWindow::willDetachDocumentFromFrame().

[Chris Dumez]

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

[Chris Dumez]

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

[Zan Dobersek]

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

[Chris Dumez]

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

[Zan Dobersek]

Created attachment 364796 [details]
EWS run

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 365054 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365054 [details]
Patch

Clearing flags on attachment: 365054

Committed r243104: <https://trac.webkit.org/changeset/243104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48993555>