When asking a WKBundleHitTestResultRef for its nodeHandle, the InjectedBundleNodeHandle dies too early in WKBundleHitTestResultGetNodeHandle. WebKit.dll!WebKit::InjectedBundleNodeHandle::~InjectedBundleNodeHandle() Line 90 C++ WebKit.dll!WebKit::InjectedBundleNodeHandle::`scalar deleting destructor'() + 0x16 bytes C++ WebKit.dll!WTF::RefCounted<WebKit::APIObject>::deref() Line 141 + 0x3a bytes C++ WebKit.dll!WTF::derefIfNotNull<WebKit::InjectedBundleNodeHandle>(WebKit::InjectedBundleNodeHandle * ptr=0x03194368) Line 60 C++ WebKit.dll!WTF::RefPtr<WebKit::InjectedBundleNodeHandle>::~RefPtr<WebKit::InjectedBundleNodeHandle>() Line 58 + 0x19 bytes C++ > WebKit.dll!WKBundleHitTestResultGetNodeHandle(const OpaqueWKBundleHitTestResult * hitTestResultRef=0x0323a848) Line 44 + 0x1c bytes C++
Created attachment 83200 [details] patch I don't know if this is right... the other functions that use release().leakRef() or release.releaseRef() that return WKBundleNodeHandleRef seem to have "copy" in their function names, but not WKBundleHitTestResultGetNodeHandle.
Comment on attachment 83200 [details] patch My guess is that you're fixing a crash. It would be good to have "crash" somewhere in the bug title (which would also get it into your ChangeLog). As you suspected, this change makes WKBundleHitTestResultGetNodeHandle follow the Create or Copy pattern, so its name will have to be changed. The actual code change seems fine. Is it possible to make a test for this? I'd imagine it is, by using TestWebKitAPI.
Created attachment 83402 [details] API test
Attachment 83402 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/In..." exit_code: 1 Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:47: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 83402 [details] API test View in context: https://bugs.webkit.org/attachment.cgi?id=83402&action=review Awesome that you got a test working! > Tools/TestWebKitAPI/PlatformWebView.h:65 > + void simulateRightClick(CGPoint pt); No need for "pt" here. (If you did need an argument name, "point" would be better.) Maybe it should be a const CGPoint&? It's slightly unfortunate to use CG types in cross-platform code, as this will make it harder for other ports to build TestWebKitAPI. Maybe two unsigneds would be better. > Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:70 > + WKContextInjectedBundleClient injectedBundleClient; > + memset(&injectedBundleClient, 0, sizeof(injectedBundleClient)); > + injectedBundleClient.version = 0; > + injectedBundleClient.clientInfo = 0; > + injectedBundleClient.didReceiveMessageFromInjectedBundle = didReceiveMessageFromInjectedBundle; > + WKContextSetInjectedBundleClient(context.get(), &injectedBundleClient); A separate setInjectedBundleClient function would make the main test logic easier to follow. > Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:82 > + TEST_ASSERT(done); No need for this assertion, as it will never be reached if done is still false. > Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle_Bundle.cpp:49 > + WKRetainPtr<WKStringRef> doneMessageName(AdoptWK, WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageName")); > + WKRetainPtr<WKStringRef> doneMessageBody(AdoptWK, WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageBody")); You can use the Util::toWK function here instead. > Tools/TestWebKitAPI/win/PlatformWebViewWin.cpp:109 > + ::SendMessage(window, WM_RBUTTONDOWN, 0, MAKELPARAM(pt.x, pt.y)); > + ::SendMessage(window, WM_RBUTTONUP, 0, MAKELPARAM(pt.x, pt.y)); Please use ::SendMessageW.
Comment on attachment 83402 [details] API test View in context: https://bugs.webkit.org/attachment.cgi?id=83402&action=review > Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle_Bundle.cpp:44 > + WKBundleNodeHandleRef nodeHandle = WKBundleHitTestResultGetNodeHandle(hitTestResult); Presumably you're leaking this nodeHandle, now that you've changed the API to a Copy-style API.
Created attachment 84163 [details] all-in-one patch Made a tweak to the test. changed to use a WKRetainPtr so that the object that is returned from the (fixed) API call is ref'ed, so that it will crash in builds where the API call is not fixed. also renamed function and other suggestions adam made.
Attachment 84163 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:47: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 84164 [details] same as above, but fixed whitespace style nit
Comment on attachment 84164 [details] same as above, but fixed whitespace style nit Looks great.
committed http://trac.webkit.org/changeset/79966