opaqueJSClassData stores cached prototypes for JSClassRefs in the C API. It doesn't make sense to share these prototypes within a JSGlobalData across JSGlobalObjects, and in fact doing so will cause a leak of the original JSGlobalObject that these prototypes were created in. Therefore we should move this cache to JSGlobalObject where it belongs and where it won't cause memory leaks.
Created attachment 194635 [details] Patch
<rdar://problem/12269677>
Comment on attachment 194635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194635&action=review r=me > Source/JavaScriptCore/API/tests/testapi.c:148 > + if (!n) { So Scheme-y!
Comment on attachment 194635 [details] Patch Attachment 194635 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17293113
Committed r146682: <http://trac.webkit.org/changeset/146682>
This patch caused multiple build failures on Windows. The last compilation error we're seeing is: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/46467/steps/compile-webkit/logs/stdio 8>####### COMPILING 1 FILES USING AT MOST 8 PARALLEL INSTANCES OF cl.exe ########### 8>testapi.c 8>..\..\API\tests\testapi.c(146) : error C2332: 'class' : missing tag name 8>..\..\API\tests\testapi.c(146) : error C2236: unexpected 'class' '<unnamed-tag>'. Did you forget a ';'? 8>..\..\API\tests\testapi.c(149) : error C2332: 'class' : missing tag name 8>..\..\API\tests\testapi.c(149) : error C2226: syntax error : unexpected type '<unnamed-tag>' 8>..\..\API\tests\testapi.c(156) : error C2332: 'class' : missing tag name 8>..\..\API\tests\testapi.c(156) : error C2226: syntax error : unexpected type '<unnamed-tag>' 8>..\..\API\tests\testapi.c(169) : error C2664: 'nestedAllocateObject' : cannot convert parameter 2 from 'JSClassRef' to '' 8> No constructor could take the source type, or constructor overload resolution was ambiguous
I've spent sometime looking into this but I couldn't figure out how to fix so I've wrapped inside !OS(WIN) for now :(
in http://trac.webkit.org/changeset/146707.
It seems like this patch also regressed jscore-test? http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7233 FAIL: Failed to finalize the original object after the first GC. PASS: Infinite prototype chain does not occur. PASS: A cycle in a prototype chain can't be created. FAIL: Some tests failed.
(In reply to comment #9) > It seems like this patch also regressed jscore-test? > > http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7233 > > FAIL: Failed to finalize the original object after the first GC. > PASS: Infinite prototype chain does not occur. > PASS: A cycle in a prototype chain can't be created. > FAIL: Some tests failed. Hmm. This could be due to my hack around the conservative GC not working on other platforms.
> Hmm. This could be due to my hack around the conservative GC not working on other platforms. And by "not working" I mean it keeps objects alive that are actually dead due to its conservatism.
(In reply to comment #10) > (In reply to comment #9) > > It seems like this patch also regressed jscore-test? > > > > http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/7233 > > > > FAIL: Failed to finalize the original object after the first GC. > > PASS: Infinite prototype chain does not occur. > > PASS: A cycle in a prototype chain can't be created. > > FAIL: Some tests failed. > > Hmm. This could be due to my hack around the conservative GC not working on other platforms. This is Mountain Lion though. Should we revert the patch for now? I don't know how we can suppress JS test failures.
> This is Mountain Lion though. Should we revert the patch for now? I don't know how we can suppress JS test failures. Sorry I should be more clear. I'm pretty sure the fix itself is correct. I believe it's the test that's flawed. It's not that it's Mountain Lion specifically, it's just that the conservative GC might keep things alive depending on how things are compiled. I tried to hack around this issue in the test, but it seems to not be working on whatever bot is running the tests.
I see. Thanks for the clarification. What should we do about the failing tests though?
(In reply to comment #14) > I see. Thanks for the clarification. What should we do about the failing tests though? Hmm good question. I'd say roll out the test. I got this test working locally, so I'm pretty confident in the correctness of the fix itself. Maybe we should file a bug to fix the test and add it back.
(In reply to comment #15) > (In reply to comment #14) > > I see. Thanks for the clarification. What should we do about the failing tests though? > > Hmm good question. I'd say roll out the test. I got this test working locally, so I'm pretty confident in the correctness of the fix itself. Maybe we should file a bug to fix the test and add it back. Okay. I've posted a patch to remove the test on https://bugs.webkit.org/show_bug.cgi?id=113125. Could you take a look at it?