Bug 113086 - opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData
Summary: opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-22 11:26 PDT by Mark Hahnenberg
Modified: 2013-03-22 21:17 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.31 KB, patch)
2013-03-22 14:52 PDT, Mark Hahnenberg
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-03-22 11:26:06 PDT
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.
Comment 1 Mark Hahnenberg 2013-03-22 14:52:02 PDT
Created attachment 194635 [details]
Patch
Comment 2 Mark Hahnenberg 2013-03-22 14:52:33 PDT
<rdar://problem/12269677>
Comment 3 Geoffrey Garen 2013-03-22 15:18:37 PDT
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 4 Build Bot 2013-03-22 15:26:32 PDT
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
Comment 5 Mark Hahnenberg 2013-03-22 16:52:20 PDT
Committed r146682: <http://trac.webkit.org/changeset/146682>
Comment 6 Ryosuke Niwa 2013-03-22 20:12:53 PDT
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
Comment 7 Ryosuke Niwa 2013-03-22 20:15:47 PDT
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 :(
Comment 8 Ryosuke Niwa 2013-03-22 20:16:06 PDT
in http://trac.webkit.org/changeset/146707.
Comment 9 Ryosuke Niwa 2013-03-22 20:46:14 PDT
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.
Comment 10 Mark Hahnenberg 2013-03-22 20:58:08 PDT
(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.
Comment 11 Mark Hahnenberg 2013-03-22 20:59:45 PDT
> 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.
Comment 12 Ryosuke Niwa 2013-03-22 20:59:59 PDT
(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.
Comment 13 Mark Hahnenberg 2013-03-22 21:04:56 PDT
> 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.
Comment 14 Ryosuke Niwa 2013-03-22 21:06:13 PDT
I see. Thanks for the clarification. What should we do about the failing tests though?
Comment 15 Mark Hahnenberg 2013-03-22 21:10:56 PDT
(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.
Comment 16 Ryosuke Niwa 2013-03-22 21:17:03 PDT
(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?