Bug 96546 – Delayed structure sweep can leak structures without bound

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

* STEPS TO REPRODUCE
1. Load attached test case (scratch.html) in TOT.
--> Memory growth without bound

[Mark Hahnenberg]

See MarkedAllocator::tryAllocateHelper and MarkedAllocator::addBlock for why this happens. We reset the m_blocksToSweep pointer to the end of the list if we end up not sweeping anything, which is entirely possible for us to do w/o actually sweeping blocks with only Structures. We need to leave the m_blocksToSweep where it is when we add a new block and only move it forward when we actually do some sweeping.

[Mark Hahnenberg]

<rdar://problem/12286575>

[Mark Hahnenberg]

Created attachment 164247 [details]
Test case

Sorry, forgot to upload the test case. Here it is.

[Mark Hahnenberg]

Created attachment 164251 [details]
Patch

[Early Warning System Bot]

Comment on attachment 164251 [details]
Patch

Attachment 164251 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13852601

[Early Warning System Bot]

Comment on attachment 164251 [details]
Patch

Attachment 164251 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13848719

[Gyuyoung Kim]

Comment on attachment 164251 [details]
Patch

Attachment 164251 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13858377

[Mark Hahnenberg]

Created attachment 164332 [details]
Patch

[WebKit Review Bot]

Attachment 164332 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 164332 [details]
Patch

Attachment 164332 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13871280

[Gyuyoung Kim]

Comment on attachment 164332 [details]
Patch

Attachment 164332 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13879096

[Early Warning System Bot]

Comment on attachment 164332 [details]
Patch

Attachment 164332 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13871282

[Mark Hahnenberg]

Created attachment 164334 [details]
Patch

[WebKit Review Bot]

Attachment 164334 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 164336 [details]
Patch

[WebKit Review Bot]

Attachment 164336 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 164420 [details]
Patch

[WebKit Review Bot]

Attachment 164420 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 164420 [details]
Patch

"dtorType" - please rename to "destructorType" – WebKit style doesn't allow abbreviation.

Is the idea that pretty much all classes that subclass JSNonFinalObject have been changed to subclass JSDestructibleObject? – if so, I'd suggest it could be a good idea to rename JSNonFinalObject, to enforce this & ensure no classes have been missed.  Even if you don't want to land this, might be worth doing a quick rename & floating a patch for the EWS system to test.

[Mark Hahnenberg]

(In reply to comment #19)
> (From update of attachment 164420 [details])
> "dtorType" - please rename to "destructorType" – WebKit style doesn't allow abbreviation.

The compiler gets mad because destructorType is the same name as the accessor function.

> 
> Is the idea that pretty much all classes that subclass JSNonFinalObject have been changed to subclass JSDestructibleObject? – if so, I'd suggest it could be a good idea to rename JSNonFinalObject, to enforce this & ensure no classes have been missed.  Even if you don't want to land this, might be worth doing a quick rename & floating a patch for the EWS system to test.

That's not quite the idea. There are some specific subclasses of JSNonFinalObject that *should not* have this extra field added, e.g. JSScope and some other stuff. That's why I used a finalizer for JSGlobalObject so as not to force JSScope to inherit from JSDestructibleObject. So we're trying to strike a balance between needing to access an object's ClassInfo safely during sweeping and not forcing too many other classes that don't need this to waste the extra space.

I could still use your suggestion and rename JSNonFinalObject to make sure that I didn't miss any subclasses of JSNonFinalObject that need to inherit from JSDestructibleObject instead.

[Mark Hahnenberg]

> I could still use your suggestion and rename JSNonFinalObject to make sure that I didn't miss any subclasses of JSNonFinalObject that need to inherit from JSDestructibleObject instead.

Actually, this still won't guarantee anything because there could be subclasses that would still not be caught by the compiler. I'll just have to land and keep my eye on the bots.

[WebKit Review Bot]

Comment on attachment 164420 [details]
Patch

Rejecting attachment 164420 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
orJS.pm
patching file Source/WebCore/bridge/runtime_array.cpp
patching file Source/WebCore/bridge/runtime_array.h
patching file Source/WebCore/bridge/runtime_object.cpp
patching file Source/WebCore/bridge/runtime_object.h
patching file Source/WebCore/bridge/objc/objc_runtime.h
patching file Source/WebCore/bridge/objc/objc_runtime.mm

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gavin Barr..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13878411

[Mark Hahnenberg]

Committed r128813: <http://trac.webkit.org/changeset/128813>

[Kent Tamura]

(In reply to comment #23)
> Committed r128813: <http://trac.webkit.org/changeset/128813>

This made all of tests on Apple Windows port crash.

http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r128813%20(27780)/accessibility/accessibility-node-memory-management-crash-log.txt

[Mark Hahnenberg]

(In reply to comment #24)
> (In reply to comment #23)
> > Committed r128813: <http://trac.webkit.org/changeset/128813>
> 
> This made all of tests on Apple Windows port crash.
> 
> http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r128813%20(27780)/accessibility/accessibility-node-memory-management-crash-log.txt

Looking into it.

[Mark Hahnenberg]

Looks like the ArrayPrototype thinks it has a normal destructor, but it shouldn't need one. I think our compiler magic we do with the NeedsDestructor template has finally caught up with us. I'll try adding support for MSVC.

[Csaba Osztrogonác]

Reopen, because I had to roll it out by https://trac.webkit.org/changeset/128851, because it made all JSC and layout test fail on EFL, GTK, Qt bots ...

[Mark Hahnenberg]

Created attachment 166396 [details]
Patch

[WebKit Review Bot]

Attachment 166396 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackConstr..." exit_code: 1
Source/JavaScriptCore/runtime/JSWrapperObject.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/JSCell.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/runtime/Arguments.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 166396 [details]
Patch

r=me

FWIW, I don't think m_immortalStructureDestructorSpace buys us much. There aren't very many of these.

[Mark Hahnenberg]

Committed r130303: <http://trac.webkit.org/changeset/130303>