Bug 113086 – opaqueJSClassData should be cached on JSGlobalObject, not the JSGlobalData

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

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 194635 [details]
Patch

[Mark Hahnenberg]

<rdar://problem/12269677>

[Geoffrey Garen]

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!

[Build Bot]

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

[Mark Hahnenberg]

Committed r146682: <http://trac.webkit.org/changeset/146682>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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 :(

[Ryosuke Niwa]

in http://trac.webkit.org/changeset/146707.

[Ryosuke Niwa]

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.

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

> 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.

[Ryosuke Niwa]

(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.

[Mark Hahnenberg]

> 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.

[Ryosuke Niwa]

I see. Thanks for the clarification. What should we do about the failing tests though?

[Mark Hahnenberg]

(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.

[Ryosuke Niwa]

(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?