Bug 174736 – REGRESSION(r204565): WKObject is broken

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

REGRESSION(r204565): WKObject is broken.

[Chris Dumez]

<rdar://problem/33246169>

[Chris Dumez]

Created attachment 316139 [details]
WIP Patch (does not work)

[Chris Dumez]

Created attachment 316140 [details]
WIP Patch (does not work)

[Chris Dumez]

Created attachment 316143 [details]
WIP Patch (Needs API test)

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Geoffrey Garen]

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.

[Geoffrey Garen]

Comment on attachment 316143 [details]
WIP Patch (Needs API test)

r- to release the EWS bot from eternal crash-i-tude.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

Trying to write an API test for this.

[Chris Dumez]

Created attachment 316172 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316172 [details]
Patch

Clearing flags on attachment: 316172

Committed r219764: <http://trac.webkit.org/changeset/219764>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.