This will make the Objective-C wrappers for API objects unique and persistent.
Created attachment 215263 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Comment on attachment 215263 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects Attachment 215263 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/11458028 New failing tests: fast/forms/document-write.html fast/events/input-image-scrolled-x-y.html fast/forms/fallback-content-submission.html fast/forms/mailto/formenctype-attribute-input-html.html fast/encoding/char-encoding.html http/tests/cache/subresource-failover-to-network.html http/tests/history/replacestate-post-to-get.html fast/events/popup-allowed-from-gesture-initiated-form-submit.html fast/dom/open-and-close-by-DOM.html http/tests/history/back-to-post.php fast/encoding/char-encoding-mac.html fast/encoding/percent-escaping.html fast/forms/mailto/advanced-put.html fast/events/popup-blocked-to-post-blank.html fast/forms/empty-get.html fast/dom/navigator-cookieEnabled-no-crash.html http/tests/history/redirect-js-form-submit-2-seconds.html http/tests/cache/post-with-cached-subresources.php fast/forms/mailto/get-multiple-items-x-www-form-urlencoded.html http/tests/history/replacestate-post-to-get-2.html fast/forms/mailto/get-non-ascii-always-utf-8.html http/tests/history/redirect-js-form-submit-0-seconds.html fast/dom/HTMLButtonElement/value/getset.html fast/forms/radio/interactive-validation-required-radio.html fast/events/popup-when-select-change.html fast/forms/button-state-restore.html fast/dom/cssTarget-crash.html http/tests/cache/post-redirect-get.php fast/encoding/mailto-always-utf-8.html fast/forms/number/number-interactive-validation-required.html
Created attachment 215266 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #3) > Created an attachment (id=215266) [details] > Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 > > The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. > Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5 Oops. Should initialize m_wrapper to 0 unconditionally.
The crashes are happening because WebFrameListenerProxy derives directly from APIObject rather than a TypedAPIObject. Not sure if this is just a mistake.
It’s possible to not allocate the wrappers separately, at least in some cases. We should probably do that.
Created attachment 215418 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Attachment 215418 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/Cocoa/APIObject.mm', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/Shared/Cocoa/WKObject.h', u'Source/WebKit2/Shared/Cocoa/WebString.mm', u'Source/WebKit2/Shared/Cocoa/WebURL.mm', u'Source/WebKit2/Shared/WebString.h', u'Source/WebKit2/Shared/WebURL.h', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h', u'Source/WebKit2/UIProcess/WebFormSubmissionListenerProxy.h', u'Source/WebKit2/UIProcess/WebFramePolicyListenerProxy.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1 Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:40: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 215418 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects Foiled by CF’s shared strings optimization.
Created attachment 215464 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Created attachment 215467 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Attachment 215467 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/Cocoa/APIObject.mm', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/Shared/Cocoa/WKNSString.h', u'Source/WebKit2/Shared/Cocoa/WKNSString.mm', u'Source/WebKit2/Shared/Cocoa/WKNSURL.h', u'Source/WebKit2/Shared/Cocoa/WKNSURL.mm', u'Source/WebKit2/Shared/Cocoa/WKObject.h', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h', u'Source/WebKit2/UIProcess/WebFormSubmissionListenerProxy.h', u'Source/WebKit2/UIProcess/WebFramePolicyListenerProxy.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1 Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:40: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 215482 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Attachment 215482 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/Cocoa/APIObject.mm', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/Shared/Cocoa/WKNSString.h', u'Source/WebKit2/Shared/Cocoa/WKNSString.mm', u'Source/WebKit2/Shared/Cocoa/WKNSURL.h', u'Source/WebKit2/Shared/Cocoa/WKNSURL.mm', u'Source/WebKit2/Shared/Cocoa/WKObject.h', u'Source/WebKit2/Shared/Cocoa/WKObject.mm', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h', u'Source/WebKit2/UIProcess/WebFormSubmissionListenerProxy.h', u'Source/WebKit2/UIProcess/WebFramePolicyListenerProxy.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1 Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:40: Extra space before ( in function call [whitespace/parens] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:37: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 215482 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects View in context: https://bugs.webkit.org/attachment.cgi?id=215482&action=review > Source/WebKit2/Shared/APIObject.h:31 > +#if PLATFORM(MAC) Maybe we should start phasing out PLATFORM(MAC) for these purposes and replacing it with USE(COCOA). At first the two would be identical. Then, some day in the future we could make PLATFORM(MAC) be false on iOS. > Source/WebKit2/Shared/APIObject.h:38 > +#if !(PLATFORM(MAC) && WK_API_ENABLED) For clarity, it might be worthwhile to define a derived constant for this header: #define DELEGATE_REF_COUNTING_TO_COCOA (PLATFORM(MAC) && WK_API_ENABLED) We could then #undef it at the bottom of the header. > Source/WebKit2/Shared/APIObject.h:163 > + void* wrapper() { return m_wrapper; } We should consider using objc_object* here instead of void*, to avoid all the (id) casts we otherwise need. With appropriate forward declaration so we can compile it in plain C++. Or even NSObject * and use the OBJC_CLASS technique. > Source/WebKit2/Shared/APIObject.h:177 > + void* operator new(size_t) = delete; Does this actually work and create a compile time error if you forget to override operator new? I remember trying this a long time ago to force arena allocation, and I failed. > Source/WebKit2/Shared/Cocoa/APIObject.mm:51 > + switch (type) { Maybe need a comment about the different idioms needed for variable-sized vs. fixed-sized object? Otherwise it’s mysterious why these are all different. If there was no need for the two different idioms I would suggest factoring the code differently so that we would just map the type to the wrapper class rather than having separate code to allocate each type of wrapper. > Source/WebKit2/Shared/Cocoa/WKNSArray.h:34 > +inline NSArray *wrapper(ImmutableArray& item) { return (NSArray *)item.wrapper(); } Unfortunate that the typecast is here in a non-member function far from the guarantee that the wrapper has the appropriate class. It would be nice if it was in ImmutableArray itself did the typecast, but I guess even that isn’t all that close to the guarantee of the wrapper type. And I suppose that doing the cast here has the benefit of keeping it out of the platform-independent header. Also, would be nice to have an assertion that the cast is correct. > Source/WebKit2/Shared/Cocoa/WKNSArray.mm:34 > + uint8_t _array[sizeof(ImmutableArray)]; Does this guarantee sufficient alignment? In WTF we use std::aligned_storage to guarantee that. I also think that the field should be named _arrayStorage so it’s clearer why a typecast is needed at each place that uses it. > Source/WebKit2/Shared/Cocoa/WKNSURL.mm:41 > + return (NSURL *)CFMakeCollectable(WKURLCopyCFURL(kCFAllocatorDefault, toAPI(reinterpret_cast<WebURL*>(&self._apiObject)))); I don’t think we support GC with WebKit2, so maybe you could just omit CFMakeCollectable entirely. We should be using CFBridgingRelease rather than CFMakeCollectable in call sites like these now. I was surprised to find some CFMakeCollectable calls still in our code. But I guess CFBridgingRelease would get the retain/release count wrong and you’d need to also call retain. > Source/WebKit2/Shared/Cocoa/WKObject.mm:37 > + BOOL _initializedTarget; > + NSObject *_target; Any reason we can’t just use null for _target instead of a separate boolean? I suppose there is. Also, not 100% happy with “initialized target” as the field name. Maybe _hasInitializedTarget? > Source/WebKit2/Shared/Cocoa/WKObject.mm:136 > + return nil; Should this also be doing an “assert not reached” sort of thing? Or are there some that have no target? I think it’s a little confusing that our wrapper is called our “target”.
(In reply to comment #15) > (From update of attachment 215482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215482&action=review > > > Source/WebKit2/Shared/APIObject.h:31 > > +#if PLATFORM(MAC) > > Maybe we should start phasing out PLATFORM(MAC) for these purposes and replacing it with USE(COCOA). At first the two would be identical. Then, some day in the future we could make PLATFORM(MAC) be false on iOS. I think that USE(COCOA) is a little weird, since it’s not really possible to build Mac or iOS with USE(COCOA) == false. I thought we were going to add a PLATFORM(COCOA).
Comment on attachment 215482 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects View in context: https://bugs.webkit.org/attachment.cgi?id=215482&action=review Thanks for the review, Darin! I found what I think is one mistake in this patch, and I want to improve one aspect of it, so I am planning on posting another revision. Some comments below. >>> Source/WebKit2/Shared/APIObject.h:31 >>> +#if PLATFORM(MAC) >> >> Maybe we should start phasing out PLATFORM(MAC) for these purposes and replacing it with USE(COCOA). At first the two would be identical. Then, some day in the future we could make PLATFORM(MAC) be false on iOS. > > I think that USE(COCOA) is a little weird, since it’s not really possible to build Mac or iOS with USE(COCOA) == false. I thought we were going to add a PLATFORM(COCOA). I’d be happy to change this to PLATFORM(COCOA). >> Source/WebKit2/Shared/APIObject.h:38 >> +#if !(PLATFORM(MAC) && WK_API_ENABLED) > > For clarity, it might be worthwhile to define a derived constant for this header: > > #define DELEGATE_REF_COUNTING_TO_COCOA (PLATFORM(MAC) && WK_API_ENABLED) > > We could then #undef it at the bottom of the header. I like that. >> Source/WebKit2/Shared/APIObject.h:163 >> + void* wrapper() { return m_wrapper; } > > We should consider using objc_object* here instead of void*, to avoid all the (id) casts we otherwise need. With appropriate forward declaration so we can compile it in plain C++. Or even NSObject * and use the OBJC_CLASS technique. I’ll experiment with those ideas. >> Source/WebKit2/Shared/APIObject.h:177 >> + void* operator new(size_t) = delete; > > Does this actually work and create a compile time error if you forget to override operator new? I remember trying this a long time ago to force arena allocation, and I failed. Yes. It’s how I caught WebColorPickerResultListenerProxy. >> Source/WebKit2/Shared/Cocoa/APIObject.mm:51 >> + switch (type) { > > Maybe need a comment about the different idioms needed for variable-sized vs. fixed-sized object? Otherwise it’s mysterious why these are all different. > > If there was no need for the two different idioms I would suggest factoring the code differently so that we would just map the type to the wrapper class rather than having separate code to allocate each type of wrapper. I could add that comment. The current form has the slight advantage that the compiler knows the type of the result of alloc and verifies that it conforms to WKObject. >> Source/WebKit2/Shared/Cocoa/WKNSArray.mm:34 >> + uint8_t _array[sizeof(ImmutableArray)]; > > Does this guarantee sufficient alignment? In WTF we use std::aligned_storage to guarantee that. I also think that the field should be named _arrayStorage so it’s clearer why a typecast is needed at each place that uses it. Good question! I’ll look into the alignment issue. I’m not sure I like adding the word storage to the ivar name. >> Source/WebKit2/Shared/Cocoa/WKNSURL.mm:41 >> + return (NSURL *)CFMakeCollectable(WKURLCopyCFURL(kCFAllocatorDefault, toAPI(reinterpret_cast<WebURL*>(&self._apiObject)))); > > I don’t think we support GC with WebKit2, so maybe you could just omit CFMakeCollectable entirely. > > We should be using CFBridgingRelease rather than CFMakeCollectable in call sites like these now. I was surprised to find some CFMakeCollectable calls still in our code. But I guess CFBridgingRelease would get the retain/release count wrong and you’d need to also call retain. Right. CFMakeCollectable is here to make the clang static analyzer happy for as long as we’re not building this code with ARC. For whatever reason, the analyzer then wants the code to be GC-safe. >> Source/WebKit2/Shared/Cocoa/WKObject.mm:37 >> + NSObject *_target; > > Any reason we can’t just use null for _target instead of a separate boolean? I suppose there is. > > Also, not 100% happy with “initialized target” as the field name. Maybe _hasInitializedTarget? We have a separate boolean because in the general case, _web_createTarget may return nil (I even thought that there’d be WebURLs that won’t be representable as CFURLs, but the CFURL creation functions that we’re using seem to be really good at not returning NULL), and we still don’t want to call it repeatedly. But I am going to try and replace the boolean with a dispatch_once_t and rework how we initialize the target. >> Source/WebKit2/Shared/Cocoa/WKObject.mm:136 >> + return nil; > > Should this also be doing an “assert not reached” sort of thing? Or are there some that have no target? > > I think it’s a little confusing that our wrapper is called our “target”. This method is reached for any APIObject Type that doesn’t have a more specific mapping in APIObject::newObject. If we really cared we could make a separate kind of generic wrapper for that case, but it seemed unnecessary to do so. It’s a little confusing, but self is the wrapper (for example, it’s the object returned by APIObject::wrapper()), and the target is an implementation detail. I agree that the name “target” isn’t great.
Created attachment 215538 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects
Attachment 215538 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/Shared/Cocoa/APIObject.mm', u'Source/WebKit2/Shared/Cocoa/WKNSArray.h', u'Source/WebKit2/Shared/Cocoa/WKNSArray.mm', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.h', u'Source/WebKit2/Shared/Cocoa/WKNSObjectExtras.mm', u'Source/WebKit2/Shared/Cocoa/WKNSString.h', u'Source/WebKit2/Shared/Cocoa/WKNSString.mm', u'Source/WebKit2/Shared/Cocoa/WKNSURL.h', u'Source/WebKit2/Shared/Cocoa/WKNSURL.mm', u'Source/WebKit2/Shared/Cocoa/WKObject.h', u'Source/WebKit2/Shared/Cocoa/WKObject.mm', u'Source/WebKit2/UIProcess/API/mac/WKBrowsingContextController.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardList.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItem.mm', u'Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h', u'Source/WebKit2/UIProcess/WebFormSubmissionListenerProxy.h', u'Source/WebKit2/UIProcess/WebFramePolicyListenerProxy.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj']" exit_code: 1 Source/WebKit2/Shared/Cocoa/WKNSArray.h:34: More than one command on the same line [whitespace/newline] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListItemInternal.h:34: More than one command on the same line [whitespace/newline] [4] Source/WebKit2/UIProcess/Cocoa/WKBackForwardListInternal.h:34: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 215538 [details] Give APIObject an Objective-C wrapper and make wrappers manage the lifetime of API objects View in context: https://bugs.webkit.org/attachment.cgi?id=215538&action=review > Source/WebKit2/Shared/Cocoa/WKObject.mm:36 > + dispatch_once_t _targetInitializtionToken; Missing a in “ztion” here.
Committed <http://trac.webkit.org/r158324>.