Bug 186579 – Share structure across instances of classes exported through the ObjC API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tadeu Zagallo]

Share structure across instances of classes exported through the ObjC API

[Tadeu Zagallo]

Created attachment 342610 [details]
Patch

[Tadeu Zagallo]

<rdar://problem/40969212>

[EWS Watchlist]

Comment on attachment 342610 [details]
Patch

Attachment 342610 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8164540

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html

[EWS Watchlist]

Created attachment 342672 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Keith Miller]

Comment on attachment 342610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342610&action=review

r- since I don't think one of the changes is warranted but other than that it looks good. It would also be good to add an API test for this: allocate like 50,000 api objects and check the number of live structures is something reasonable (< 5000) afterwords.

> Source/JavaScriptCore/API/JSWrapperMap.mm:540
> -    if (!constructor)
> -        constructor = [self allocateConstructorAndPrototypeInContext:context].first;
> -    ASSERT(!!constructor);
> -    return constructor;
> +    if (constructor)
> +        return constructor;
> +
> +    m_constructor = [self allocateConstructorAndPrototypeInContext:context].first;
> +    ASSERT(!!m_constructor);
> +    return m_constructor.get();

Why make this change? allocateConstructorAndPrototypeInContext sets m_constructor/m_prototype so you shouldn't need to do it here.

> Source/JavaScriptCore/API/JSWrapperMap.mm:551
> -    if (!prototype)
> -        prototype = [self allocateConstructorAndPrototypeInContext:context].second;
> -    ASSERT(!!prototype);
> -    return prototype;
> +    if (prototype)
> +        return prototype;
> +
> +    m_prototype = [self allocateConstructorAndPrototypeInContext:context].second;
> +    ASSERT(!!m_prototype);
> +    return m_prototype.get();

Ditto.

[Saam Barati]

Comment on attachment 342610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342610&action=review

>> Source/JavaScriptCore/API/JSWrapperMap.mm:540
>> +    return m_constructor.get();
> 
> Why make this change? allocateConstructorAndPrototypeInContext sets m_constructor/m_prototype so you shouldn't need to do it here.

The old code never cached to our weak variable.

>> Source/JavaScriptCore/API/JSWrapperMap.mm:551
>> +    return m_prototype.get();
> 
> Ditto.

ditto

[Keith Miller]

Comment on attachment 342610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342610&action=review

>>> Source/JavaScriptCore/API/JSWrapperMap.mm:540
>>> +    return m_constructor.get();
>> 
>> Why make this change? allocateConstructorAndPrototypeInContext sets m_constructor/m_prototype so you shouldn't need to do it here.
> 
> The old code never cached to our weak variable.

Yeah, it did... right before it returned the tuple.

>>> Source/JavaScriptCore/API/JSWrapperMap.mm:551
>>> +    return m_prototype.get();
>> 
>> Ditto.
> 
> ditto

Ditto.

[Tadeu Zagallo]

Created attachment 342730 [details]
Patch

[EWS Watchlist]

Comment on attachment 342730 [details]
Patch

Attachment 342730 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8178650

New failing tests:
media/media-fragments/TC0089.html

[EWS Watchlist]

Created attachment 342733 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Saam Barati]

Comment on attachment 342730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342730&action=review

r=me

> Source/JavaScriptCore/API/JSWrapperMap.mm:521
> +    auto structure = [self structureInContext:context];

I think we tend to use auto* here or write out the type. Ditto in below uses

[Saam Barati]

Comment on attachment 342730 [details]
Patch

It would be good to add a test forth is where you assert that the structures are the same

[EWS Watchlist]

Comment on attachment 342730 [details]
Patch

Attachment 342730 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8185530

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html

[EWS Watchlist]

Created attachment 342773 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tadeu Zagallo]

Created attachment 342776 [details]
Patch

Style fixes + test

[WebKit Commit Bot]

Comment on attachment 342776 [details]
Patch

Rejecting attachment 342776 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
fno-exceptions -fno-rtti -fpascal-strings -O3 -fno-common -Werror -Wno-missing-field-initializers -Wmissing-prototypes -Wdocumentation -Wnon-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wno-missing-braces -Wparentheses -Wswitch -Wunused-function -Wno-unused-label -Wno-unused-parameter -Wunused-variable -Wunused-value -Wempty-body -Wuninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-c++11-extensions -DNDEBUG -DENABLE_3D_TRANSFORMS -DENABLE_APPLE_PAY -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_ANIMATIONS_LEVEL_2 -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CURSOR_VISIBILITY -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_INTL_NUMBER_FORMAT_TO_PARTS -DENABLE_INTL_PLURAL_RULES -DENABLE_JS_ASYNC_ITERATION -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.12 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/testapi-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/testapi-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/testapi-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/testapi-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/System/Library/Frameworks/JavaScriptCore.framework/PrivateHeaders -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/Objects-normal/x86_64/TypedArrayCTest.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/Objects-normal/x86_64/TypedArrayCTest.dia -c /Volumes/Data/EWS/WebKit/Source/JavaScriptCore/API/tests/TypedArrayCTest.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/Objects-normal/x86_64/TypedArrayCTest.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/testapi.build/Objects-normal/x86_64/JSWrapperMapTests.o API/tests/JSWrapperMapTests.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/8187046

[Tadeu Zagallo]

Created attachment 342807 [details]
Patch

[EWS Watchlist]

Comment on attachment 342807 [details]
Patch

Attachment 342807 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8199858

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 342833 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tadeu Zagallo]

Created attachment 342850 [details]
Patch

Add missing include

[WebKit Commit Bot]

Comment on attachment 342850 [details]
Patch

Clearing flags on attachment: 342850

Committed r232936: <https://trac.webkit.org/changeset/232936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.