Summary: | opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | buildbot, rniwa | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-03-22 11:26:06 PDT
Created attachment 194635 [details]
Patch
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 :( 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? |