Summary: | Delayed structure sweep can leak structures without bound | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, ggaren, gustavo, haraken, japhet, ossy, philn, tkent, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 96980 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2012-09-12 12:52:15 PDT
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. Created attachment 164247 [details]
Test case
Sorry, forgot to upload the test case. Here it is.
Created attachment 164251 [details]
Patch
Comment on attachment 164251 [details] Patch Attachment 164251 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13852601 Comment on attachment 164251 [details] Patch Attachment 164251 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13848719 Comment on attachment 164251 [details] Patch Attachment 164251 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13858377 Created attachment 164332 [details]
Patch
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.
Comment on attachment 164332 [details] Patch Attachment 164332 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13871280 Comment on attachment 164332 [details] Patch Attachment 164332 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13879096 Comment on attachment 164332 [details] Patch Attachment 164332 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13871282 Created attachment 164334 [details]
Patch
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.
Created attachment 164336 [details]
Patch
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.
Created attachment 164420 [details]
Patch
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.
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.
(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. > 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.
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 Committed r128813: <http://trac.webkit.org/changeset/128813> (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 (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. 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. 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 ... Created attachment 166396 [details]
Patch
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.
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.
Committed r130303: <http://trac.webkit.org/changeset/130303> |