REGRESSION(r204565): WKObject is broken.
<rdar://problem/33246169>
Created attachment 316139 [details] WIP Patch (does not work)
Created attachment 316140 [details] WIP Patch (does not work)
Created attachment 316143 [details] WIP Patch (Needs API test)
Comment on attachment 316143 [details] WIP Patch (Needs API test) Attachment 316143 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4164485 Number of test failures exceeded the failure limit.
Created attachment 316151 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 316143 [details] WIP Patch (Needs API test) Attachment 316143 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4164459 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 316154 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 316143 [details] WIP Patch (Needs API test) View in context: https://bugs.webkit.org/attachment.cgi?id=316143&action=review > Source/WebKit/Shared/Cocoa/WKObject.mm:33 > +@interface NSObject () I think maybe this is supposed to be @interface NSProxy or @interface WKObject. Do we even need this declaration at all? We need to implement these methods because they're called at runtime, but I don't think we need to declare them for anyone's direct benefit.
Comment on attachment 316143 [details] WIP Patch (Needs API test) r- to release the EWS bot from eternal crash-i-tude.
(In reply to Geoffrey Garen from comment #9) > Comment on attachment 316143 [details] > WIP Patch (Needs API test) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316143&action=review > > > Source/WebKit/Shared/Cocoa/WKObject.mm:33 > > +@interface NSObject () > > I think maybe this is supposed to be @interface NSProxy or @interface > WKObject. > > Do we even need this declaration at all? We need to implement these methods > because they're called at runtime, but I don't think we need to declare them > for anyone's direct benefit. I will double check but I am pretty sure I got compiling errors without it because this is NSObject SPI.
(In reply to Chris Dumez from comment #11) > (In reply to Geoffrey Garen from comment #9) > > Comment on attachment 316143 [details] > > WIP Patch (Needs API test) > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316143&action=review > > > > > Source/WebKit/Shared/Cocoa/WKObject.mm:33 > > > +@interface NSObject () > > > > I think maybe this is supposed to be @interface NSProxy or @interface > > WKObject. > > > > Do we even need this declaration at all? We need to implement these methods > > because they're called at runtime, but I don't think we need to declare them > > for anyone's direct benefit. > > I will double check but I am pretty sure I got compiling errors without it > because this is NSObject SPI. We call these on target, which is an NSObject so it looks correct to me.
Assertion hits in debug look like so: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010a3bb467 WTFCrash + 39 (Assertions.cpp:278) 1 com.apple.WebKit 0x000000010d09bc5f API::Object::unwrap(void*) + 111 (APIObject.mm:337) 2 com.apple.WebKit 0x000000010d1d3045 API::String* WebKit::toImpl<OpaqueWKString const*, API::String>(OpaqueWKString const*) + 21 (WKSharedAPICast.h:133) 3 com.apple.WebKit 0x000000010dc6f875 WKStringGetMaximumUTF8CStringSize + 21 (WKString.cpp:70) 4 DumpRenderTree 0x0000000108942260 dumpFramesAsText(WebFrame*) + 384 (DumpRenderTree.mm:1491) 5 DumpRenderTree 0x0000000108941b36 dump() + 374 (DumpRenderTree.mm:1699) 6 DumpRenderTree 0x0000000108960812 -[FrameLoadDelegate webView:locationChangeDone:forDataSource:] + 210 (FrameLoadDelegate.mm:169) 7 DumpRenderTree 0x0000000108961233 -[FrameLoadDelegate webView:didFinishLoadForFrame:] + 419 (FrameLoadDelegate.mm:261) 8 com.apple.WebKitLegacy 0x000000011c681a1e objc_object* wtfCallIMP<objc_object*, WebView*, objc_object*>(void (*)(), objc_object*, objc_selector*, WebView*, objc_object*) + 62 (ObjcRuntimeExtras.h:44) I believe this is: ASSERT([(id)object conformsToProtocol:@protocol(WKObject)]); in API::Object* Object::unwrap(void* object).
(In reply to Chris Dumez from comment #13) > Assertion hits in debug look like so: > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x000000010a3bb467 WTFCrash + 39 > (Assertions.cpp:278) > 1 com.apple.WebKit 0x000000010d09bc5f > API::Object::unwrap(void*) + 111 (APIObject.mm:337) > 2 com.apple.WebKit 0x000000010d1d3045 API::String* > WebKit::toImpl<OpaqueWKString const*, API::String>(OpaqueWKString const*) + > 21 (WKSharedAPICast.h:133) > 3 com.apple.WebKit 0x000000010dc6f875 > WKStringGetMaximumUTF8CStringSize + 21 (WKString.cpp:70) > 4 DumpRenderTree 0x0000000108942260 > dumpFramesAsText(WebFrame*) + 384 (DumpRenderTree.mm:1491) > 5 DumpRenderTree 0x0000000108941b36 dump() + 374 > (DumpRenderTree.mm:1699) > 6 DumpRenderTree 0x0000000108960812 -[FrameLoadDelegate > webView:locationChangeDone:forDataSource:] + 210 (FrameLoadDelegate.mm:169) > 7 DumpRenderTree 0x0000000108961233 -[FrameLoadDelegate > webView:didFinishLoadForFrame:] + 419 (FrameLoadDelegate.mm:261) > 8 com.apple.WebKitLegacy 0x000000011c681a1e objc_object* > wtfCallIMP<objc_object*, WebView*, objc_object*>(void (*)(), objc_object*, > objc_selector*, WebView*, objc_object*) + 62 (ObjcRuntimeExtras.h:44) > > I believe this is: > ASSERT([(id)object conformsToProtocol:@protocol(WKObject)]); > > in API::Object* Object::unwrap(void* object). I suppose we could just drop the assertion (and the one that immediately follows it, which we’re probably also going to fail with the patch). If we get a non-<WKObject> there it will be pretty obvious when it fails to respond to -_apiObject.
Trying to write an API test for this.
Created attachment 316172 [details] Patch
Comment on attachment 316172 [details] Patch Clearing flags on attachment: 316172 Committed r219764: <http://trac.webkit.org/changeset/219764>
All reviewed patches have been landed. Closing bug.